Skip to content

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

Open
@bcmills

Description

@bcmills

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    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.

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions