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

Errors after test completion are swallowed #3226

Closed
4 tasks done
rpaterson opened this issue Jan 29, 2018 · 3 comments · Fixed by Urigo/tortilla#64
Closed
4 tasks done

Errors after test completion are swallowed #3226

rpaterson opened this issue Jan 29, 2018 · 3 comments · Fixed by Urigo/tortilla#64
Labels
type: bug a defect, confirmed by a maintainer

Comments

@rpaterson
Copy link

Prerequisites

  • Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
  • Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
  • 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
  • Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

Sometimes tests are written incorrectly and throw errors after they're complete. This applies to all three of Mocha's test forms (sync, promise, callback). If you write this sort of code in a normal Node program then the errors usually bubble up and kill the Node event loop, an error message is printed, and the process exits with a non-zero code. However when run with Mocha these errors are completely swallowed and never appear anywhere, and the process exits with code 0.

Obviously these tests are written badly, but I would expect Mocha to at least exit with an error code if a developer makes this sort of mistake! I can see how it may be difficult or impossible for Mocha to figure out which test caused the error, but it shouldn't just swallow the error completely.

Steps to Reproduce

describe('error after test complete', function() {

    it('sync', function() {
        setImmediate(() => {
            throw new Error('oops');
        });
    });

    it('promise', function() {
        setTimeout(() => {
            throw new Error('oops');
        }, 1000);
        return Promise.resolve();
    });

    it('callback', function(done) {
        setImmediate(() => {
            done();
            throw new Error('oops');
        });
    });
});

Expected behavior:

At minimum Mocha executable should fail with a non-zero error code and print out some sort of error message. Ideally Mocha would also be able to associate the error with the test that caused it, but I realize that may not be possible.

Actual behavior:

Tests pass, Mocha exits with code zero.

  error after test complete
    ✓ sync
    ✓ promise
    ✓ callback


  3 passing (16ms)

Reproduces how often: 100%

Versions

Tested with Mocha 5.0

  • The output of mocha --version and node node_modules/.bin/mocha --version: 5.0.0
  • The output of node --version: v8.9.3
  • The version and architecture of your operating system: Ubuntu 16.04, x86 64
  • Your shell (bash, zsh, PowerShell, cmd, etc.): bash
  • Your browser and version (if running browser tests): n/a
  • Any other third party Mocha related modules (with versions): n/a
  • The code transpiler being used: none
@rpaterson
Copy link
Author

I see other issues implying that maybe Mocha does error in these cases, but I can reproduce with the test case above 100% of the time. I tried specifying --exit and --no-exit but they didn't make any difference.

@Bamieh Bamieh added the type: question support question label Feb 1, 2018
@rpaterson
Copy link
Author

The ignoring code seems to be here: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L729. Looks like this behavior was introduced by #1578. The intent was to ignore async failures after the test had already failed, but as an unintended(?) side-effect Mocha now also ignores errors after the test has succeeded.

@boneskull
Copy link
Contributor

@rpaterson I think you're right that this is unintended.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer and removed type: question support question labels Mar 1, 2018
jleverenz added a commit to jleverenz/node-livereload that referenced this issue Nov 25, 2018
See the following mocha issue and changelog:

mochajs/mocha#3226
https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#502--2018-03-05

Test should have failed in all cases, but false passing occured
with older mocha versions.
jleverenz added a commit to jleverenz/node-livereload that referenced this issue Nov 25, 2018
A mocha defect silently ignored this for the previous package-lock mocha
version (5.0.1).

See the following issue and changelog:

mochajs/mocha#3226
https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#502--2018-03-05

Test should have failed in all cases, but false passing occured with older
mocha versions.
napcs pushed a commit to napcs/node-livereload that referenced this issue May 24, 2019
A mocha defect silently ignored this for the previous package-lock mocha
version (5.0.1).

See the following issue and changelog:

mochajs/mocha#3226
https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#502--2018-03-05

Test should have failed in all cases, but false passing occured with older
mocha versions.
velichkov added a commit to velichkov/node-smpp that referenced this issue Feb 18, 2022
The 5.* version is the latest one that supports node.js 4. Upgrade to
latest 9.2.* version requires node.js 12.*

Improve several unit tests as they started to fail because of
mochajs/mocha#3226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants