You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
NeedsFixThe path to resolution is known, but the work has not been done.TestingAn issue that has been verified to require only test changes, not just a test failure.
What operating system and processor architecture are you using (go env)?
N/A
What did you do?
In https://go.dev/cl/192578 (for #32834) I added a testenv.ExitIfSmallMachine helper function that causes the entire test process to exit based on heuristics about the GO_BUILDER_NAME environment variable.
What did you expect to see?
I expected that we would remove ExitIfSmallMachine by either reducing the memory footprint of the x/tools tests or increasing the available memory on the builders.
What did you see instead?
ExitIfSmallMachine has become an attractive nuisance.
It prevents go test -list from listing the tests in these binaries on affected machines, which may interfere with structured test runners (such as LUCI).
It is now used in four test binaries, and encodes assumptions about machine size for six of the Go project's builders — but those assumptions do not prevent the same tests from failing if Go users run them on their own machines with similar specs.
It too easy a tool to reach for when a test fails, and gets in the way of applying more appropriate tools.
In particular, it conflates together “slow” and “memory-constrained”, which really ought to be treated separately.
In terms of “more appropriate tools”:
For tests that are just slow, and for individual test cases that require very large amounts of memory, we should skip the tests if testing.Short() is set.
If we don't want to lose coverage in the default TryBot set / LUCI try-set, we could consider either adding longtest builders by default for the x/tools repo, or opting the test back in in short mode based on GO_BUILDER_NAME (and perhaps GOOS/GOARCH). (Builder-based opt-ins are generally much better than opt-outs because they don't cause spurious failures for external users.)
For tests that require a lot of memory due to running multiple subtests concurrently, we should either adjust the builders to pass a less aggressive -parallel flag to go test, or add explicit semaphores to restrict the concurrency of the test more than what is already implied by that flag.
The text was updated successfully, but these errors were encountered:
gopherbot
added
the
Tools
This label describes issues relating to any tools in the x/tools repository.
label
Dec 1, 2023
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
Tools
This label describes issues relating to any tools in the x/tools repository.
labels
Dec 1, 2023
It is also used in all of the gopls integration and marker tests -- a total of 13 binaries.
We can experiment with simply removing this guard, as the tests have gotten faster and smaller since it was originally added. I expect something will fail, but perhaps we can replace it with a more accurate guard (such as skipping GOOS=android, ios).
NeedsFixThe path to resolution is known, but the work has not been done.TestingAn issue that has been verified to require only test changes, not just a test failure.
Go version
N/A
What operating system and processor architecture are you using (
go env
)?What did you do?
In https://go.dev/cl/192578 (for #32834) I added a
testenv.ExitIfSmallMachine
helper function that causes the entire test process to exit based on heuristics about theGO_BUILDER_NAME
environment variable.What did you expect to see?
I expected that we would remove
ExitIfSmallMachine
by either reducing the memory footprint of thex/tools
tests or increasing the available memory on the builders.What did you see instead?
ExitIfSmallMachine
has become an attractive nuisance.go test -list
from listing the tests in these binaries on affected machines, which may interfere with structured test runners (such as LUCI).In terms of “more appropriate tools”:
For tests that are just slow, and for individual test cases that require very large amounts of memory, we should skip the tests if
testing.Short()
is set.longtest
builders by default for thex/tools
repo, or opting the test back in in short mode based onGO_BUILDER_NAME
(and perhapsGOOS
/GOARCH
). (Builder-based opt-ins are generally much better than opt-outs because they don't cause spurious failures for external users.)For tests that require a lot of memory due to running multiple subtests concurrently, we should either adjust the builders to pass a less aggressive
-parallel
flag togo test
, or add explicit semaphores to restrict the concurrency of the test more than what is already implied by that flag.The text was updated successfully, but these errors were encountered: