Report skipped tests upon beforeEach hook failure #4233

juergba · 2020-04-21T06:46:55Z

Description

required: #4221

describe("parent suite", function() {
    beforeEach(function() {
      throw new Error("this is a failure in a beforeEach hook");
    });
  
    it("test 1", function() { });
    it("test 2", function() { });
  
    describe("inner suite", function() {
      it("test 3", function() { });
    });
})

Before: a failing beforeEach hook ignores the three tests completely.

parent suite
    1) "before each" hook for "test 1"

0 passing (14ms)
1 failing

1) parent suite
     "before each" hook for "test 1":
   Error: this is a failure in a beforeEach hook
   [...]

After:

parent suite
    1) "before each" hook for "test 1"
    - test 1
    - test 2
    inner suite
      - test 3

0 / 3 passing (20ms)
3 skipped
1 failing

1) parent suite
     "before each" hook for "test 1":
   Error: this is a failure in a beforeEach hook
   [...]

Description of the Change

add a new event EVENT_TEST_SKIPPED
add a new test.state: skipped
beforeEach hook: each skipped test is reported and summerised in the epilogue. The output is very similar to pending test cases, but in color red.
there are no changes to the hook pattern, skipped tests will not be executed.
adapt reporter JSON-STREAM, LIST, TAP and XUNIT

Applicable issues

#1955
#1815

coveralls · 2020-04-21T06:56:48Z

Coverage increased (+0.1%) to 93.211% when pulling bb10ce9 on juergba/beforeEach into 400bf9b on master.

boneskull

wow, this is great. tests look good too. I don't see any major technical issues, and I think this is a good idea in general. mainly I'm concerned about the mismatch between our terminology and function names, as I've commented here. looking forward to your thoughts

boneskull · 2020-04-30T22:58:31Z

docs/index.md

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


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

boneskull · 2020-04-30T22:59:52Z

docs/index.md

+
+  describe('inner', function() {
+    before(function() {
+      // will be skipped


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?

boneskull · 2020-04-30T23:01:32Z

docs/index.md

 ## 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:


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

boneskull · 2020-04-30T23:02:19Z

docs/index.md

@@ -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).


suggestion:

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

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.

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.

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.

boneskull · 2020-04-30T23:03:16Z

docs/index.md

@@ -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`).


boneskull · 2020-04-30T23:26:50Z

lib/runner.js

+      if (err || test.isPending()) {
+        if (test.isPending()) {
+          if (self.forbidPending) {
+            self.fail(test, new Error('Pending test forbidden'), true);


this should probably be an error with a code as in lib/errors.js

boneskull · 2020-04-30T23:27:44Z

lib/runner.js

@@ -726,8 +747,9 @@ Runner.prototype.runSuite = function(suite, fn) {
  }

  this.emit(constants.EVENT_SUITE_BEGIN, (this.suite = suite));
+  suite.started = true;


if we have any more booleans on Suite like this, we should consider a state prop instead

boneskull · 2020-04-30T23:27:58Z

lib/runner.js


-  function next(errSuite) {
+  function nextSuite(errSuite) {


boneskull · 2020-04-30T23:28:24Z

lib/runner.js

@@ -840,7 +852,7 @@ Runner.prototype.uncaught = function(err) {

  runnable.clearTimeout();

-  if (runnable.isFailed()) {
+  if (runnable.isFailed() || runnable.isSkipped()) {


pls update the debug statement here

boneskull · 2020-04-30T23:29:10Z

lib/suite.js

@@ -62,6 +62,7 @@ function Suite(title, parentContext, isRoot) {
  this.suites = [];
  this.tests = [];
  this.pending = false;
+  this.skipped = false;


yeah, so, if it's not possible to have this.pending && this.skipped === true, then we should use a state

JoshuaKGoldberg · 2024-03-01T18:57:43Z

Per #1815 (comment), marking this and #4221 as status: blocked for now. We can take a deeper look once we've gone through some smaller changes first.

juergba mentioned this pull request Apr 21, 2020

Reporter HTML: listen to new 'EVENT_TEST_SKIPPED` event #4231

Closed

juergba force-pushed the juergba/beforeEach branch from 4f28b5b to 9bd10b3 Compare April 22, 2020 16:10

merge PR4221 - beforeAll

1a49fb0

juergba force-pushed the juergba/beforeEach branch 2 times, most recently from 7caf4f7 to da95e71 Compare April 29, 2020 08:04

juergba removed the status: needs review a maintainer should (re-)review this pull request label Apr 29, 2020

juergba added 5 commits April 30, 2020 15:42

report skipped tests upon failing beforeEach

8b1ec51

extend existing tests, additional tests

b70bf5a

adapt reporter xunit, json-stream, list, tap

eafd390

reporter tests

5cfb8d6

docs

bb10ce9

juergba force-pushed the juergba/beforeEach branch from bfe684c to bb10ce9 Compare April 30, 2020 13:44

juergba mentioned this pull request Apr 30, 2020

Report skipped tests upon beforeAll hook failure #4221

Open

juergba marked this pull request as ready for review April 30, 2020 14:48

juergba requested a review from a team April 30, 2020 14:48

juergba self-assigned this Apr 30, 2020

juergba added the status: needs review a maintainer should (re-)review this pull request label Apr 30, 2020

boneskull reviewed Apr 30, 2020

View reviewed changes

juergba removed status: needs review a maintainer should (re-)review this pull request type: cleanup a refactor block labels Aug 31, 2021

alexmi256 mentioned this pull request Oct 20, 2021

Subsequent Tests Do Not Run If beforeEach ever fails webdriverio/webdriverio#7577

Closed

JoshuaKGoldberg added the status: blocked Waiting for something else to be resolved label Mar 1, 2024

JoshuaKGoldberg unassigned juergba Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report skipped tests upon beforeEach hook failure #4233

Report skipped tests upon beforeEach hook failure #4233

juergba commented Apr 21, 2020 •

edited

Loading

coveralls commented Apr 21, 2020 •

edited

Loading

boneskull left a comment

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

juergba May 1, 2020 •

edited

Loading

dasilvacontin Aug 15, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

boneskull Apr 30, 2020

JoshuaKGoldberg commented Mar 1, 2024 •

edited

Loading

Report skipped tests upon beforeEach hook failure #4233

Are you sure you want to change the base?

Report skipped tests upon beforeEach hook failure #4233

Conversation

juergba commented Apr 21, 2020 • edited Loading

Description

Description of the Change

Applicable issues

coveralls commented Apr 21, 2020 • edited Loading

boneskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juergba May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 1, 2024 • edited Loading

juergba commented Apr 21, 2020 •

edited

Loading

coveralls commented Apr 21, 2020 •

edited

Loading

juergba May 1, 2020 •

edited

Loading

JoshuaKGoldberg commented Mar 1, 2024 •

edited

Loading