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

Open
tv42 opened this issue Feb 18, 2015 · 6 comments

Comments

@tv42
Copy link

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 added the repo-tools label Feb 18, 2015

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Mar 9, 2017

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

@robpike

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.