-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(runner): fix '.only()' exclusive feature, #1481 #1591
Conversation
@a8m Thanks! Hopefully someone will check this out soon--I'm going to be swamped for next few wks still. I'm thinking might be good for |
Great @boneskull |
@a8m looks great, tyvm! |
Very busy lately, I'll try to dedicate some time to mocha this weekend. Anyways, thanks for the amazing work you've been pushing, @a8m! :) |
looks pretty good, i've looked it over quickly a couple of times and got the feeling some parts could be simpler/clearer. i should get sometime tomorrow to look more in-depth |
Great @travisjeffery |
Looks pretty good! Works with almost all my tests, though I think I found a couple edge cases. The first is with the root suite I think: describe.only('suite', function() {
it.only('test', function() {});
it('test2', function() {});
});
it('should not be ran', function() {});
it.only('another test', function() {});
On master, however, I get the following:
I think it was decided in the issue that spec-related
|
The second issue I hit was with the following test: describe.only('suite', function() {
it.only('test', function() {});
it('test2', function() {});
describe('suite2', function() {
it('test3', function() {});
});
});
|
haha, I forgot to test the root suite, it should be fix here. I'll do it. |
Fixed! |
Those 2 look good, awesome! Got another example, though I don't know if this is expected behavior or not. So this is probably not a bug, but just wanted to clarify :) describe.only('suite', function() {
describe('inner', function() {
it.only('test', function() {});
it('should not run', function() {});
});
it('should not run either', function() {});
});
Is that expected output? If so, then the rest looks good to me! |
This is the expected behaviour, PTAL bdd-test |
rebased! |
function filterOnly(suite) { | ||
// If it has `only` tests | ||
if(suite.hasOnly) suite.tests = suite.hasOnly; | ||
// If it doesn't has a suites make it running |
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.
These comments have a few typos.
@a8m, in general, you are writing |
f07b352
to
0e61241
Compare
* Filter suites based on `isOnly` logic. | ||
* | ||
* @param {Array} suite | ||
* @returns {*} |
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.
Maybe we can set this now to Boolean
?
50f86ea
to
d915328
Compare
LGTM 👍. @travisjeffery, were you going to suggest any code improvements? |
ping @mochajs/mocha |
Well, goes. |
We'll have to wait for the v3.0.0 release tho. |
2f458ab
to
2952eca
Compare
@a8m I haven't gotten a chance to look at this yet, but please let me before this gets merged. cc @mochajs/mocha |
var test = context.it(title, fn); | ||
var reString = '^' + escapeRe(test.fullTitle()) + '$'; | ||
mocha.grep(new RegExp(reString)); | ||
var test = context.it(title, fn) |
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.
various places in this PR will need to be linted
Replaced with #1807(rebased on top of v3.0.0) |
WTF? Are you trying to fix all bugs with one PR? Release some fix already. |
@Vanuan I understand you are probably frustrated with the |
This PR fix #1481, and also extends the
.only()
behaviour.e.g:
grep
anymore.only()
(on many files)There's a tests for bdd, tdd and qunit interfaces that illustrate the point more clearly. so please take a look on it, and let me now what do you think.