Correctly skip tests when skipping in a suites before()

[ryan-shaw]

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

[danielstjules]

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]

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

[ryan-shaw]

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.

[mislav]

Implementation of the whole method could be simply:

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

[mislav]

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]

@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]

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]

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

[ryan-shaw]

@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]

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]

My bad! Been busy!

Just rebased and should be good to go.

[puzrin]

@danielstjules @dasilvacontin ok to merge now?

[mislav]

This condition could be written simply as

if (parentPending(test)) {

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

[puzrin]

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

[mislav]

👍

[dasilvacontin]

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)

[mislav]

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.

[puzrin]

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