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

cmd/vet or golint: Catch TestMain without os.Exit #9917

Closed
tv42 opened this issue Feb 18, 2015 · 8 comments
Closed

cmd/vet or golint: Catch TestMain without os.Exit #9917

tv42 opened this issue Feb 18, 2015 · 8 comments

Comments

@tv42
Copy link

tv42 commented Feb 18, 2015

Saw someone do this today:

func TestMain(m *testing.M) { m.Run() }

which, naturally, gives false positives for test success.

@mikioh mikioh changed the title tools/cmd/vet or golint: Catch TestMain without os.Exit cmd/vet or golint: Catch TestMain without os.Exit Feb 18, 2015
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/vet or golint: Catch TestMain without os.Exit x/tools/cmd/vet or golint: Catch TestMain without os.Exit Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@dominikh
Copy link
Member

dominikh commented Apr 8, 2016

Note that this can lead to false positives when the os.Exit call is in a helper function, possibly in another package.

@dominikh
Copy link
Member

dominikh commented Apr 8, 2016

Though of course that can be greatly reduced by checking that m.Run is called directly in TestMain and whether its return value is discarded or not.

@chlunde
Copy link
Contributor

chlunde commented Mar 9, 2017

Do we need to check if the method is in TestMain?

I'm creating a CL which only checks that the return value is used. This will detect most common issues (but not fmt.Println(m.Run())), and it can be reused for other methods, by using the syntax (type).Method, for example:

go tool vet -unusedmethods='(database/sql.Tx).Commit,(*testing.M).Run'

I skimmed parts of stdlib but didn't find any obvious other candidates as serious as (*testing.M).Run

* before type names are ignored when matching methods

@gopherbot
Copy link

CL https://golang.org/cl/37968 mentions this issue.

@robpike
Copy link
Contributor

robpike commented Mar 10, 2017

The unusedresults module in vet is not well designed. I'd prefer to see a redesign that generalizes better, rather than adding special features for every function we can think of that needs its results used.

@chlunde
Copy link
Contributor

chlunde commented Mar 10, 2017

@robpike OK, I'm not sure what you're after. Just to play around, I added interface support and moved everything to a single argument, but I do not know types package, so I guess there are issues with the code, besides requiring cleanup.

errors.New,fmt.Errorf,...,sort.Reverse,(testing.M).Run,(error).Error,(fmt.Stringer).String

Are there any other classes of unused results you think it should catch?

@mvdan mvdan changed the title x/tools/cmd/vet or golint: Catch TestMain without os.Exit cmd/vet or golint: Catch TestMain without os.Exit May 31, 2018
@changkun
Copy link
Member

Since #34129 is an accepted proposal, I believe this can be closed.

cc @rsc @robpike @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

Thanks.

@golang golang locked and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants