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

Fixes issues #124 and #166 - Proper display of suites/tests count when using --grep #368

Merged
merged 3 commits into from Apr 10, 2012

Conversation

Projects
None yet
3 participants
@antoviaque
Contributor

antoviaque commented Apr 9, 2012

The first commit is about #124, the second one about #166, the second commit depending on changes from the first one. Additional details are in each commit.

This is part of a Hacker School project, by Brendan Nee and myself. We're going to work on Mocha for the rest of the week, so we appreciate feedback about coding style, issues you'd like to be taken care of, etc.

Thanks!

brendannee and others added some commits Apr 9, 2012

BugFix: Support for correct runner.total count when using --grep
When running --grep the runner.total is updated to the total number of
tests matched by the grep search.

Added an .iterateTests() method for the Suite class that iterates
recursively over all tests within the suite and applies a function.
This is used to get the total number of tests that match grep search.

In order to apply .iterateTests() before the reported is called it was
needed to move the 'new Reporter()' call to after the 'runner.grep()'
call in 'bin/_mocha'.
Fixes #166 - When grepping don't display the empty suites
Don't emit the 'suite' event if the suite doesn't contain tests matching
the grep expression.
suite.iterateTests(fn);
});
return this;
};

This comment has been minimized.

@tj

tj Apr 10, 2012

Contributor

awesome thanks again guys. This one I would be careful with .forEach since it'll need to work on the client-side as well, and I would make this private API (at least for now). Not a huge deal but I would maybe renamed it to .eachTest(

Fixes #124 and fixes #166 - Changes the name of the suite.iterateTests()
function to suite.eachTest(), and replaces forEach by util.forEach()
for browser compatibility.
@antoviaque

This comment has been minimized.

Contributor

antoviaque commented Apr 10, 2012

Thanks for the quick review! We have updated the branch to reflect the changes you were requesting - good catch btw, eachTest() is a better name, and good to know about utils.forEach().

If there are other issues, don't hesitate!

var self = this;
utils.forEach(self.tests, function(test){
fn(test);
});

This comment has been minimized.

@tj

tj Apr 10, 2012

Contributor

I can tweak these if you want but u guys wanted me to be verbose :D anyway, this could be: utils.forEach(this.tests, fn);, and the self var removed

This comment has been minimized.

@antoviaque

antoviaque Apr 10, 2012

Contributor

Oh, thanks - I saw you fixed this already : )

describe('when there are several levels of nested suites', function(){
it('should return the number', function(){
this.suite.addTest(new Test('a child test'));
var suite = (new Suite('a child suite'));

This comment has been minimized.

@tj

tj Apr 10, 2012

Contributor

the parens here are a little wonky, I can remove those tho

tj added a commit that referenced this pull request Apr 10, 2012

Merge pull request #368 from antoviaque/issue124
Fixes issues #124 and #166 - Proper display of suites/tests count when using --grep

@tj tj merged commit 2e56414 into mochajs:master Apr 10, 2012

if(self.grepTotal(suite)) {
this.emit('suite', this.suite = suite);
}

This comment has been minimized.

@tj

tj Apr 10, 2012

Contributor

there was a bug here i didnt notice, you can take a look at cfec012 if you're curious

This comment has been minimized.

@antoviaque

antoviaque Apr 10, 2012

Contributor

I see - we need to skip the test of the method in this case, and just call the callback. Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment