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/go: run vet automatically during 'go test' #18084

Closed
rsc opened this issue Nov 28, 2016 · 9 comments
Closed

cmd/go: run vet automatically during 'go test' #18084

rsc opened this issue Nov 28, 2016 · 9 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@rsc
Copy link
Contributor

rsc commented Nov 28, 2016 •
edited

See #17058 (comment).

We'd like to have a set of vet tests that can be cause for failing 'go test' and then run go vet, possibly in parallel with the test binary, during 'go test'.

Update:

Obviously this is not much description. We wanted to write it down to remind ourselves for Go 1.9. If there are significant issues to discuss, we can move it into a formal proposal.

Rob and I have talked about doing something like this since summer 2014 or maybe earlier. The key insight is that running 'go vet' is considered a best practice, but why is that something people should need to learn and think to do explicitly? If it's such a good practice, it should be integrated into something that happens already. Put another way, we arrange (via editor hooks) to run gofmt automatically on our code. Why doesn't something run 'go vet' automatically too? Running tests is a good time for it: it happens after the code is modified, vet needs most of the same inputs as linking a test binary (all the precompiled objects for dependencies), and for the problems it can diagnose, vet's report should be clearer and more direct than any test failure.

It is an explicit goal to keep 'go test' running as fast as it does now, by overlapping vet with the ordinary test sequence. The goal is that users don't notice vet as part of the 'go test' process at all, until vet speaks up and says something important. Brad points out that instead of overlapping with the test binary execution it might be possible to overlap with the linking of the test binary. That usually takes longer anyway, and then vet failure can keep the test binary from running at all, if we decide that's desirable.
@rsc rsc added this to the Go1.9Early milestone Nov 28, 2016
This was referenced Nov 28, 2016
Closed
Closed
@mvdan
Copy link
Member

mvdan commented Nov 28, 2016

Is this a proposal, or was it proposed and accepted internally?

@bradfitz
Copy link
Contributor

Some bugs are filed as normal bugs, and then become proposals later.

Some bugs are filed as proposals, and we say they're very obviously fine, and they bypass the heavier proposal process.

In this case, I haven't heard any objections. This has come up a few times lately in various forums and people seem to think it's a good idea if it doesn't make go test slower and it's 100% reliable, and there will still be a command line flag to opt out (or opt-in to non-100% checks) anyway.

@rsc
Copy link
Contributor Author

rsc commented Nov 29, 2016

Edited the original report to add more context.

@calmh
Copy link
Contributor

calmh commented Jan 18, 2017

What is it about go test specifically that makes it a good place to hook in vet? I mean, if the vet complaints are 100% reliable and should always be fixed, why not make them compile errors? I understand not wanting to break compilation for large swathes of existing code, but breaking go test on them is probably about as disruptive?

I see the point about there being an opt out, but doesn't that just bring us back to the compiler warning discussion? That is, some package author will opt out, and then I can't run tests in my project because it fails on their package and I need to opt out as well, and so on ...

(I think the more honest approach here would be to actually introduce the checks as compiler warnings, as that is essentially what they are, and lobby for it being socially unacceptable to have warnings on your package...)

@bradfitz bradfitz mentioned this issue Feb 2, 2017
Open
@mvdan mvdan mentioned this issue Apr 26, 2017
Open
@mvdan
Copy link
Member

mvdan commented Apr 26, 2017

@calmh making vet part of compiler errors or warnings would noticeably slow down the compiler, so I'm assuming that's why it was never considered.

Regarding your socially unacceptable argument - gofmt and go test aren't enforced by the compiler, but they are already socially unacceptable. Making vet part of the default go test would also put it in that group.

@jimmyfrasche jimmyfrasche mentioned this issue May 2, 2017
Open
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 3, 2017
@nathany
Copy link
Contributor

nathany commented May 5, 2017

While I like the idea of making vet more common practice, I have a few questions/comments.

Would go tool vet stick around? There are a number of flags for vet. I can't imagine all those being supported through go test.

The documentation says the vet is only a guideline. I've had team members run into false positives, such as assuming Error refers fmt.Error:

possible formatting directive in Error call
require.Error(err, "%+v", stu)

Does the benefit of beginners automatically running vet outweigh the trouble of them figuring out how to opt out?

@bradfitz bradfitz mentioned this issue Jun 8, 2017
Open
@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
mattbostock added a commit to mattbostock/timbala that referenced this issue Sep 4, 2017
@mattbostock
Add go vet to test Make target
a9c9ecc 
`go vet` checks for code correctness:
https://golang.org/cmd/vet/

I believe that `go vet` generates few enough false positives that it's
safe to include as part of the CI tests.

See also:
golang/go#18084
@mattbostock mattbostock mentioned this issue Sep 4, 2017
Merged
mattbostock added a commit to mattbostock/timbala that referenced this issue Sep 4, 2017
@mattbostock
Add go vet to test Make target
15f0e40 
`go vet` checks for code correctness:
https://golang.org/cmd/vet/

I believe that `go vet` generates few enough false positives that it's
safe to include as part of the CI tests.

See also:
golang/go#18084
@gopherbot
Copy link

Change https://golang.org/cl/74356 mentions this issue: cmd/go: run vet automatically during go test

@rsc rsc mentioned this issue Oct 30, 2017
Closed
@rsc
Copy link
Contributor Author

rsc commented Nov 1, 2017

@nathany, the vet flags are all now supported by "go vet". The plan is not to add them to "go test". Instead just use "go vet" if you want extra control over the tests.

fujimoto-s added a commit to fujimoto-s/gobgp that referenced this issue Nov 6, 2017
@fujimoto-s
*: Fix problems pointed by 'go vet'
24211b2 
In the near feature (likely in Go1.10),
'go test' will never work if 'go vet' fails.
(See: golang/go#18084)

This commit is for dealing with such a situation
(and also for improving the quality of our code).

Signed-off-by: Satoshi Fujimoto <satoshi.fujimoto7@gmail.com>
@fujimoto-s fujimoto-s mentioned this issue Nov 6, 2017
Closed
fujita pushed a commit to osrg/gobgp that referenced this issue Nov 7, 2017
@fujimoto-s @fujita
*: Fix problems pointed by 'go vet'
8f0c42d 
In the near feature (likely in Go1.10),
'go test' will never work if 'go vet' fails.
(See: golang/go#18084)

This commit is for dealing with such a situation
(and also for improving the quality of our code).

Signed-off-by: Satoshi Fujimoto <satoshi.fujimoto7@gmail.com>
@odeke-em odeke-em mentioned this issue Nov 7, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/79398 mentions this issue: all: fix test breakage due to vet checks

@golang golang locked and limited conversation to collaborators Nov 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@bradfitz @nathany @rsc @calmh @mvdan @gopherbot