Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bytes: Test killed with quit on wasip1-wasm-wazero #61071

Closed
gopherbot opened this issue Jun 29, 2023 · 8 comments
Closed

bytes: Test killed with quit on wasip1-wasm-wazero #61071

gopherbot opened this issue Jun 29, 2023 · 8 comments
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@gopherbot
Copy link

gopherbot commented Jun 29, 2023

#!watchflakes
post <- goos == "wasip1" && pkg == "bytes" && `Test killed with quit`

Issue created automatically to collect these failures.

Example (log):

SIGQUIT: quit
PC=0x467a61 m=0 sigcode=0

goroutine 1 [running]:
	goroutine running on other thread; stack unavailable

goroutine 2 [running]:
	goroutine running on other thread; stack unavailable
created by runtime.init.6
	/usr/local/go/src/runtime/proc.go:293 +0x25
...
r12    0x0
r13    0x3
r14    0x8e69a0
r15    0xc00004f800
rip    0x467a61
rflags 0x286
cs     0x33
fs     0x0
gs     0x0
*** Test killed with quit: ran too long (4m0s).

watchflakes

@gopherbot gopherbot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 29, 2023
@gopherbot
Copy link
Author

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "bytes" && test == ""
2023-05-27 02:24 wasip1-wasm-wazero go@7ad92e95 bytes (log)
SIGQUIT: quit
PC=0x467a61 m=0 sigcode=0

goroutine 1 [running]:
	goroutine running on other thread; stack unavailable

goroutine 2 [running]:
	goroutine running on other thread; stack unavailable
created by runtime.init.6
	/usr/local/go/src/runtime/proc.go:293 +0x25
...
r12    0x0
r13    0x3
r14    0x8e69a0
r15    0xc00004f800
rip    0x467a61
rflags 0x286
cs     0x33
fs     0x0
gs     0x0
*** Test killed with quit: ran too long (4m0s).
2023-05-31 16:25 wasip1-wasm-wazero go@d329fc5b bytes (log)
SIGQUIT: quit
PC=0x467a61 m=0 sigcode=0

goroutine 1 [running]:
	goroutine running on other thread; stack unavailable

goroutine 2 [running]:
	goroutine running on other thread; stack unavailable
created by runtime.init.6
	/usr/local/go/src/runtime/proc.go:293 +0x25
...
r12    0x0
r13    0x0
r14    0x8e69a0
r15    0xc000610800
rip    0x467a61
rflags 0x286
cs     0x33
fs     0x0
gs     0x0
*** Test killed with quit: ran too long (4m0s).

watchflakes

@bcmills
Copy link
Contributor

bcmills commented Jun 29, 2023

attn @golang/wasm

@bcmills bcmills added the arch-wasm WebAssembly issues label Jun 29, 2023
@bcmills bcmills added this to the Backlog milestone Jun 29, 2023
@bcmills bcmills changed the title bytes: unrecognized failures bytes: Test killed with quit on wasip1-wasm-wazero Jun 29, 2023
@codefromthecrypt
Copy link

wazero runs this package, but breaks compilation from run.. Also, it uses the short flag...

$ cd $(gotip env GOROOT)
$ GOOS=wasip1 GOARCH=wasm gotip test -v -c -o bytes.test ./src/bytes
$ wazero run -mount=/:/ -env PWD=$PWD bytes.test -- -test.short -test.v

If the runner is not using the -test.short flag, the problem could be this. I noticed TestCompareBytes
takes a long time (not a short test)

=== RUN   TestCompareBytes
--- PASS: TestCompareBytes (47.26s)

@codefromthecrypt
Copy link

codefromthecrypt commented Jun 30, 2023

TL;DR; we may want to skip heavy, optimizable tests on wazero until its optimizing compiler is finished. Specifically, skip TestCompareBytes and cite tetratelabs/wazero#1496 which should remove the skip.

TestCompareBytes is a heavy test with optimizable code. wasmtime and wasmer have optimizers on by default. While I'm not testing on the same arch as the runner, there's a significant difference on this test when not optimized. I'm using wasmer because its optimization is simpler to disable.

$ wasmer  run --dir=/ --env PWD="$PWD" --env PATH="$PATH" --singlepass bytes.test -- -test.v  -test.run TestCompareBytes
=== RUN   TestCompareBytes
--- PASS: TestCompareBytes (26.72s)
PASS

$ wasmer  run --dir=/ --env PWD="$PWD" --env PATH="$PATH" bytes.test -- -test.v  -test.run TestCompareBytes
=== RUN   TestCompareBytes
--- PASS: TestCompareBytes (16.36s)
PASS

@johanbrandhorst
Copy link
Member

I'd prefer not to add runtime specific checks into tests. It seems like the suggestion is that this test sometimes might just take too long to run on wazero (especially on machines that are already running other tests)? I think we can wait and revisit this once wazero has an optimizing compiler in place.

@johanbrandhorst johanbrandhorst added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 30, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 30, 2023

@codefromthecrypt, by default the Go builders run the test with -short.

But it looks like TestCompareBytes intentionally runs more slowly on builders:
https://cs.opensource.google/go/go/+/master:src/bytes/compare_test.go;l=76;drc=bfa25c3f6c72829cd36f5701418f726702de9c06

Given that we now have much better longtest builder coverage than we did when that check was added, I suggest that we simply remove the || testenv.Builder() != "" from that condition, which will cause it to be skipped on the wasip1 builders (and any other builder than runs with -short too).

@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jun 30, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/507416 mentions this issue: bytes: remove builders check from compare test

@codefromthecrypt
Copy link

thanks for thinking this through, folks!

bradfitz pushed a commit to tailscale/go that referenced this issue Aug 2, 2023
TestCompareBytes already took into account the -short
testing flag, however, only if not run on one of the Go builders.
This extra condition is no longer necessary as we have much
better longtest coverage than we did when the check was added.

Fixes golang#61071

Change-Id: I0fc716f4e7baa04019ee00608f223f27c931edcc
Reviewed-on: https://go-review.googlesource.com/c/go/+/507416
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
TryBot-Bypass: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Status: Done
Development

No branches or pull requests

4 participants