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: use `go tool dist test -k` on fast builders? #14305

Open
minux opened this issue Feb 12, 2016 · 6 comments
Open

x/build: use `go tool dist test -k` on fast builders? #14305

minux opened this issue Feb 12, 2016 · 6 comments

Comments

@minux
Copy link
Member

@minux minux commented Feb 12, 2016

Especially for fast builders, I suggest that we make then use the keepgoing
mode of go tool dist test so that even if a test fails, all the remaining tests
are still tested.

This will be especially useful if a flaky tests fails and then masks the real
problem of a given a CL because the affected tests are not run.

As an example, https://golang.org/cl/19455 changes the runtime, but
the openbsd builder fails a unrelated net tests, and the runtime tests are
not run so we don't actually know if the CL catches any problems on
OpenBSD/386.
http://build.golang.org/log/38e92c1c987f7a7eeb4b6bb418c0d95ed706ba80

I don't know if it's worthwhile to also use the -k for the trybots. I can see
arguments from both sides.

/cc @bradfitz @adg

@minux minux changed the title x/build: use `go tool dist test -k` on fast builders? Proposal: x/build: use `go tool dist test -k` on fast builders? Feb 12, 2016
@minux minux added the Proposal label Feb 12, 2016
@minux minux added this to the Proposal milestone Feb 13, 2016
@adg
Copy link
Contributor

@adg adg commented Feb 15, 2016

I'm in favor of -k for fast builders. Seems like a good way to get a sense of overall build health.

I don't think trybots should use -k. There should be some deterrent to having flaky builds, and having the trybots fail seems like a good impetus for fixing those flakes.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 15, 2016

The builders also don't use "go tool dist test" by itself anyway. The builder coordinator has its own logic for test sharding and when to stop.

I'm not sure how useful this would be in practice. Usually the first failure is enough and the rest would be noise.

I also want to read errors quickly from the builders and not waste more time finding more errors (especially if they'll be useless).

But as long as it can be done without slowing things done, I'm not opposed to this.

I don't have the time to work on this at the moment, though.

@bradfitz bradfitz changed the title Proposal: x/build: use `go tool dist test -k` on fast builders? x/build: use `go tool dist test -k` on fast builders? Aug 15, 2016
@bradfitz bradfitz modified the milestones: Unreleased, Proposal Aug 15, 2016
@gopherbot gopherbot added the Builders label Mar 21, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2019

A bit more discussion is in #36089, like whether this should only be on release branches or everywhere.

But this bug also discusses policy of when we'd do this.

For TryBots I'd want to fail fast. Or I at least want the comment on Gerrit (and email) as fast as possible, even if it keeps going after the failure comment (which implies parsing the output as it's still running).

So for simplicity I'd say just use -k (or equivalent) on post-submit builds and make trybots fail fast.

/cc @dmitshur @bcmills

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 17, 2019

Change https://golang.org/cl/211678 mentions this issue: cmd/coordinator: use 'go tool dist test -k' on release branches

gopherbot pushed a commit to golang/build that referenced this issue Dec 19, 2019
Right now, on release branches, testing is stopped very soon after
encountering the first package with a failing test. For example:

	[...]
	FAIL	cmd/go/internal/modfetch	17.181s
	ok  	cmd/go/internal/modfetch/codehost	5.474s
	FAIL
	2019/11/07 02:35:34 Failed: exit status 1

Ideally we should not have failing tests on release branches for long,
but that is not the current state. Until we reach that state, make
coordinator run 'go tool dist test' with -k flag so that it keeps
testing all remaining packages even if one fails, and report complete
test results in the log.

That way, a package that fails early doesn't make it completely
impossible to see if other package tests pass or fail.

Updates golang/go#14305
Updates golang/go#36181

Change-Id: Ia674005ae45c6b9dbffa6f5b56d60b7ed38f6b34
Reviewed-on: https://go-review.googlesource.com/c/build/+/211678
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Dec 20, 2019

Based on the latest commit results from https://build.golang.org/?branch=release-branch.go1.13, the change in CL 211678 was not sufficient to have an effect on release branches of the main Go repository. It still stopped short. E.g., here's windows-amd64-longtest results:

https://build.golang.org/log/060b17127f4e1e7bc35e12e00058274c77a06d9c

Something more needs to be done for it to have an effect.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 6, 2020

dist test -k doesn't really do anything from the coordinator.

The coordinator doesn't use all.bash/run.bash so it doesn't do just one go tool dist test execution. It instead does go tool dist list to find the list of dist test names, and then it executes them each itself (usually shared out over multiple buildlets, to speed up test execution).

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
5 participants
You can’t perform that action at this time.