Skip to content

cmd/go: add -tests to list of vet checks run by "go test" #44251

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
Closed
kybin opened this issue Feb 13, 2021 · 16 comments
Closed

cmd/go: add -tests to list of vet checks run by "go test" #44251

kybin opened this issue Feb 13, 2021 · 16 comments
Assignees
@adonovan
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) GoCommand cmd/go Proposal Proposal-Accepted
Milestone
Go1.24

Comments

@kybin
Copy link
Contributor

kybin commented Feb 13, 2021

What version of Go are you using (go version)?

go 1.15.8

What did you do?

I resturctured my test TestXXX as an example ExampleXXX. But forgot to remove the argument.

What did you expect to see?

ExampleXXX(t *testing.T) is wrong signature for an example (has *testing.T as an argument),
so it should raise error on go test.

What did you see instead?

It didn't run. Made me assumes the test has successfully finished.
@seankhliao seankhliao changed the title ExampleXXX(t *testing.T) test sliently discarded, instead of raising error testing: ExampleXXX(t *testing.T) test sliently discarded, instead of raising error Feb 13, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 13, 2021
@D1CED
Copy link

D1CED commented Feb 13, 2021

Is at least correctly reported on the playground https://play.golang.org/p/dj_FUdOxN1j

@ianlancetaylor
Copy link
Member

The problem is reported by go vet.

@kybin
Copy link
Contributor Author

kybin commented Feb 14, 2021

@D1CED Ah, I thought it isn't relevant to playground, my guess was wrong.

@ianlancetaylor I keep forget about go vet, thanks. Doesn't go test do basic vetting? I feel that I read it somewhere...

@ianlancetaylor
Copy link
Member

Yes, go test runs go vet by default. But it only runs it with a limited set of vet tests, ones that are considered to be very reliable when it comes to showing problems. Perhaps we should add this check to that list.

I'll change this issue into a proposal to do that.

@ianlancetaylor ianlancetaylor changed the title testing: ExampleXXX(t *testing.T) test sliently discarded, instead of raising error proposal: cmd/go: add -tests to list of vet checks run by "go test" Feb 14, 2021
@gopherbot gopherbot added this to the Proposal milestone Feb 14, 2021
@ianlancetaylor ianlancetaylor removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2021
@kybin
Copy link
Contributor Author

kybin commented Feb 15, 2021

Great, thank you.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Mar 10, 2021
@rsc rsc moved this to Incoming in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@bcmills bcmills added the GoCommand cmd/go label Sep 7, 2023
@adonovan
Copy link
Member

adonovan commented Sep 20, 2023 •
edited
Loading

Perusing the source of the tests analyzer, it appears to be in effect a type checker, and 100% precise, so I have no objection to adding this to the 'go test' suite. I'll send a CL.

@adonovan adonovan self-assigned this Sep 20, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/529816 mentions this issue: cmd/go/internal/test: add 'tests' vet check to 'go test' suite

@rsc
Copy link
Contributor

rsc commented Nov 29, 2023

@adonovan to run ecosystem metrics pipeline analysis to make sure there aren't false positives in the broader Go ecosystem.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2023
@adonovan
Copy link
Member

adonovan commented Dec 6, 2023 •
edited
Loading

This text file shows diagnostics found by running the tests analyzer across a sample of 1056 modules in the Go module proxy's corpus. Of those modues, 38 had diagnostics, a total of 242. Of those diagnostics, 96% (233) are of the form "ExampleXYZ refers to unknown identifier: XYZ". I inspected a couple dozen diagnostics and they all looked sound.

The first 10 "Example" diagnostics :

$ grep Example tests.txt | head

https://go-mod-viewer.appspot.com/github.com/olekukonko/tablewriter@v0.0.5/table_test.go#L57: ExampleLong refers to unknown identifier: Long
https://go-mod-viewer.appspot.com/github.com/olekukonko/tablewriter@v0.0.5/table_test.go#L84: ExampleCSV refers to unknown identifier: CSV
https://go-mod-viewer.appspot.com/github.com/gonum/matrix@v0.0.0-20181209220409-c518dec07be9/mat64/cholesky_example_test.go#L72: ExampleCholeskySymRankOne refers to unknown identifier: CholeskySymRankOne
https://go-mod-viewer.appspot.com/github.com/robertkrimen/otto@v0.2.1/documentation_test.go#L7: ExampleSynopsis refers to unknown identifier: Synopsis
https://go-mod-viewer.appspot.com/github.com/robertkrimen/otto@v0.2.1/documentation_test.go#L72: ExampleConsole refers to unknown identifier: Console
https://go-mod-viewer.appspot.com/github.com/cosmos/cosmos-sdk@v0.50.1/crypto/hd/hdpath_test.go#L188: ExampleStringifyPathParams refers to unknown identifier: StringifyPathParams
https://go-mod-viewer.appspot.com/github.com/cosmos/cosmos-sdk@v0.50.1/crypto/hd/hdpath_test.go#L198: ExampleSomeBIP32TestVecs refers to unknown identifier: SomeBIP32TestVecs
https://go-mod-viewer.appspot.com/github.com/cosmos/cosmos-sdk@v0.50.1/crypto/ledger/encode_test.go#L27: ExamplePrintRegisteredTypes refers to unknown identifier: PrintRegisteredTypes
https://go-mod-viewer.appspot.com/github.com/biogo/store@v0.0.0-20201120204734-aad293a2328f/interval/int_interval_example_test.go#L41: Example_2 has malformed example suffix: 2
https://go-mod-viewer.appspot.com/github.com/biogo/store@v0.0.0-20201120204734-aad293a2328f/interval/interval_example_test.go#L67: Example_1 has malformed example suffix: 1

The 9 non-Example ones:

$ grep -v Example tests.txt

https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/common_constraints_test.go#L63: TestlistConstraintsWithDuplicates has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/common_constraints_test.go#L88: TestlistConstraintsWithOffInList has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/dispatch_filterdispatcher_test.go#L31: TestfilterDispatcher_Pass has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/dispatch_filterdispatcher_test.go#L51: TestfilterDispatcher_Deny has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/dispatch_splitdispatcher_test.go#L42: TestsplitDispatcher has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/cihub/seelog@v0.0.0-20170130134532-f561c5e57575/writers_formattedwriter_test.go#L31: TestformattedWriter has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/alexflint/go-arg@v1.4.3/example_test.go#L203: output comment block must be the last comment block
https://go-mod-viewer.appspot.com/github.com/justinas/nosurf@v1.1.1/utils_test.go#L8: TestsContains has malformed name: first letter after 'Test' must not be lowercase
https://go-mod-viewer.appspot.com/github.com/justinas/nosurf@v1.1.1/utils_test.go#L22: TestsameOrigin has malformed name: first letter after 'Test' must not be lowercase

@rsc
Copy link
Contributor

rsc commented Dec 6, 2023

Very nice!

Are there any remaining concerns about this proposal?

@kybin
Copy link
Contributor Author

kybin commented Dec 9, 2023

It's great to see my small, old issue has been changed to a proposal and has come this far.

I don't have any complaints. If you asked to me ;)

Thanks! @adonovan @rsc

@rsc
Copy link
Contributor

rsc commented Dec 14, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to run ‘go vet -tests’ automatically during ‘go test’, with the many other vet checks that run then.

@rsc rsc moved this from Active to Likely Accept in Proposals Dec 14, 2023
@rsc rsc moved this from Likely Accept to Accepted in Proposals Dec 21, 2023
@rsc
Copy link
Contributor

rsc commented Dec 21, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to run ‘go vet -tests’ automatically during ‘go test’, with the many other vet checks that run then.

@rsc rsc changed the title proposal: cmd/go: add -tests to list of vet checks run by "go test" cmd/go: add -tests to list of vet checks run by "go test" Dec 21, 2023
@rsc rsc modified the milestones: Proposal, Backlog Dec 21, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 22, 2024
@dmitshur
Copy link
Contributor

Reopening since CL 529816 was rolled back in CL 571695, and a roll forward hasn't been submitted.

@dmitshur dmitshur reopened this May 23, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/603476 mentions this issue: cmd/go/internal/test: add 'tests' vet check to 'go test' suite

@dmitshur dmitshur modified the milestones: Go1.23, Go1.24 Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) GoCommand cmd/go Proposal Proposal-Accepted
Projects
Proposals
Status: Accepted
Milestone
Go1.24
Development

No branches or pull requests
10 participants
@rsc @kybin @timothy-king @dmitshur @ianlancetaylor @bcmills @adonovan @gopherbot @seankhliao @D1CED