cmd/dist: modify all.bash to no longer skip testing packages without tests #60463
Assignees
Labels
early-in-cycle
A change that should be done early in the 3 month dev cycle.
FrozenDueToAge
NeedsFix
The path to resolution is known, but the work has not been done.
Milestone
Comments
dmitshur
added
the
NeedsInvestigation
|
Change https://go.dev/cl/498270 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
May 26, 2023
Found by the vet check that runs with 'go test cmd/go/internal/modget'. For #57001. For #60463. Change-Id: I4be94f7156724459a5c47bb9745cbb5651fb972c Reviewed-on: https://go-review.googlesource.com/c/go/+/498270 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
|
One option, if we want to minimize output noise, is that dist could do some light filtering of the output to remove skipped package lines. It's a bit odd that the output of dist and go test diverge today, though. It seems annoying to have the skipped packages in the dist output, but I'm having trouble justifying why it would make a different decision about output than go test. |
dmitshur
added
the
early-in-cycle
|
Good example of an issue this would have caught: https://go.dev/cl/501220 introduced a malformed |
|
We discussed this earlier and there was agreement the high-confidence vet coverage will almost certainly warrant doing this now, just need to sort out some of the small implementation details here. |
dmitshur
changed the title
|
I just chatted with Russ about this and he thinks we should just go ahead and do this, and not worry about the increased noise or excluding vendored packages (vendored packages all pass go vet right now, and we probably want to know if they stop passing vet). |
|
Sounds good. Sent CL 503095. The timing aspect of this seems reasonable. There are 164 normal and 96 vendored packages without tests at tip now. On my local machine, the first 'go test -short' run on them took 10 seconds, and the second run took 2.5 seconds. The value provided by this should outweigh that cost. Package listA note about vendored packages. They are already tested and vetted at their canonical upstream location (where their test source lives), but the doing it again in all.bash is comparatively inexpensive since their tests are skipped, and may report findings sooner when the behavior of 'go test' or vet itself changes in the main repo. For reference, here's a sample all.bash output with the change applied. Seems not bad. An upside of this is that it lets one see a more complete picture of all packages in the main repo. And doesn't matter when looking at the structured test output in CI (#59990). Sample all.bash output (with the change) |
|
Change https://go.dev/cl/503095 mentions this issue: |
dmitshur
added
NeedsFix
|
Change https://go.dev/cl/503115 mentions this issue: |
6 tasks
|
This issue is currently labeled as early-in-cycle for Go 1.22. |
gopherbot
pushed a commit
that referenced
this issue
Jul 27, 2023
For short all.bash, we can keep the small speedup of 2-10 seconds by skipping 'go test' on packages without tests. This is viable without coverage loss since the Go release process is guaranteed to run long tests for all first class ports. For #60463. Change-Id: Ib5a6bd357d757141bc8f1c1dec148a6565726587 Reviewed-on: https://go-review.googlesource.com/c/go/+/503115 Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
CL 10492 in 2015 applied a change to have all.bash skip testing Go packages in the standard library (those matched by
go list std cmd) that have no tests. Sadly, the commit message doesn't specify what exactly motivated the change. A guess based on review comments is that it may have been to avoid doing unnecessary work and speed things up, as well as possibly to reduce verbosity of all.bash output.By now, running
go teston a Go package without tests can still produce useful information, such as the high-confidence vet checks that run (e.g., it would've caught an unused printf argument in TryBots), the fact that the package builds successfully, and so on. Furthermore, we have been generally wanting for all.bash not to miss any legitimate problems that a user runninggo test -short std cmdlocally might find.So we can revisit this. Some things worth considering here:
CC @golang/release, @aclements, @bcmills.
The text was updated successfully, but these errors were encountered: