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.

Sign up for GitHub

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/go: short tests are not short enough #26472

Closed
rsc opened this issue Jul 19, 2018 · 9 comments
Closed

cmd/go: short tests are not short enough #26472

rsc opened this issue Jul 19, 2018 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@rsc
Copy link
Contributor

rsc commented Jul 19, 2018

Running all.bash (which does go test -short std cmd), my target is usually that individual tests should run in under 1 second.

The cmd/go tests will never reach this target - cmd/go does more than perhaps the entire standard library combined, so it's fine to take more than 1 second. But my usual target is to keep it below 30s, if not 20s. Right now it's at 76s. We should keep bringing that down.

Converting more tests to scripts may help.
@rsc rsc mentioned this issue Jul 19, 2018
Open
@bcmills bcmills added this to the Go1.12 milestone Jul 19, 2018
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 19, 2018
@iamoryanmoshe
Copy link
Contributor

iamoryanmoshe commented Jul 22, 2018 •
edited
Loading

Would love to give it a shot, can you please explain what

Converting more tests to scripts

Mean?

@bcmills
Copy link
Contributor

bcmills commented Jul 23, 2018

See src/cmd/go/testdata/script.

(The .txt files are pretty self-explanatory, but the README contains more detail about the format and supported commands.)

@shivakumargn
Copy link
Contributor

There are several cmd/go tests in go_test.go. Will submit CL for three of the most time consuming tests listed below. Additionally, will make them non-short.

TestCpuprofileTwice (8.55s)
TestCoverpkgAllRuntime (27.87s)
TestAtomicCoverpkgAll (28.72s)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/126636 mentions this issue: cmd/go: 3 cmd/go tests (>8s) made as non-short scripts

@iwdgo
Copy link
Contributor

iwdgo commented Aug 2, 2018 •
edited
Loading

cmd/go times out on my Windows 10 but I noticed that -vet off could be set. During distribution, running vet has limited value and it is tested separately anyway. The fix seems elementary.
Added:
Recently added GOFLAGS allows to set vet off everywhere if needed: GOFLAGS=-vet=off
No fix needed.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/127542 mentions this issue: cmd/dist: removing vet during testing of build

@iwdgo iwdgo mentioned this issue Aug 3, 2018
Closed
gopherbot pushed a commit that referenced this issue Aug 7, 2018
@shivakumargn
cmd/go: 3 cmd/go tests (>8s) made as non-short scripts
15c106d 
* TestAtomicCoverpkgAll -> Script/cover_atomic_pkgall.txt and make it
* non-short
* TestCoverpkgAllRuntime -> Script/cover_pkgall_runtime.txt and make it
* non-short
* TestCpuprofileTwice -> Script/cpu_profile_twice.txt and make it
* non-short
* TestGoTestMainTwice -> make it non-short

Updates #26472

Change-Id: I24f3d4c2a8b6e317adb369a1b1426e693f9571ed
Reviewed-on: https://go-review.googlesource.com/126636
Reviewed-by: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@bcmills bcmills mentioned this issue Nov 15, 2018
Closed
@bcmills
Copy link
Contributor

bcmills commented Jan 18, 2019

We're currently at 37s for go test -short cmd/go on my workstation. Getting better, but still not quite at the goal.

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/267883 mentions this issue: cmd/go: prefer 'go get -d' instead of 'go get' in script tests

gopherbot pushed a commit that referenced this issue Nov 9, 2020
cmd/go: prefer 'go get -d' instead of 'go get' in script tests
a07e436 
'get -d' has somewhat narrower semantics and is generally faster.
We're deprecating the non-'-d' mode in CL 266360.

For #26472

Change-Id: Id4a324771f77b83e5f47043fd50b74e1c062390b
Reviewed-on: https://go-review.googlesource.com/c/go/+/267883
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2020

As of CL 267883, we're back down to ~25s end-to-end wall time for the test on my workstation.

~/go-review/src$ time go test -short cmd/go
ok      cmd/go  19.888s

real    0m25.565s
user    1m15.674s
sys     0m25.297s

I'm going to close this issue as “resolved enough”, but we can of course keep improving the tests afterward.

@bcmills bcmills closed this as completed Nov 9, 2020
@golang golang locked and limited conversation to collaborators Nov 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @shivakumargn @andybons @bcmills @gopherbot @iwdgo @iamoryanmoshe