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/build: reduce ambiguity in long test of main Go repo #39054

Open
dmitshur opened this issue May 13, 2020 · 3 comments
Open

x/build: reduce ambiguity in long test of main Go repo #39054

dmitshur opened this issue May 13, 2020 · 3 comments

Comments

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 13, 2020

Taking into account the build infrastructure at build.golang.org, local development, and issues such as #26473, #34707, I understand we currently have the goal of having good support for two well-known testing configurations (for a given GOOS/GOARCH pair) for the main Go repository:

  • Short, as implemented by all.bash
    • target of 3-4 minutes, acceptable to skip slow tests as implemented by go test -short
  • Long, as implemented by -longtest post-submit builders (e.g., linux-amd64-longtest)
    • no goal of skipping tests for performance reasons

While investigating #29252, I found that there is some ambiguity in what it means to test the main Go repo in long mode. It's not easy to say "in order to run a long test, do this" and have a predictable outcome. We currently say "run all.bash and go test std cmd" in some places, but there's room for improvement.

We want to ensure long tests are passing for Go releases. To support that goal, I think it will helpful to reduce ambiguity in what it means to run a long test on the Go repo.

This is a high level tracking issue for improvements in this area, and for any discussion that may need to happen.

/cc @andybons @cagedmantis @toothrot @golang/osp-team @bradfitz @rsc

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2020

Change https://golang.org/cl/233898 mentions this issue: cmd/dist: use GO_TEST_SHORT value more consistently

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2020

Change https://golang.org/cl/233901 mentions this issue: dashboard: use more consistent definition of longtest builder

gopherbot pushed a commit to golang/build that referenced this issue May 18, 2020
Previously, it was possible to define a builder whose IsLongTest method
would report positively, such that it'd test golang.org/x repos in long
test mode, but the main Go repository in short test mode. This would be
the case for any builder with "-longtest" suffix if it did not manually
include GO_TEST_SHORT=0 in its environment configuration.

It's unlikely we would want to do that intentionally, so this refactor
makes such misconfiguration less likely by inserting the GO_TEST_SHORT
environment variable assignment into the output from Env automatically.

Now, a longtest builder is defined in one consistent way: it must have
a "-longtest" suffix, so that the IsLongTest method reports positively.

For golang/go#39054.
For golang/go#29252.
For golang/go#12508.

Change-Id: Ic24df7b3b7de7db728bba6dc6ad4dd38a2e61e82
Reviewed-on: https://go-review.googlesource.com/c/build/+/233901
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234531 mentions this issue: cmd/releasebot, cmd/release: include long tests in release process

gopherbot pushed a commit to golang/build that referenced this issue Jun 2, 2020
The goal of this change is to reduce the chance of issuing a release
with an unintended regression that would be caught by running long
tests. This change adds long tests that are run during the -prepare
step, in addition to all the existing short tests that are run.

Executing the long tests is implemented by adding two new test-only
release targets. For a release to be considered complete, all release
targets must complete.

These test-only targets are built only for the purpose of verifying
their tests succeed, or to block the release otherwise.
They do not produce release artifacts.

The new test-only targets are named after the builder which is used
to perform their tests, and they are:

• linux-amd64-longtest
• windows-amd64-longtest

More builders may be added in the future, but care must be taken
to ensure the test execution environment is as close as possible
to that of build.golang.org post-submit builders, in order to
avoid false positives and false negatives.

As part of a gradual rollout, this change also adds a flag to skip
longtest builders. It's meant to be used in case a long test proves
to be flaky, and enough confidence can be gained through testing
elsewhere that the failure is not a regression caused by a change
merged to the release branch.

For now, its default value includes both longtest builders, so they
are currently opt-in and this CL is a no-op. After testing proves
that it is viable to rely on this (and any issues preventing that
from being possible are resolved), the default value of the flag
will be changed to the empty string.

For golang/go#29252.
For golang/go#39054.
For golang/go#37827.
Fixes golang/go#24614.

Change-Id: I33ea6601aade2873f857c63f00a0c11821f35a95
Reviewed-on: https://go-review.googlesource.com/c/build/+/234531
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
gopherbot pushed a commit that referenced this issue Aug 18, 2020
There were two places where the -short flag was added in order to
speed up tests when run in short mode, in CL 178399 and CL 177417.

It appears viable to re-use the GO_TEST_SHORT value so that -short
flag is not used when the tests are executed on a longtest builder,
where it is not a goal to skip slow tests for improved performance.

Do so, in order to make the testing configurations simpler and more
predictable.

Factor out the flag name out of the string returned by short, so that
it can be used in context of 'go test' which can accept a -short flag,
and a test binary which requires the use of a longer -test.short flag.

For #39054.
For #29252.

Change-Id: I52dfbef73cc8307735c52e2ebaa609305fb05933
Reviewed-on: https://go-review.googlesource.com/c/go/+/233898
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants