mocha 4 doesn't exit unlike mocha 3

[sagiegurari]

Prerequisites

• [x ] Checked that your issue isn't already filed by cross referencing issues with the common mistake 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.
• [ x] '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
• [ x] 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

I have been running specific set of tests for few years now and have been always upgrading to the latest mocha and everything was ok.
With mocha 4 suddenly all tests are passing but it doesn't end as if the --no-exit is automatically added although I never added it.

Steps to Reproduce

Expected behavior:
After all tests end, the process should stop even if there are timeoutes or sockets that prevent the process from existing.

Actual behavior:
Mocha 4 process waits forever like mocha 3 with --no-exit flag

Reproduces how often:
With our tests always. I have 700 tests so it is hard to pinpoint which one casuses the issue or if maybe it is in our codebase.

Versions

mocha 4.0.0 fails. before that everything works good.

[mkrufky]

I am seeing the exact same problem. Tests will pass and then mocha just hangs. See my Travis CI:
https://travis-ci.org/mkrufky/node-dvbtee/builds/282593109

I also noticed this same problem on video-dev/hls.js - mocha just hangs after passing tests:
https://travis-ci.org/video-dev/hls.js/builds/282590422

[pgilad]

Did you guys read the release notes? https://boneskull.com/mocha-v4-nears-release/#mochawontforceexit

[sagiegurari]

Thanks. tok bad the mocha website is not updated with this breaking change. The cli args there do not mention it.

[pgilad]

Other than providing a quick fix (use --exit), I do agree they left the core issue of finding the faulty tests to the user. I'm struggling with that myself now, but when upgrading major versions, I make sure to read the release notes and not blindly upgrade

[borisovg]

The release notes suggest using why-is-node-running except it doesn't work due to mafintosh/why-is-node-running#7

[mceachen]

Hit by this too. I understand the reasoning behind the change to the default, and thank you for incrementing the major version, but given that why-is-node-running is abandoned and broken, this may not be the most user-friendly change.

[ScottFreeCode]

Hi all,

First off, I want to apologize for the rough upgrade path on this point -- we definitely agree that Mocha ought to tell the user what's keeping the tests running (and then it wouldn't even need to keep the process open; it could just be a reason to return failure after they're done), but haven't found an entirely satisfactory way to do so yet. Version 4 didn't get the amount of time we wanted to put into it, as it was prompted by our CI failing due to changes in PhantomJS 1.x's installer (package-lock.json probably would have prevented this if we'd had it set up beforehand, but we still can't get it working). why-is-node-running was the one tool we found to help, but we don't think we can integrate it (between the requirement of --expose-internals and lack of a good way to programmatically get its output); I have found that it does work if you follow a couple steps:

• run Mocha with --expose-internals (node --expose-internals node_modules/mocha/bin/_mocha)
• make your first test file (e.g. listed in mocha.opts) contain (or at least begin with) after(require('why-is-node-running'))

...so it's better than nothing (although I'll see if I can update the release notes to describe this in more detail), but if anyone knows a better option please let us know!

Also sorry about missing the site documentation -- we'll get it updated as soon as possible. (Once #2987 is done we can even make it an automatic part of our version/publish scripting!)

[borisovg]

@ScottFreeCode

borisov@glossy:~/test/mocha $ node --version
v6.11.3

borisov@glossy:~/test/mocha $ ./node_modules/.bin/mocha --version
4.0.0

borisov@glossy:~/test/mocha $ ./node_modules/.bin/mocha --expose-internals test.spec.js 
  error: unknown option `--expose-internals'

Edit:

This works:

node --expose-internals ./node_modules/.bin/mocha test.spec.js

[ScottFreeCode]

Right, sorry I wasn't clear about that -- --expose-internals is a Node option that can be used by running node --expose-internals ./node_modules/mocha/bin/_mocha instead of ./node_modules/.bin/mocha. That's also something we can fix, since Mocha passes certain flags on to Node and we can add --expose-internals to those flags.

[ScottFreeCode]

Created mochajs/old-mochajs-site#81 and #3045 to track updating the documentation and Mocha's Node flags, respectively. Will keep this issue open at least till the documentation is updated.

[mceachen]

@ScottFreeCode you may want to mention that --expose-internals only works for node <= 6. Hopefully people can downgrade to node 6 temporarily so they can find what timer needs to be cancelled or unref'ed, or sockets needing to be unref'ed. You may also want to point people to after and afterEach hooks to do cleanup.

(is there a global "after" hook that mocha calls when all tests are complete?)

In any event, I appreciate the help, and thanks for Mocha!

[ScottFreeCode]

you may want to mention that --expose-internals only works for node <= 6.

Are you sure? I tested with Node 8.6.0. Albeit not on all OSes, so I guess I should fire up every box I have and triple-check...

[boneskull]

Here's a Gist with a rough guide to work around this for the time being.

[boneskull]

Are you sure? I tested with Node 8.6.0. Albeit not on all OSes, so I guess I should fire up every box I have and triple-check...

This "works" in that it triggers the output from the module, but it doesn't actually give me much information, regardless of the version of Node.js.

I'm going to add some updates to the site, but please see my forthcoming comments on #3045.

[ScottFreeCode]

This "works" in that it triggers the output from the module, but it doesn't actually give me much information, regardless of the version of Node.js.

It gives a useful stacktrace for setTimeout and setImmediate, but no real info at all for other things such as a listening server (as I just recently found out while trying to understand how it's doing what it's doing). The "async-dump" example in the gist has a real advantage in terms of working for everything (and not requiring --expose-internals), even though it's only useable on newer versions of Node.

[boneskull]

I suppose my general advice would be "if you're pulling your hair out, just add --exit and relax". Then don't use it for your next project 😉

[boneskull]

...and here are the docs

[andywer]

Sorry to comment on the closed PR again, but there might a user-friendly solution here:

One could log a warning about the changed behavior after the runner has finished for 3s or so, but the process didn't exit.
Would just need to add a setTimeout() after the runner finished and call timeout.unref() to make sure this timeout doesn't prevent the process from exiting. If the timeout gets executed it's time for a warning 😉

[boneskull]

I’ve considered that but I’m wary that it will cause other problems with reporters or other integrations... I can’t prove it won’t.

[williamdes]

var timers = sinon.useFakeTimers({
    now: new Date().getTime(),
    shouldAdvanceTime: true,
});

If I forget timers.restore(); the process hangs forever.

[rivasvict]

Based on the documentation @pgilad sent here, to me there is a cleaner solution. As the documentaion says:

To avoid false positives and encourage better testing practices, Mocha will no longer automatically kill itself via process.exit() when it thinks it should be done running.

A cleaner solution then it would be to create a global after function (an after outside any describe function), I would recommend in a separate file like exit-mocha.js or exit-mocha if you will. The callback sent to after you can force a gracefully node process exit which is going to exit without any error. The file can be sent to mocha cli as if it was another test file (it could simulate an --exit flag)

exit-mocha.js or exit-mocha

after('Exit mocha gracefully after finishing all tests execution'. function () {
  // Exit node process
  process.exit();
});

Then you can execute mocha tests like:

mocha exit-mocha test/**/*.spec.js

or

mocha exit-mocha.js test/**/*.spec.js

It is important that if you are using wildcards for the name of the tests files like I did with test/**/*.spec.js that the name of the exit-mocha file does NOT match the wildcard pattern, otherwise, it will not be possible for you to use it as a "flag"

[machineghost]

@vctr90 That's a great solution, although you have a period where you should have a comma in your example. Also the whole thing can be code golf-ed down to just:

after('Exit mocha gracefully after finishing all tests execution', process.exit);

Is there any chance a dev could chime in on why adding this to Mocha proper would hurt someone (because it would clearly help a lot of people)?

[evert]

@machineghost I like the new behavior for 2 reasons:

1. Almost no other library will exit after it's 'done its work'. For example, closing a Node TCP server will not automatically trigger an exit. In that way, it's consistent with other libraries.
2. If node doesn't exit, it means that there's events still waiting to be resolved. It's probably better to try and improve your code so things get cleaned up after each test. It could also suggest that there's memory leaks.

So when I run into this, this is an incentive for me to try and clean up my code so it doesn't happen.

Your perspective on this might be 'i don't care about memory leaks', or 'this is not worth fixing in my application'. If you're in this category the easiest is to make a mocha.opts and add --exit.

[machineghost]

It just seems to me that this is about making more noise, not signal: in the significant majority of cases no one's app is going to get better because Mocha hung at the end.

If Mocha is going to default, it seems like it should default to ending when tests (and all after/afterEach calls) have finished. Not doing so just to tell people "hey your artificial testing environment is artificial" doesn't benefit the majority (or even a decent minority) of users.

If people really want to debug unclosed connections then it seems that should be the case you provide an option for. But all the rest of the time, does it really benefit the library's users to confuse everyone else with (what amounts to saying) "you're in a testing environment and one of a billion possible things is open"?

To put it another way, if you're going to tell 99% of the people that run into this "just use --exit" then maybe --exit shouldn't be a special option you have to provide ... maybe the default behavior should be to serve 99% of the user cases (while of course still giving users the option of --tell-me-if-I-have-unclosed-stuff-in-my-testing-environment-and-that-is-actually-what-i-am-trying-to-find-out)?

I mean, if you made that the option, realistically how often do you think people would even pass it?

[machineghost]

P.S. I just came across this: https://stackoverflow.com/questions/54999115/where-to-destroy-knex-connection. If you look at the second answer, this behavior in Mocha directly caused at least one user (who isn't me, and I swear I don't know them: I just found this by luck after making my last post) to add incorrect non-test code (this "feature" made them incorrectly think needed to call knex.destroy() in their app).

Summary version:

Nice Person: You probably don't usually need to explicitly call knex.destroy() – this is implied by the documentation itself saying (emphasis mine):

Nice Person: quotes docs

Confused Person: If I don't call knex.destroy() my script will hang. Then what does it mean we don't need to explicitly call knex.destroy()

Nice Person: Ah, you only mentioned elsewhere that this is for Mocha. For regular server uses you don't need to tear down the connection pool – for Mocha, you might want to look into a global teardown hook, see futurestud.io/tutorials/…

But to be clear, this user had a working environment, and because Mocha (essentially) told them their perfectly good code was wrong, they lost who knows how much time trying to solve the bug they created by destroying their connections. And that's just this one unlucky person who happened to do it in public, and who I happened to see.

So, I guess what I'm trying to convey is, this isn't just about what's philosophically correct, nor is it just that some noise is obscuring useful signals ... this behavior is actually causing harm, and making programmers waste time tilting at windmills.

[evert]

I'm sorry, I mistook your tired point for an honest question. I regret answering. Maybe the developers can lock this thread.

[levindoneto]

"mocha --reporter mocha-allure-reporter ./tests/controllers --exit" worked for me. Indeed the --exit is a very good workaround. I use the version 5.2.0 in my project.

[regaldisclaimer]

Is there a way to get the equivalent effect of using the --exit flag when using mocha "programmatically?"

I do not see a documented option for exit/noexit, nor a general way to pass in a flag string while using the NodeJS API.

Following the pattern of other options, I tried:

const mocha = new Mocha({
	exit: true,
});

but was not able to get the desired effect.

Cursory inspection of https://github.com/mochajs/mocha/blob/master/lib/mocha.js seems to show that this option needs to be added to not only the documentation, but also to the source.

[AjaySabarish97]

Other than providing a quick fix (use --exit), I do agree they left the core issue of finding the faulty tests to the user. I'm struggling with that myself now, but when upgrading major versions, I make sure to read the release notes and not blindly upgrade

How exactly do you pass it ? This is my package.json script

"scripts": { "test": "istanbul cover node_modules/mocha/bin/_mocha --exit test/Testcases/ " } and it's not working

[xdolnak]

Other than providing a quick fix (use --exit), I do agree they left the core issue of finding the faulty tests to the user. I'm struggling with that myself now, but when upgrading major versions, I make sure to read the release notes and not blindly upgrade

How exactly do you pass it ? This is my package.json script

"scripts": { "test": "istanbul cover node_modules/mocha/bin/_mocha --exit test/Testcases/ " } and it's not working

Had the same issue and I think I am not the only one. I just had to move --exit at the end.
This did not work:
istanbul cover ./node_modules/mocha/bin/_mocha --exit -- test/.test.js
This did work me:
istanbul cover ./node_modules/mocha/bin/_mocha -- test/.test.js --exit

[boneskull]

exit does nothing when running Mocha programmatically, fwiw. the process is force-exited by the CLI wrapper, not Mocha-the-library.