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: add -tests to list of vet checks run by "go test" #44251

Closed
kybin opened this issue Feb 13, 2021 · 14 comments
Closed

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

kybin opened this issue Feb 13, 2021 · 14 comments
Assignees
@adonovan
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) GoCommand cmd/go Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Backlog

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
Contributor

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
Contributor

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
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) 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
@bcmills bcmills added the GoCommand cmd/go label Sep 7, 2023
@adonovan
Copy link
Member

adonovan commented Sep 20, 2023 •
edited

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

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

@adonovan
Copy link
Member

adonovan commented Dec 6, 2023 •
edited

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
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
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
Proposals (old)
Incoming
Milestone
Backlog
Development

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