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

x/tools/internal/testenv: remove ExitIfSmallMachine #64499

Open
bcmills opened this issue Dec 1, 2023 · 1 comment
Open

x/tools/internal/testenv: remove ExitIfSmallMachine #64499

bcmills opened this issue Dec 1, 2023 · 1 comment
Labels
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

@bcmills
Copy link
Contributor

bcmills commented Dec 1, 2023

Go version

N/A

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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 1, 2023
@gopherbot gopherbot added this to the Unreleased milestone Dec 1, 2023
@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 Tools This label describes issues relating to any tools in the x/tools repository. labels Dec 1, 2023
@bcmills bcmills modified the milestones: Unreleased, Backlog Dec 1, 2023
@findleyr
Copy link
Contributor

findleyr commented Dec 1, 2023

It is now used in four test binaries

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
Development

No branches or pull requests

3 participants