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

Ignore asynchronous errors after global failure #1540

Merged
merged 1 commit into from
Mar 6, 2015

Conversation

jugglinmike
Copy link
Contributor

Runner#uncaught labels the currently-executing Runnable instance as
"failed" in response to uncaught exceptions. If the the Runnable
instance later signals failure through Mocha's asynchronous interface,
the Runner counts this as a distinct test failure. Consequently,
reporters generate inaccurate reports (and in some cases, this triggers
uncaught exceptions causing the process itself to crash).

When a runnabe completes, explicitly check that it has not already been
labeled as failing before reporting any additional errors.

`Runner#uncaught` labels the currently-executing `Runnable` instance as
"failed" in response to uncaught exceptions. If the the `Runnable`
instance later signals failure through Mocha's asynchronous interface,
the `Runner` counts this as a distinct test failure. Consequently,
reporters generate inaccurate reports (and in some cases, this triggers
uncaught exceptions causing the process itself to crash).

When a runnabe completes, explicitly check that it has not already been
labeled as failing before reporting any additional errors.
@dasilvacontin dasilvacontin added the status: waiting for author waiting on response from OP - more information needed label Feb 8, 2015
@danielstjules
Copy link
Contributor

I'd noticed this before as well. And your PR looks good for the test case you mentioned. To highlight, for project maintainers. Given the following test, which only has one spec:

it('fails exactly once', function(done) {
  setTimeout(function() {
    setTimeout(function() {
      done(new Error('test error'));
    }, 0);
    throw new Error('global error');
  }, 0);
});

You get two two failures:

$ ./bin/mocha pr1540.js

  ․․

  0 passing (6ms)
  2 failing

  1)  fails exactly once:
     Error: test error
      at null._onTimeout (/Users/danielstjules/git/mocha/pr1540.js:10:12)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)
      at timers.js:117:18
      at process._tickCallback (node.js:415:13)
      at process._tickFromSpinner (node.js:390:15)

  2)  fails exactly once:
     Error: test error
      at null._onTimeout (/Users/danielstjules/git/mocha/pr1540.js:10:12)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)
      at timers.js:117:18
      at process._tickCallback (node.js:415:13)
      at process._tickFromSpinner (node.js:390:15)

But after this PR, it correctly only shows one failure, and highlights the error thrown first ("global error"):

$ ./bin/mocha pr1540.js

  

  0 passing (6ms)
  1 failing

  1)  fails exactly once:
     Uncaught Error: global error
      at null._onTimeout (/Users/danielstjules/git/mocha/pr1540.js:12:11)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

But while the PR does handle the case highlighted above, it doesn't fix all. Take, for example:

it('fails exactly once', function(done) {
  setTimeout(function() {
    done(new Error('test error'));
  }, 0);

  setTimeout(function() {
    throw new Error('global error');
  }, 0);
});

Even with the proposed changes, you still get:

$ ./bin/mocha pr1540.js

  ․․

  0 passing (6ms)
  2 failing

  1)  fails exactly once:
     Uncaught Error: global error
      at null._onTimeout (/Users/danielstjules/git/mocha/pr1540.js:7:11)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  2)  fails exactly once:
     Uncaught Error: global error
      at null._onTimeout (/Users/danielstjules/git/mocha/pr1540.js:7:11)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

@jugglinmike
Copy link
Contributor Author

Right on! Thanks for the assist, @danielstjules

@danielstjules
Copy link
Contributor

No problem, teamwork ✋ haha

@boneskull boneskull removed the status: waiting for author waiting on response from OP - more information needed label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants