do not recursively call process.exit()

[addaleax]

Description of the Change

When inside process.on('exit'), calling process.exit()
stops other exit event listeners from being invoked.

This has lead to issues with coverage generation with nyc
because a fallback exit detection mechanism in it that relies
on internal Node.js APIs was accidentally disabled in Node v11.7.0,
leaving nyc with no way to detect process exit.

Alternate Designs

No alternatives were considered.

Benefits

process.on('exit') handlers are run even if exitMochaLater is used. At least nyc relies on this behaviour.

Possible Drawbacks

I guess running extra code while exiting could have drawbacks.

Applicable issues

This fixes nodejs/node#25650 (which is already closed, but because Node.js is (possibly temporarily) reintroducing monkey-patching for the internal method in question, not because the underlying cause was addressed).

[jsf-clabot]

All committers have signed the CLA.

[addaleax]

I’ve also included a revert that resets the .travis.yml version to Node 11’s latest release – since this fixes the issue, that should work (it does locally for me).

[coveralls]

Coverage increased (+0.04%) to 91.106% when pulling 062f519 on addaleax:override-exit into 1abfef6 on mochajs:master.

[boneskull]

@addaleax Thanks. This looks correct, but I guess I can't really be sure given it's a behavior change. You never know what kind of nonsense user code is going to execute.

But now I'm curious how this ever worked in the first place--doesn't the process.exit() call cause the listener to be fired recursively?

[boneskull]

I'm going to add a couple extra tests around this. Our integration tests that hit this behavior (--no-exit) may not be taking exit codes into account.

[addaleax]

But now I'm curious how this ever worked in the first place--doesn't the process.exit() call cause the listener to be fired recursively?

I don’t know the reasons for this behaviour, but calling process.exit() while already exiting exits the process immediately rather than emitting the event again.

[plroebuck]

I’ve also included a revert that resets the .travis.yml version to Node 11’s latest release – since this fixes the issue, that should work (it does locally for me).

Lost here. Your patch to "lib/cli/run-helpers.js" eliminates the recursive call which would appear to fix the stated problem. So why is the TravisCI Node version fix needed as well?

[addaleax]

@plroebuck I’m not sure I understand the question – this reverts the Travis Node version change, because that was a workaround for the bug in question, and usually, projects want to have the latest versions of each major Node.js release in their Travis matrix?

[plroebuck]

My apologies. Read diff backwards accidentally (not realizing it had been modified 2 days earlier without formal PR) as we had been using the latest Node. Your PR was just putting it back like it was...

[boneskull]

yeah I just updated w/o PR it to fix our broken builds.