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.

Sign up for GitHub

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

Jump to bottom

Correctly skip tests when skipping in a suites before() #1945

Merged
merged 1 commit into from
Jan 26, 2016
Merged

Correctly skip tests when skipping in a suites before() #1945

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,12 @@ Runner.prototype.runTests = function(suite, fn) {
return;
}

function parentPending(suite) {
return suite.pending || (suite.parent && parentPending(suite.parent));
}

// pending
if (test.pending) {
if (test.pending || parentPending(test.parent)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition could be written simply as

if (parentPending(test)) {

but it's not a big deal. It could stay as-is for clarity.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not obvious. "parentPending" name means we testing parent, not item itself. I'd leave as is - more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not obvious. "parentPending" name means we testing parent, not item itself. I'd leave as is - more readable.

Due to the naming, parentPending feels like it will only check the ancestors, but it will also check the Runnable passed as argument.

What about (test.pending || parentPending(test))? (with the necessary changes in the function)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the function and the condition is perfectly fine as-is right now. The code could always be expressed in different ways, but what's important right now is to get this merged.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Let's merge it and move forward. 3 months passed...

self.emit('pending', test);
self.emit('test end', test);
return next();
Expand Down