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

Print failures for pending/only forbidden #2874

Merged
merged 9 commits into from
Sep 29, 2017
Merged

Print failures for pending/only forbidden #2874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e2bb820
Test this.skip
ScottFreeCode Jun 11, 2017
dd34b2f
Test that something is printed
ScottFreeCode Jun 11, 2017
262a17c
Add printout of failures
ScottFreeCode Jun 11, 2017
89c7107
Simplify redundant lines
ScottFreeCode Jun 11, 2017
e0b785f
Fix linting errors
ScottFreeCode Jun 11, 2017
a0549d1
Test beforeEach skip
ScottFreeCode Jun 13, 2017
2307511
Test before as well
ScottFreeCode Jun 13, 2017
245c9cc
Also test suite skipped
ScottFreeCode Jun 17, 2017
6cdfa05
Add test for suite marked only
ScottFreeCode Aug 2, 2017
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
34 changes: 25 additions & 9 deletions lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,10 @@ Runner.prototype.runTest = function (fn) {
if (!test) {
return;
}
if (this.forbidOnly && this.hasOnly) {
fn(new Error('`.only` forbidden'));
return;
}
if (this.asyncOnly) {
test.asyncOnly = true;
}
Expand Down Expand Up @@ -529,7 +533,13 @@ Runner.prototype.runTests = function (suite, fn) {
}

if (test.isPending()) {
self.emit('pending', test);
if (self.forbidPending) {
test.isPending = alwaysFalse;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely happy with this hack to circumvent Runner.prototype.fail's check that ignores pending tests based on test.isPending(), but I'm not sure if there's a better option. Any opinions would be appreciated, provided we can avoid stalling over the matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Actually, I honestly wonder why Runner.prototype.fail needs to ignore pending tests. Wouldn't it be more correct not to call it on pending tests? Do we know where it's being called on pending tests so we could evaluate whether that could be adjusted instead?)

Copy link
Contributor

Choose a reason for hiding this comment

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

A pending test is a failure, because a pending test (a synchronous one, anyway) actually throws an exception. We catch that in the Runnable, then set the pending flag, then continue on to the "fail" handler in Runner. At least, if memory serves...

Copy link
Contributor

Choose a reason for hiding this comment

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

And Runner.prototype.fail is what reporters depend on for events...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. The Pending exception is being handled in the Runner around line 533, in which a pending event is emitted instead of going to the fail function at all. (The fail function ignores pending tests altogether, rather than emitting anything pending for them; hence the need to circumvent it in this line.)

The Runnable is only throwing (and in one case passing to done) Pending exceptions, not catching any (it also only emits an "error" event, in the case that done is called multiple times, so it's not handling the pending emission before sending an ignored error of some sort to the Runner either).

Copy link
Contributor Author

@ScottFreeCode ScottFreeCode Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Additionally, Coveralls indicates that the fail function's pending test ignore branch is never hit: https://coveralls.io/builds/11947829/source?filename=lib%2Frunner.js#L224 However, to confirm that we aren't just missing a test for this case, I'd have to go look at whether we have tests for all... five? six?... cases where tests can be pending: describe.skip, it.skip, it("title and no callback"), this.skip in it, this.skip in beforeEach, this.skip in before...

self.fail(test, new Error('Pending test forbidden'));
delete test.isPending;
} else {
self.emit('pending', test);
}
self.emit('test end', test);
return next();
}
Expand All @@ -538,7 +548,13 @@ Runner.prototype.runTests = function (suite, fn) {
self.emit('test', self.test = test);
self.hookDown('beforeEach', function (err, errSuite) {
if (test.isPending()) {
self.emit('pending', test);
if (self.forbidPending) {
test.isPending = alwaysFalse;
self.fail(test, new Error('Pending test forbidden'));
delete test.isPending;
} else {
self.emit('pending', test);
}
self.emit('test end', test);
return next();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two if (test.isPending()) { ... sections are identical or nearly so; we should consider breaking it out into a function or something.

Expand All @@ -550,7 +566,9 @@ Runner.prototype.runTests = function (suite, fn) {
test = self.test;
if (err) {
var retry = test.currentRetry();
if (err instanceof Pending) {
if (err instanceof Pending && self.forbidPending) {
self.fail(test, new Error('Pending test forbidden'));
} else if (err instanceof Pending) {
test.pending = true;
self.emit('pending', test);
} else if (retry < test.retries()) {
Expand Down Expand Up @@ -586,6 +604,10 @@ Runner.prototype.runTests = function (suite, fn) {
next();
};

function alwaysFalse () {
return false;
}

/**
* Run the given `suite` and invoke the callback `fn()` when complete.
*
Expand Down Expand Up @@ -820,12 +842,6 @@ Runner.prototype.run = function (fn) {

// callback
this.on('end', function () {
if (self.forbidOnly && self.hasOnly) {
self.failures += self.stats.tests;
}
if (self.forbidPending) {
self.failures += self.stats.pending;
}
debug('end');
process.removeListener('uncaughtException', uncaught);
fn(self.failures);
Expand Down
5 changes: 5 additions & 0 deletions test/integration/fixtures/options/forbid-only/only-suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

describe.only('forbid only - suite marked with only', function () {
it('test1', function () {});
});
6 changes: 6 additions & 0 deletions test/integration/fixtures/options/forbid-pending/before-this.skip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

describe('forbid pending - before calls `skip()`', function () {
it('test', function () {});
before(function () { this.skip(); });
});
6 changes: 6 additions & 0 deletions test/integration/fixtures/options/forbid-pending/beforeEach-this.skip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

describe('forbid pending - beforeEach calls `skip()`', function () {
it('test', function () {});
beforeEach(function () { this.skip(); });
});
5 changes: 5 additions & 0 deletions test/integration/fixtures/options/forbid-pending/skip-suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

describe.skip('forbid pending - suite marked with skip', function () {
it('test1', function () {});
});
7 changes: 7 additions & 0 deletions test/integration/fixtures/options/forbid-pending/this.skip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

describe('forbid pending - test calls `skip()`', function () {
it('test1', function () {});
it('test2', function () { this.skip(); });
it('test3', function () {});
});
52 changes: 52 additions & 0 deletions test/integration/options.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ describe('options', function () {
});

describe('--forbid-only', function () {
var onlyErrorMessage = '`.only` forbidden';

before(function () {
args = ['--forbid-only'];
});
Expand All @@ -198,12 +200,24 @@ describe('options', function () {
run('options/forbid-only/only.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, onlyErrorMessage);
done();
});
});

it('fails if there are tests in suites marked only', function (done) {
run('options/forbid-only/only-suite.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, onlyErrorMessage);
done();
});
});
});

describe('--forbid-pending', function () {
var pendingErrorMessage = 'Pending test forbidden';

before(function () {
args = ['--forbid-pending'];
});
Expand All @@ -220,6 +234,7 @@ describe('options', function () {
run('options/forbid-pending/skip.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});
Expand All @@ -228,6 +243,43 @@ describe('options', function () {
run('options/forbid-pending/pending.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});

it('fails if tests call `skip()`', function (done) {
run('options/forbid-pending/this.skip.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});

it('fails if beforeEach calls `skip()`', function (done) {
run('options/forbid-pending/beforeEach-this.skip.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});

it('fails if before calls `skip()`', function (done) {
run('options/forbid-pending/before-this.skip.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});

it('fails if there are tests in suites marked skip', function (done) {
run('options/forbid-pending/skip-suite.js', args, function (err, res) {
assert(!err);
assert.equal(res.code, 1);
assert.equal(res.failures[0].err.message, pendingErrorMessage);
done();
});
});
Expand Down