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

Jump to bottom

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
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 19, 2020 •
edited

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
@bcmills bcmills added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector labels Mar 19, 2020
@bcmills bcmills added this to the Backlog milestone Mar 19, 2020
@bcmills bcmills mentioned this issue Mar 19, 2020
Closed
@gopherbot
Copy link

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

This was referenced Mar 19, 2020
Closed
Closed
@gopherbot
Copy link

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
cmd/go: do not append to the global cfg.OrigEnv slice
bfb1342 
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>
@golang golang locked and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@bcmills @gopherbot