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

cmd/dist: run cmd tests in race mode #37940

Closed
bcmills opened this issue Mar 19, 2020 · 2 comments
Closed

cmd/dist: run cmd tests in race mode #37940

bcmills opened this issue Mar 19, 2020 · 2 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 19, 2020

When race.bash was originally added (back in CL 7179052), the Go compiler was still written in C and thus was not meaningful to test under the race detector.
(I'm not sure why the rest of cmd was not tested under the race detector at that point. Perhaps @davecheney remembers?)

When race.bash was gutted and moved into cmd/dist, that behavior was retained:

go/src/cmd/dist/test.go

Lines 1529 to 1532 in b3b174f

func (t *tester) shouldTestCmd() bool {
if t.race {
return false
}

These days, cmd includes an extensive amount of Go code, much of which includes non-trivial concurrency, and some of which exercises packages that do not themselves have race-detector tests (#32783).

I think we should be testing all of cmd under the race detector, not just std. It will increase builder latency and potentially cost more resources, but the additional benefit of testing the concurrent code in cmd seems well worth the cost.

CC @golang/osp-team @josharian @dvyukov

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 19, 2020

Change https://golang.org/cl/224038 mentions this issue: cmd/dist: do not skip 'cmd' tests in race mode

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2020

Change https://golang.org/cl/225577 mentions this issue: cmd/go: do not append to the global cfg.OrigEnv slice

gopherbot pushed a commit that referenced this issue Mar 26, 2020
Appending to a global slice is only safe if its length is already
equal to its capacity. That property is not guaranteed for slices in
general, and empirically does not hold for this one.

This is a minimal fix to make it easier to backport.
A more robust cleanup of the base.EnvForDir function will be sent in a
subsequent CL.

Fixes #38077
Updates #37940

Change-Id: I731d5bbd0e516642c2cf43e713eeea15402604e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/225577
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot gopherbot closed this in 8cb865c Mar 26, 2020
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
You can’t perform that action at this time.