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

proposal: test: indicate when no tests match -run #15211

Closed
cmarcelo opened this issue Apr 9, 2016 · 20 comments
Closed

proposal: test: indicate when no tests match -run #15211

cmarcelo opened this issue Apr 9, 2016 · 20 comments

Comments

@cmarcelo
Copy link
Contributor

cmarcelo commented Apr 9, 2016

When using go test -run to filter tests, there's no clear indication when no tests match. The proposal is to change testing and test package to provide and display this information.

Examples:

$ go test -run DontMatchAnything
testing: warning: no tests match regexp: DontMatchAnything      # 1
PASS
?       _/tmp   [no tests match regexp]                         # 2

$ go test -run DontMatchAnything testing/...
?       testing [no tests match regexp]
?       testing/iotest  [no test files]
?       testing/quick   [no tests match regexp]

$ go test -run Mutually testing/...       # Test matches in testing/quick
?       testing [no tests match regexp]
?       testing/iotest  [no test files]
ok      testing/quick   0.004s

In all cases the exit code would remaing 0, as it would be without the proposal.

The testing package would output the line indicated by (1) and the go test, instead of "ok ..." would output "? ..." for the package, like in the line indicated by (2), in the same style as when there are no test files.

While it's possible to use -v and inspect whether tests were ran or not, it's easy to get confused as the output for not matching anything (with verbose) is identical to output of matching without verbose.

@bradfitz bradfitz added this to the Unplanned milestone Apr 9, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Apr 9, 2016

/cc @ianlancetaylor @robpike @mpvl @adg

@robpike
Copy link
Contributor

robpike commented Apr 9, 2016

Fine with me.

@martisch
Copy link
Contributor

martisch commented Apr 9, 2016

Note that for benchmarks (when -bench is specified too) sometimes it might not desired to run tests again and again. So maybe no warning when -bench matches or when -run=^$ is specified. But i guess if no existing commonly used benchmark analyses tools get confused by the additional output it will be fine too.

@adg
Copy link
Contributor

adg commented Apr 11, 2016

no warning when ... -run=^$ is specified

This sounds good to me.

@cespare
Copy link
Contributor

cespare commented Apr 11, 2016

maybe no warning when -bench matches or when -run=^$ is specified

I like no warning when -bench matches. (I don't usually use ^$; I type something like -run xx which doesn't require quoting.)

@minux
Copy link
Member

minux commented Apr 11, 2016 via email

@adg
Copy link
Contributor

adg commented Apr 11, 2016

Something like this, where -run=xx does not match any tests:

-run= -bench= warning
"" "" ""
^$ / xx "" "no tests match -run regexp xx"
"" . "N tests will run; use -run=^$ to disable" (suppress if N == 0)
^$ / xx . ""

?

@cespare
Copy link
Contributor

cespare commented Apr 11, 2016

Is the "N tests will run" warning helpful?

If the tests are quick I often do intend them to run before the benchmarks (to check that the change I'm benchmarking doesn't introduce a bug).

@adg
Copy link
Contributor

adg commented Apr 11, 2016

@cespare it'd be useful to me, I often forget to disable tests when running benchmarks.

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2016

I'm also inclined not to add the N tests will run warning. Benchmarks are meaningless if you introduce a bug. It is actually not a bad thing to run the tests. I often disable them myself this way and have been bitten by this.

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2016

I'm waiting on a review from @rsc related to this code. I can implement this as soon as that is in.

@adg
Copy link
Contributor

adg commented Apr 11, 2016 via email

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2016

To me the message is too much of a suggestion to the user that running benchmarks without running tests first is a good thing. If the user can bear running the tests it is better to do so.

@adg
Copy link
Contributor

adg commented Apr 11, 2016

I didn't put much thought into the text I suggested ("N tests will run; use -run=^$ to disable"). It could just as easily be "running N tests" which has no suggestive qualities to it at all.

@adg adg closed this as completed Apr 11, 2016
@adg adg reopened this Apr 11, 2016
@rsc
Copy link
Contributor

rsc commented Apr 12, 2016

The initial proposal, to treat no tests matching as equivalent to having no test files, sounds good to me. I don't think there's any need to print the number of tests.

@cmarcelo
Copy link
Contributor Author

Thanks for the feedback.

My plan is to extend the idea to cover together benchmarks, tests and examples. If testing can't find anything to run, it outputs a warning message and changes the summary line to "? package [no matches]".

The illustrated cases in original proposal description remain the same. In cases where there are benchmarks to run, the warning or the ? will not appear.

@mpvl thanks for volunteering but I already wrote a proof-of-concept and intend to make into a proper change. However, could I count on you to review it once done? :-)

@mpvl
Copy link
Contributor

mpvl commented Apr 17, 2016

Sure, I can review it. But would be good for CL 19122 to be in first.

Note that returning the count is also problematic, as it is not possible to know how many subtests will be run in advance.

@cmarcelo
Copy link
Contributor Author

@mpvl could you take a look at https://go-review.googlesource.com/#/c/22341/?

I think it was done in a way that doesn't conflict with subtest matching. I've taken into account the fact that we can't know how many subtests will run in advance.

@gopherbot
Copy link
Contributor

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

@cmarcelo
Copy link
Contributor Author

@mpvl ping :)

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

10 participants