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

fix weird "pending" behavior; closes #2286 #2623

Closed
wants to merge 1 commit into from
Closed

fix weird "pending" behavior; closes #2286 #2623

Show file tree
Hide file tree
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
9 changes: 2 additions & 7 deletions lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,12 @@ Runner.prototype.hook = function (name, fn) {
}
if (err) {
if (err instanceof Pending) {
if (name === 'beforeEach' || name === 'afterEach') {
if (name === 'beforeEach') {
self.test.pending = true;
} else {
utils.forEach(suite.tests, function (test) {
test.pending = true;
});
// a pending hook won't be executed twice.
hook.pending = true;
}
} else {
Expand Down Expand Up @@ -541,7 +540,7 @@ Runner.prototype.runTests = function (suite, fn) {
if (test.isPending()) {
self.emit('pending', test);
self.emit('test end', test);
return next();
return self.hookUp('afterEach', next);
}
if (err) {
return hookErr(err, errSuite, false);
Expand All @@ -567,10 +566,6 @@ Runner.prototype.runTests = function (suite, fn) {
}
self.emit('test end', test);

if (err instanceof Pending) {
return next();
}

return self.hookUp('afterEach', next);
}

Expand Down
179 changes: 169 additions & 10 deletions test/integration/regression.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var fs = require('fs');
var path = require('path');
var run = require('./helpers').runMocha;
var runJSON = require('./helpers').runMochaJSON;
var map = require('../../lib/utils').map;

describe('regressions', function () {
it('issue-1327: should run all 3 specs exactly once', function (done) {
Expand Down Expand Up @@ -54,17 +55,175 @@ describe('regressions', function () {
});
});

describe('issue-2286: after doesn\'t execute if test was skipped in beforeEach', function () {
var afterWasRun = false;
describe('suite with skipped test for meta test', function () {
beforeEach(function () { this.skip(); });
after(function () { afterWasRun = true; });
it('should be pending', function () {});
});
after('meta test', function () {
afterWasRun.should.be.ok();
describe(
"issue-2286: after doesn't execute if test was skipped in beforeEach",
function () {
/**
* Generates tests for behavior of `this.skip()`
* @param {string} name - Name of Runnable abort via `this.skip()`
* @param {Array.<boolean>} expected - For each of the Runnable types,
* whether or not the Runnable should have been run. The order is
* "beforeAll" x 2, "beforeEach" x 2, "test" x 2, "afterEach" x 2, then
* "afterAll" x 2. There should be 10 items in this array.
* @param {string} [mode=always] - One of 'always' or 'once'
*/
function testPendingRunnables (name, expected, mode) {
mode = mode || 'always';
var spies = [];

function spy (skip) {
function wrapped () {
if ((!wrapped.runCount++ || mode === 'always') && skip) {
Copy link

Choose a reason for hiding this comment

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

wrapped.runCount++ === 0 would add a bit more clarity and make it very explicit what you're looking for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your thoughts!

I usually just prefer to use ! when a falsy check is adequate. It's a common style, so I'm not too concerned about it.

Though, some prefer explicit addition (+) over increment operator(s). This is more obvious for readers of JavaScript who are, say, unfamiliar with the difference between foo++ and ++foo (or even +foo).

But, if it's opaque, I'll find another way to skin this particular cat.

this.skip();
}
}

wrapped.runCount = 0;
spies.push(wrapped);
return wrapped;
}

describe(name, function () {
describe('meta', function () {
before(spy(name === 'beforeAll'));
before(spy(name === 'beforeAll'));
beforeEach(spy(name === 'beforeEach'));
beforeEach(spy(name === 'beforeEach'));
it('might be pending', spy(name === 'test'));
it('might be pending', spy(name === 'test'));
afterEach(spy(name === 'afterEach'));
afterEach(spy(name === 'afterEach'));
after(spy(name === 'afterAll'));
after(spy(name === 'afterAll'));
});

after(name + ' - ' + mode, function () {
map(spies, function (spy) {
return Boolean(spy.runCount);
})
.should
.deepEqual(expected);
});
});
}

testPendingRunnables('beforeAll', [
true, // beforeAll
Copy link

@fearphage fearphage May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Booleans that map to calls are not intuitive. What if you used strings instead?

Possibly use beforeAll and beforeAll-called-skip (or something similar) to denote the state?

Then you wouldn't need comments to explain what everything means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion.

This entire test is a pretty rotten hack, which I am unhappy with. If anyone can think of a better solution--and/or implement the requisite harness(es)--I'd be grateful!

true,
false, // beforeEach
false,
false, // test
false,
false, // afterEach
false,
true, // afterAll
true
]);

testPendingRunnables('beforeAll', [
true, // beforeAll
true,
false, // beforeEach
false,
false, // test
false,
false, // afterEach
false,
true, // afterAll
true
], 'once');

testPendingRunnables('beforeEach', [
true, // beforeAll
true,
true, // beforeEach
true,
false, // test
false,
true, // afterEach
true,
true, // afterAll
true
]);

testPendingRunnables('beforeEach', [
true, // beforeAll
true,
true, // beforeEach
true,
false, // test
true,
true, // afterEach
true,
true, // afterAll
true
], 'once');

testPendingRunnables('test', [
true, // beforeAll
true,
true, // beforeEach
true,
true, // test
true,
true, // afterEach
true,
true, // afterAll
true
]);

testPendingRunnables('test', [
true, // beforeAll
true,
true, // beforeEach
true,
true, // test
true,
true, // afterEach
true,
true, // afterAll
true
], 'once');

testPendingRunnables('afterEach', [
true, // beforeAll
true,
true, // beforeEach
true,
true, // test
false,
true, // afterEach
true,
true, // afterAll
true
], 'once');

testPendingRunnables('afterAll', [
true, // beforeAll
true,
true, // beforeEach
true,
true, // test
true,
true, // afterEach
true,
true, // afterAll
true
]);

testPendingRunnables('afterAll', [
true, // beforeAll
true,
true, // beforeEach
true,
true, // test
true,
true, // afterEach
true,
true, // afterAll
true
], 'once');
});
});

it('issue-2315: cannot read property currentRetry of undefined', function (done) {
runJSON('regression/issue-2315.fixture.js', [], function (err, res) {
Expand Down