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

Deprecate this.skip() for "after all" hooks #3719

Merged
merged 3 commits into from Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/index.md
Expand Up @@ -626,7 +626,7 @@ Because this test _does nothing_, it will be reported as _passing_.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the following line to the preceding codefence at L616?

// antipattern
it('should only test in the correct environment', function() {

> _Best practice_: Don't do nothing! A test should make an assertion or use `this.skip()`.

To skip _multiple_ tests in this manner, use `this.skip()` in a "before" hook:
To skip _multiple_ tests in this manner, use `this.skip()` in a "before all" hook:

```js
before(function() {
Expand Down Expand Up @@ -662,6 +662,8 @@ describe('outer', function() {
});
```

Skipping a test within an "after all" hook is deprecated and will throw an exception in a future version of Mocha. Use a return statement or other means to abort hook execution.

> Before Mocha v3.0.0, `this.skip()` was not supported in asynchronous tests and hooks.

## Retry Tests
Expand Down Expand Up @@ -910,7 +912,7 @@ Enforce a rule that tests must be written in "async" style, meaning each test pr

### `--bail, -b`

Causes Mocha to stop running tests after the first test failure it encounters. Corresponding `after()` and `afterEach()` hooks are executed for potential cleanup.
Causes Mocha to stop running tests after the first test failure it encounters. Corresponding "after each" and "after all" hooks are executed for potential cleanup.

`--bail` does _not_ imply `--exit`.

Expand Down
10 changes: 8 additions & 2 deletions lib/runner.js
Expand Up @@ -363,9 +363,9 @@ Runner.prototype.hook = function(name, fn) {
}
self.currentRunnable = hook;

if (name === 'beforeAll') {
if (name === HOOK_TYPE_BEFORE_ALL) {
hook.ctx.currentTest = hook.parent.tests[0];
} else if (name === 'afterAll') {
} else if (name === HOOK_TYPE_AFTER_ALL) {
hook.ctx.currentTest = hook.parent.tests[hook.parent.tests.length - 1];
} else {
hook.ctx.currentTest = self.test;
Expand All @@ -388,6 +388,12 @@ Runner.prototype.hook = function(name, fn) {
}
if (err) {
if (err instanceof Pending) {
if (name === HOOK_TYPE_AFTER_ALL) {
utils.deprecate(
'Skipping a test within an "after all" hook is DEPRECATED and will throw an exception in a future version of Mocha. ' +
'Use a return statement or other means to abort hook execution.'
);
}
if (name === HOOK_TYPE_BEFORE_EACH || name === HOOK_TYPE_AFTER_EACH) {
self.test.pending = true;
juergba marked this conversation as resolved.
Show resolved Hide resolved
} else {
juergba marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
18 changes: 18 additions & 0 deletions test/integration/fixtures/pending/skip-sync-after.fixture.js
@@ -0,0 +1,18 @@
'use strict';

describe('skip in after', function () {
it('should run this test-1', function () {});

after('should print DeprecationWarning', function () {
this.skip();
throw new Error('never throws this error');
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
});

describe('inner suite', function () {
it('should run this test-2', function () {});
});
});

describe('second suite', function () {
it('should run this test-3', function () {});
});
22 changes: 22 additions & 0 deletions test/integration/pending.spec.js
Expand Up @@ -71,6 +71,28 @@ describe('pending', function() {
});
});

describe('in after', function() {
it('should run all tests', function(done) {
runMocha(
'pending/skip-sync-after.fixture.js',
args,
function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have passed').and('to satisfy', {
passing: 3,
failing: 0,
pending: 0,
output: expect.it('to contain', '"after all" hook is DEPRECATED')
Copy link
Member

Choose a reason for hiding this comment

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

this can be output: /"after all" hook is DEPRECATED/ per the semantics of "to satisfy"

});
done();
},
'pipe'
);
});
});

describe('in before', function() {
it('should skip all suite specs', function(done) {
run('pending/skip-sync-before.fixture.js', args, function(err, res) {
Expand Down