mocha
mocha copied to clipboard
mochajs
fix: do not exit when only unref'd timer is present in test code
In the timeout-handling code in Runnable#resetTimeout, if _enableTimeouts was false, Mocha would completely avoid setting a timer, even if the timeout was set to 0.
This PR changes the behavior so that a timer is always set, regardless of _enableTimeouts. If the test timeout value is 0, we set it to the maximum delay value (docs). ~~Even if you happen to leave Mocha running in your debugger for longer than 24.8 days, the timer will just reset itself instead of throw a timeout error. Talk about corner cases...~~
UPDATE: Resetting the timeout is problematic if timeouts are disabled after the timeout is reset. We don't really need this behavior anyway.
Fixes #3817.
I note that _enableTimeouts is essentially a useless, internal flag that could be removed (across several different files) if we assume a timeout value of 0 is false and anything else is true.
@boneskull - I pulled your branch in to my local copy of mocha, I then npm link'd it in to my own project's npm repo, and ran the tests that initially caused my failues, in both normal, and debug mode, using webstorm to do it. (i.e. the worst case scenario). I also ran individual tests as selected from the WebStorm UI that uses the --grep functionality to run individual tests only.
I can (happily) report a success. It ran all tests and skipped none (and these things were failing before).
@juergba - when the task completes: https://github.com/mochajs/mocha/blob/603da4464f34b58fb7bc96707680976b0f76cbe3/lib/runnable.js#L330
Unfortunately there's more to be done here. My guess is there's a problem with --no-exit / --exit behavior, as the builds seem to hang.
@boneskull - I ran in to the unit tests here hanging with --timeout 0 - as I mentioned in #3817. It may help you narrow down the issue... maybe. https://github.com/mochajs/mocha/blob/master/test/unit/mocha.spec.js#L281
what I'm seeing is a hang in the Runnable unit tests around some edge-case stuff
coverage: 94.33% (-0.03%) from 94.359% when pulling 1dbfcf7e3a442792b7eef007d429296e55351f53 on boneskull/issue/3817 into 2f3fedcc41cbb9d3e503d84098fcc07d7c3c49f1 on master.
out of topic: when debugging with VSCode, the CL input looks like this:
node.exe --inspect-brk=19942 bin\_mocha D:/mocha3291/unref.js
This way the --inspect-brk is consumed directly by node and is not piped through Mocha first. Therefore timeout=0 has to be set manually while debugging in order to avoid a timeout error.
Nevertheless docu states:
Implies --no-timeout.
When I run
'use strict';
it('just kind of stops', function(done) {
setTimeout(done, 10).unref();
});
it('just kind of stops', function(done) {
setTimeout(done, 10).unref();
});
the output is:
√ just kind of stops
√ just kind of stops
2 passing (31ms)
while debugging the same code, the output contains additional duration information:
√ just kind of stops (9410ms)
√ just kind of stops (5798ms)
2 passing (15s)
This solution sets a timer for each individual test. Just as if user would call --timeout 999999 by himself or Mocha translates --timeout 0 into --timeout 999999 for convenience.
I would prefer a root timer, similar to the one proposed by @rolfl.
In "Mocha.prototype.run":
var runner = new exports.Runner(suite, options.delay);
var setTimeout = global.setTimeout;
if (suite._timeout === 0) {
runner.rootTimer = setTimeout(function() {}, 999999);
}
rootTimer has to be cleared on the EVENT_RUN_END event.
This works and "npm test" terminates successfully. Yes, there is this issue with the global timers.
I would prefer a root timer, similar to the one proposed by @rolfl.
I'd have to see this working to be convinced it's the right choice.
UPDATE:
Let me clarify.
- This of course adds an extra timer, regardless of the value of
timeout. - Most runs of Mocha will have a nonzero
timeoutvalue. - I don't fully understand the impact and if there are any unintended consequences to doing this (FUD). Does this impact watch mode, signals,
--exit/--no-exit, browsers? While our test coverage is pretty OK in Node.js, there's always edge cases and interplay between various usages that we're missing.
This PR sticks to the "status quo", so I'm more comfortable with this strategy.
√ just kind of stops (9410ms) √ just kind of stops (5798ms) 2 passing (15s)
This is "slow" output. People using --timeout 0 or --timeout 99999999 don't usually bother setting --slow 0 or --slow 99999999999 anyway; this is how you'd suppress it.
I wouldn't be opposed to updating the default value of slow to be a function of the value of timeout (I've mentioned this before) but it doesn't belong in this PR.
This way the --inspect-brk is consumed directly by node and is not piped through Mocha first. Therefore timeout=0 has to be set manually while debugging in order to avoid a timeout error.
This is why the debuggers which, when creating a "Mocha" debug profile, use node bin/_mocha --inspect-brk --timeout <value>.
The documentation is technically correct, but it might be worth mentioning that this only applies when executing bin/mocha. Please create an issue?
@boneskull - re:
I would prefer a root timer, similar to the one proposed by @rolfl.
I'd have to see this working to be convinced it's the right choice.
In all my testing with the solution I proposed (using an interval instead of a timeout to avoid the 24-day limit) it works really well except for in the base mocha.#run() test case, where it appears sinon is interfering with either the setInterval or the clearInterval. I can investigate further, but the basic logic is similar to the logic you propose, except I put it out her for the overall run, wheras your change above puts it there for the individual suites. I put a ref() interval on the event loop that prevents node exit... and clear it when the mocha.run completes (on end event or done(...) call.
@rolfl I can't see what you did, but you can't just use setTimeout or setInterval without dereferencing it, e.g. var setInterval = global.setInterval, because Sinon (or Unexpected, or any other test helper) could monkey with it. you may have gotten an ESLint error if this was the case.
BTW just noted, there is a very old branch about that issue: https://github.com/mochajs/mocha/commit/ead35769eea3e4576b53e8fe7cfdfa441c505b1c
Released in [email protected]. We did it! 🚀
s/o @boneskull for setting up this change over five years ago.
