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

Report skipped tests upon beforeEach hook failure #4233

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
74 changes: 69 additions & 5 deletions docs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ Mocha is a feature-rich JavaScript test framework running on [Node.js][] and in
- [test coverage reporting](#wallabyjs)
- [string diff support](#diffs)
- [javascript API for running tests](#more-information)
- proper exit status for CI support etc
- [auto-detects and disables coloring for non-ttys](#reporters)
- [async test timeout support](#delayed-root-suite)
- [test retry support](#retry-tests)
Expand All @@ -36,7 +35,6 @@ Mocha is a feature-rich JavaScript test framework running on [Node.js][] and in
- [auto-exit to prevent "hanging" with an active loop](#-exit)
- [easily meta-generate suites](#markdown) & [test-cases](#list)
- [config file support](#-config-path)
- clickable suite titles to filter test execution
- [node debugger support](#-inspect-inspect-brk-inspect)
- [node native ES modules support](#nodejs-native-esm-support)
- [detects multiple calls to `done()`](#detects-multiple-calls-to-done)
Expand Down Expand Up @@ -449,9 +447,73 @@ setTimeout(function() {
}, 5000);
```

### Failing Hooks

Upon a failing "before all", "before each" or "after each" hook, all remaining tests in the current suite and also its nested suites will be skipped. Skipped tests are included in the test results and marked as **skipped**. A skipped test is not considered a failed test.
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so we're differentiating between "skipped" and "pending"... I know these two concepts have been conflated.


```js
describe('outer', function() {
before(function() {
throw new Error('Exception in before hook');
});

it('should skip this outer test', function() {
// will be skipped and listed as 'skipped'
});

after(function() {
// will be executed
});

describe('inner', function() {
before(function() {
// will be skipped
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 it's important to show (somehow) that order matters. In this case, the outer "before all" hook is run before the inner "before all" hook. If, instead, the inner hook threw, what would happen?

});

it('should skip this inner test', function() {
// will be skipped and listed as 'skipped'
});

after(function() {
// will be skipped
});
});
});
```

#### "before each" hook

```js
describe('failing "before each" hook', function() {
beforeEach(function() {
// runs and fails only once
throw new Error('Exception in beforeEach hook');
});

it('should skip this outer test', function() {
// will be skipped and reported as 'skipped' test
});

afterEach(function() {
/* will be executed */
});
after(function() {
/* will be executed */
});

describe('inner suite', function() {
// all hooks, tests and nested suites will be skipped

it('should skip this inner test', function() {
// will be skipped and reported as 'skipped' test
});
});
});
```

## Pending Tests

"Pending"--as in "someone should write these test cases eventually"--test-cases are simply those _without_ a callback:
"Pending" - as in "someone should write these test cases eventually" - test-cases are simply those _without_ a callback:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an "EM DASH"? Ideally we should have something like smartypants that translates -- to . Maybe revert this?


```js
describe('Array', function() {
Expand All @@ -462,7 +524,9 @@ describe('Array', function() {
});
```

Pending tests will be included in the test results, and marked as pending. A pending test is not considered a failed test.
By appending `.skip()`, you may also tell Mocha to ignore a test: [inclusive tests](#inclusive-tests).
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

By appending `.skip()`, you may also tell Mocha to [ignore a test](#inclusive-tests).

Copy link
Contributor

Choose a reason for hiding this comment

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

wait... I'm confused now. this.skip() means "test is pending", not "test is skipped"?

I think we could be more clear here. For your consideration:

  • Maybe we can, instead, make this.skip() report a test as skipped, and this.pending() report a test as pending. The behavior would be roughly similar (the test wouldn't get run), but the test would not be displayed as "pending" unless the user called this.pending() instead of this.skip() (likewise for describe.skip()/describe.pending()/it.skip()/it.pending()). This would be less of a breaking change than just trying to rename skip() to pending().
  • Maybe instead of calling them "skipped tests", as you introduce above, we can just use a different word. "Omitted" tests, maybe?
  • This may be a good place to update the terminology we use elsewhere re: "inclusive" (it.skip()) and "exclusive" (it.only()) tests. Those terms always felt vague to me. Instead of "inclusive", we could say "omitted", and instead of "exclusive", we could say "selected" or "picked" or something.

Copy link
Contributor Author

@juergba juergba May 1, 2020

Choose a reason for hiding this comment

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

The current situation (incl. this PR) is:

  • pending: set by the user
    • test without callback: it('some test')
    • static: it.skip() or describe.skip()
    • conditional: this.skip()
  • skipped: test not run because of a failing hook

I haven't changed anything on the first item pending. This mismatch between our terminology and function is an old problem. We could replace label pending by label skipped, but this would break a few third-party tools, reporters, ...
I certainly don't want to rename/add a new this.pending().
And the pending part is out of this PR's scope.

The second label skipped is new and is about failing hooks. We could rename it to "omitted" or "dropped" or whatever. Eg. Cypress and Mochawesome are using skipped for tests omitted by failing hooks. That's why I have chosen skipped. But yes, it is confusing.

We need two independent labels: pending (or we rename it to skipped) and a new one for hook-failed-omitted tests. So I need to know which name to chose.

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit of brainstorm:

My thoughts are, when I mark a test not to run, I want Mocha to ignore it, to disable it, to skip it, to omit it.

If it's skipped because of hook-failed, Mocha is skipping it, leaving out, or the test was unrun, unused, benched, called off, unprepared, dropped, withdrawn, canceled, halted. And some of this could apply to the previous case as well.

I think my favorites so far for hook-failed-omitted are called off, canceled, halted. I think called off even best.

It could be interesting to amass a large number of these potential labels, and run a survey through users of Mocha to see how well they feel each of those words describes (from 1 to 10) those two cases. Not to base our choice on our opinion alone.


Pending tests will be included in the test results, and marked as **pending**. A pending test is not considered a failed test.

## Exclusive Tests

Expand Down Expand Up @@ -1641,7 +1705,7 @@ mocha.setup({
### Browser-specific Option(s)

Browser Mocha supports many, but not all [cli options](#command-line-usage).
To use a [cli option](#command-line-usage) that contains a "-", please convert the option to camel-case, (eg. `check-leaks` to `checkLeaks`).
To use a [cli option](#command-line-usage) that contains a "-", please convert the option to camel-case, (e.g. `check-leaks` to `checkLeaks`).
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


#### Options that differ slightly from [cli options](#command-line-usage):

Expand Down
13 changes: 10 additions & 3 deletions lib/reporters/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,18 +343,25 @@ Base.prototype.epilogue = function() {
// passes
fmt =
color('bright pass', ' ') +
color('green', ' %d passing') +
color('green', ' %d / %d passing') +
color('light', ' (%s)');

Base.consoleLog(fmt, stats.passes || 0, milliseconds(stats.duration));
Base.consoleLog(fmt, stats.passes, stats.tests, milliseconds(stats.duration));

// pending
if (stats.pending) {
fmt = color('pending', ' ') + color('pending', ' %d pending');
fmt = color('pending', ' %d pending');

Base.consoleLog(fmt, stats.pending);
}

// skipped
if (stats.skipped) {
fmt = color('fail', ' %d skipped');

Base.consoleLog(fmt, stats.skipped);
}

// failures
if (stats.failures) {
fmt = color('fail', ' %d failing');
Expand Down
10 changes: 10 additions & 0 deletions lib/reporters/json-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
var Base = require('./base');
var constants = require('../runner').constants;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_RUN_BEGIN = constants.EVENT_RUN_BEGIN;
var EVENT_RUN_END = constants.EVENT_RUN_END;
Expand Down Expand Up @@ -43,6 +45,14 @@ function JSONStream(runner, options) {
writeEvent(['pass', clean(test)]);
});

runner.on(EVENT_TEST_PENDING, function(test) {
writeEvent(['pend', clean(test)]);
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
writeEvent(['skip', clean(test)]);
});

runner.on(EVENT_TEST_FAIL, function(test, err) {
test = clean(test);
test.err = err.message;
Expand Down
15 changes: 11 additions & 4 deletions lib/reporters/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@
var Base = require('./base');
var constants = require('../runner').constants;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_TEST_END = constants.EVENT_TEST_END;
var EVENT_RUN_END = constants.EVENT_RUN_END;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;

/**
* Expose `JSON`.
Expand All @@ -35,9 +36,10 @@ function JSONReporter(runner, options) {

var self = this;
var tests = [];
var passes = [];
var pending = [];
var skipped = [];
var failures = [];
var passes = [];

runner.on(EVENT_TEST_END, function(test) {
tests.push(test);
Expand All @@ -55,13 +57,18 @@ function JSONReporter(runner, options) {
pending.push(test);
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
skipped.push(test);
});

runner.once(EVENT_RUN_END, function() {
var obj = {
stats: self.stats,
tests: tests.map(clean),
passes: passes.map(clean),
pending: pending.map(clean),
failures: failures.map(clean),
passes: passes.map(clean)
skipped: skipped.map(clean),
failures: failures.map(clean)
};

runner.testResults = obj;
Expand Down
8 changes: 7 additions & 1 deletion lib/reporters/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var EVENT_TEST_BEGIN = constants.EVENT_TEST_BEGIN;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var color = Base.color;
var cursor = Base.cursor;

Expand Down Expand Up @@ -49,7 +50,12 @@ function List(runner, options) {
});

runner.on(EVENT_TEST_PENDING, function(test) {
var fmt = color('checkmark', ' -') + color('pending', ' %s');
var fmt = color('pending', ' - %s');
Base.consoleLog(fmt, test.fullTitle());
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
var fmt = color('fail', ' - %s');
Base.consoleLog(fmt, test.fullTitle());
});

Expand Down
6 changes: 6 additions & 0 deletions lib/reporters/spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var EVENT_SUITE_END = constants.EVENT_SUITE_END;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var inherits = require('../utils').inherits;
var color = Base.color;

Expand Down Expand Up @@ -88,6 +89,11 @@ function Spec(runner, options) {
Base.consoleLog(indent() + color('fail', ' %d) %s'), ++n, test.title);
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
var fmt = indent() + color('fail', ' - %s');
Base.consoleLog(fmt, test.title);
});

runner.once(EVENT_RUN_END, self.epilogue.bind(self));
}

Expand Down
16 changes: 11 additions & 5 deletions lib/reporters/tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ var util = require('util');
var Base = require('./base');
var constants = require('../runner').constants;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_TEST_END = constants.EVENT_TEST_END;
var EVENT_RUN_BEGIN = constants.EVENT_RUN_BEGIN;
var EVENT_RUN_END = constants.EVENT_RUN_END;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_END = constants.EVENT_TEST_END;
var inherits = require('../utils').inherits;
var sprintf = util.format;

Expand Down Expand Up @@ -63,6 +64,10 @@ function TAP(runner, options) {
self._producer.writePending(n, test);
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
self._producer.writePending(n, test);
});

runner.on(EVENT_TEST_PASS, function(test) {
self._producer.writePass(n, test);
});
Expand Down Expand Up @@ -199,10 +204,11 @@ TAPProducer.prototype.writeFail = function(n, test, err) {
* @param {Object} stats - Object containing run statistics.
*/
TAPProducer.prototype.writeEpilogue = function(stats) {
// :TBD: Why is this not counting pending tests?
println('# tests ' + (stats.passes + stats.failures));
println('# tests ' + stats.tests);
println('# pass ' + stats.passes);
// :TBD: Why are we not showing pending results?
if (stats.pending || stats.skipped) {
println('# skip ' + (stats.pending + stats.skipped));
}
println('# fail ' + stats.failures);
};

Expand Down
16 changes: 12 additions & 4 deletions lib/reporters/xunit.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,14 @@ var errors = require('../errors');
var createUnsupportedError = errors.createUnsupportedError;
var constants = require('../runner').constants;
var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var EVENT_TEST_SKIPPED = constants.EVENT_TEST_SKIPPED;
var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
var EVENT_RUN_END = constants.EVENT_RUN_END;
var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
var STATE_FAILED = require('../runnable').constants.STATE_FAILED;
var Runnable = require('../runnable');
var STATE_FAILED = Runnable.constants.STATE_FAILED;
var STATE_PENDING = Runnable.constants.STATE_PENDING;
var STATE_SKIPPED = Runnable.constants.STATE_SKIPPED;
var inherits = utils.inherits;
var escape = utils.escape;

Expand Down Expand Up @@ -78,6 +82,10 @@ function XUnit(runner, options) {
tests.push(test);
});

runner.on(EVENT_TEST_SKIPPED, function(test) {
tests.push(test);
});

runner.on(EVENT_TEST_PASS, function(test) {
tests.push(test);
});
Expand All @@ -95,7 +103,7 @@ function XUnit(runner, options) {
tests: stats.tests,
failures: 0,
errors: stats.failures,
skipped: stats.tests - stats.failures - stats.passes,
skipped: stats.pending + stats.skipped,
timestamp: new Date().toUTCString(),
time: stats.duration / 1000 || 0
},
Expand Down Expand Up @@ -180,7 +188,7 @@ XUnit.prototype.test = function(test) {
)
)
);
} else if (test.isPending()) {
} else if (test.state === STATE_PENDING || test.state === STATE_SKIPPED) {
this.write(tag('testcase', attrs, false, tag('skipped', {}, true)));
} else {
this.write(tag('testcase', attrs, true));
Expand Down
20 changes: 18 additions & 2 deletions lib/runnable.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function Runnable(title, fn) {
this._retries = -1;
this._currentRetry = 0;
this.pending = false;
this.skipped = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

You know, if we have Test#state, maybe we should have Runnable#state instead, and do away with these booleans...?

}

/**
Expand Down Expand Up @@ -166,6 +167,17 @@ Runnable.prototype.isPassed = function() {
return !this.isPending() && this.state === constants.STATE_PASSED;
};

/**
* Return `true` if this Runnable has been skipped.
* @return {boolean}
* @private
*/
Runnable.prototype.isSkipped = function() {
return (
!this.pending && (this.skipped || (this.parent && this.parent.isSkipped()))
Copy link
Contributor

Choose a reason for hiding this comment

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

There should always be a parent afaik

);
};

/**
* Set or get number of retries.
*
Expand Down Expand Up @@ -270,7 +282,7 @@ Runnable.prototype.run = function(fn) {
var finished;
var emitted;

if (this.isPending()) return fn();
if (this.isSkipped() || this.isPending()) return fn();

// Sometimes the ctx exists, but it is not runnable
if (ctx && ctx.runnable) {
Expand Down Expand Up @@ -458,7 +470,11 @@ var constants = utils.defineConstants(
/**
* Value of `state` prop when a `Runnable` has been skipped by user
*/
STATE_PENDING: 'pending'
STATE_PENDING: 'pending',
/**
* Value of `state` prop when a `Runnable` has been skipped by failing hook
*/
STATE_SKIPPED: 'skipped'
}
);

Expand Down
Loading