-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support .only for all files #556
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
Support .only for all files #556
Conversation
By analyzing the blame information on this pull request, we identified @novemberborn, @ariporad and @sotojuan to be potential reviewers |
111dab7
to
8377081
Compare
@sindresorhus @vdemedes @jamestalmage Added unit test. Should be ready to go. |
Is there a test for multiple exclusive tests in multiple files? |
The test uses three files: One file that has only an exclusive test, one file that has an exclusive test and a non-exclusive test, one file that has two non-exclusive test. Total of five tests across three files with two exclusive tests in different files. |
I'm wondering how this should work with Initially all test files are run and because Subsequently Later a non-test file is changed and all test files are rerun. Only the This isn't necessarily bad but it may be surprising to some. I wonder if This is a little tricky to implement though #544 requires similar per-test-file housekeeping. Anyway not meant as a blocker to this PR landing. We can discuss in a separate issue if people think |
@@ -108,6 +109,15 @@ Api.prototype._handleExceptions = function (data) { | |||
}; | |||
|
|||
Api.prototype._handleStats = function (stats) { | |||
if (this.hasExclusive && !stats.hasExclusive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment as to why the function is returning here? It's because if we're running exclusive tests, but the test file does not include exclusive tests, then we don't want those tests to be counted, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, although I'm not super keen on just dropping this info on the floor. I just went for consistency with how non-exclusive tests are dropped within test-collection
. I'll add a comment to that effect.
That said, I would favor changing the reporting completely as suggested by #471.
Solution makes sense to me. Great job given the amount of (necessary) indirection involved in coordinating test runs 👍 |
I'm not actually sure how it works with |
Watch itself isn't documented yet so don't worry about it. We can get this in and then discuss repercussions in a new issue. |
f3ea64f
to
d3d1652
Compare
@novemberborn I've incorporated those tweaks. If everything looks good, merging this would make it easier to work on #471. Thanks! |
LGTM. @vdemedes @jamestalmage ? |
LGTM from me! @naptowncode could you please resolve the merge conflict? Big thanks for your work, this is an important change! |
|
||
Runner.prototype.run = function (options) { | ||
options = options || {}; | ||
this._reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You no longer need the _reset()
method now that run
is always called.
d3d1652
to
6b1617a
Compare
@novemberborn @vdemedes @sindresorhus Updated and rebased. Also added a unit test that should have been there earlier for the |
@naptowncode Tests are failing on Travis it seems: https://travis-ci.org/sindresorhus/ava/builds/112949149 |
@sindresorhus that's just one test in Node 0.10 though. |
@naptowncode LGTM! |
Thank you so much, @naptowncode! |
Support .only for multiple files
Please see #593 for how |
My suggested solution for #549. Needs unit tests, of course.