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

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

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

ryan-shaw
Copy link

Tests didn't get skipped when calling this.skip() in the before function of a suite.
e.g.

describe('Suite', function(){
  before(function(){
    this.skip();
  });

  it('this is still ran', function(){
    //code
  });
});

I don't know if this is the best way of fixing the issue, or even if this is the intended behaviour. Suggestions welcome to improve this PR. 💥

Review on Reviewable

@danielstjules
Copy link
Contributor

Thanks for the PR! Could you undo your changes to mocha.js (we only build it before releases to keep minimal changesets) and add an integration test to https://github.com/mochajs/mocha/blob/master/test/integration/pending.js

@danielstjules
Copy link
Contributor

When done, could you also rebase/squash to a single commit? :)

@ryan-shaw
Copy link
Author

Will do, only had time yesterday to revert not make test.

Sent from my iPhone

On 26 Dec 2015, at 17:04, Daniel St. Jules notifications@github.com wrote:

When done, could you also rebase/squash to a single commit? Thanks! :)


Reply to this email directly or view it on GitHub.

}

if (parent.parent) {
return parentPending(parent.parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Implementation of the whole method could be simply:

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

@mislav
Copy link
Contributor

mislav commented Jan 10, 2016

I was bitten by this bug too and am glad that there is this PR to solve it.

This references #946 (/cc @cmbuckley) and fixes #1936.

@mislav
Copy link
Contributor

mislav commented Jan 10, 2016

@ryanshawty When updating your PR, if you have time, can you pull in my related fix from master...mislav:fix-skip? I have confirmed that my change fixes the below simple case, but haven't got time to decipher mocha's integration suite right now and figure out how to write tests for this.

describe('skipping', function() {
  it('amazes', function() {
    this.skip()
    assert(true)
  })
})

// expected: "amazes" test shows as pending within HTML test results
// actual outcome: "TypeError: Cannot read property 'toString' of undefined"

@danielstjules
Copy link
Contributor

Sorry, I realize this fix is for the browser reporters. Please ignore my previous request for an integration test, as this behavior works correctly from node.

@dasilvacontin
Copy link
Contributor

I can pick this up on Thursday if @danielstjules or someone doesn't beat me to it. :)

@ryan-shaw
Copy link
Author

@danielstjules I see! I wasn't sure what you wanted there so haven't got around to fixing updating it yet!

@mislav Yes, I shall try that soon.

@puzrin
Copy link

puzrin commented Jan 22, 2016

Any update on this? I could make new PR if that's acceptable. As far as i understand, this is the only PR blocking new release.

@ryan-shaw
Copy link
Author

My bad! Been busy!

Just rebased and should be good to go.

@ryan-shaw ryan-shaw force-pushed the master branch 2 times, most recently from 7b78fbd to d57ff2f Compare January 22, 2016 15:25
@puzrin
Copy link

puzrin commented Jan 22, 2016

@danielstjules @dasilvacontin ok to merge now?

// 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...

danielstjules added a commit that referenced this pull request Jan 26, 2016
Correctly skip tests when skipping in a suites before()
@danielstjules danielstjules merged commit 8522755 into mochajs:master Jan 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants