golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.33k stars 17.7k forks source link

cmd/go: building a test binary with "go test -c" probably shouldn't run vet #32134

Open bradfitz opened 5 years ago

bradfitz commented 5 years ago

While working on some slow full emulation qemu VMs, I noticed go test -c was much slower than expected. Looking at top, I saw vet eating CPU.

It seems surprising that we default to running vet on a flag that's described as:

Compile the test binary to pkg.test but do not run it

Any vet output would look like a test failed and thus ran, even if it's not technically a test.

I vote we turn it off for -c. @bcmills, @jayconrod, @ianlancetaylor, @rsc?

In any case, it's not critical. Now I know to always use -vet=off with -c:

gopher@buildlet:~/go/src/io$ time ../../bin/go test -c

real    0m28.740s
user    0m14.184s
sys     0m9.208s

gopher@buildlet:~/go/src/io$ time ../../bin/go test -c -vet=off

real    0m19.385s
user    0m9.772s
sys     0m6.488s

gopher@buildlet:~/go/src/io$ time ../../bin/go test -c 

real    0m28.651s
user    0m16.804s
sys     0m10.260s

gopher@buildlet:~/go/src/io$ time ../../bin/go test -c -vet=off

real    0m18.051s
user    0m10.504s
sys     0m6.692s

Marking for Go 1.14, unless this is a recent regression. I didn't check.

cuonglm commented 5 years ago

The feature was added in https://github.com/golang/go/issues/26451

bcmills commented 5 years ago

The trouble with turning off the vet step during go test -c is that it could bury the vet output entirely: typically a go test -c invocation corresponds to a later manual run of the binary to get the test's output, and I don't think we should be embedding the vet warnings in the actual test binary, so that would mean that the vet diagnostics are never presented to the user.

I understand why this is surprising, but — especially given the -vet=off workaround — I don't think we should disable the vet step implicitly.

mvdan commented 5 years ago

@bcmills given that the standard way to run a package's tests is go test, not go test -c && ./pkg.test, I think vet warnings wouldn't be buried for long anyway.

Perhaps one user or machine could unintentionally ignore vet for a bit, but I imagine other machines or humans would quickly start seeing the vet warnings.

I don't think we should be telling people who really just want go test -c (and the old, non-surprising behavior) to also use -vet=off. I realise that being conservative is generally a good idea, but I don't think we should make go test -c slow by default for the sake of it.