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

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

Comments

Projects
None yet
6 participants
@rsc
Contributor

rsc commented Nov 28, 2016

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.

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 28, 2016

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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 28, 2016

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

This comment has been minimized.

Contributor

rsc commented Nov 29, 2016

Edited the original report to add more context.

@calmh

This comment has been minimized.

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...)

@mvdan

This comment has been minimized.

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.

@nathany

This comment has been minimized.

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 modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017

mattbostock added a commit to mattbostock/timbala that referenced this issue Sep 4, 2017

Add go vet to test Make target
`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 added a commit to mattbostock/timbala that referenced this issue Sep 4, 2017

Add go vet to test Make target
`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

This comment has been minimized.

gopherbot commented Oct 30, 2017

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

@rsc

This comment has been minimized.

Contributor

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.

@gopherbot gopherbot closed this in 0d18875 Nov 3, 2017

fujimoto-s added a commit to fujimoto-s/gobgp that referenced this issue Nov 6, 2017

*: Fix problems pointed by 'go vet'
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>

fujita added a commit to osrg/gobgp that referenced this issue Nov 7, 2017

*: Fix problems pointed by 'go vet'
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>
@gopherbot

This comment has been minimized.

gopherbot commented Nov 22, 2017

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.