mocha: Calling `done(undefined)` passes a test

I haven’t checked the docs, but I would be 99% certain that Mocha does a falsy/truthy check on the first argument passed to done. Imagine you have a child process, if the cp exits with 0, it is successful, if it exits with 1 or greater, it’s usually not. 0 is of course, falsy.

it('tests child process', function(done){

const cp = require('child_process');
const child  = cp.fork('foo.js');
child.on('exit',done);

});

Mocha is not checking for the amount of arguments passed to done, Mocha is checking whether the first argument passed to done is truthy. If it’s truthy, then the test fails. That is the expected behavior, TMK.

IMO the best thing to do would be to always log the first argument passed to the error-first callback, if arguments.length > 0, in the test results. That is the only change (besides the docs) that I see necessary. The pattern of truthy/falsy is so widespread in Node, it would be far more surprising to deviate from that than anything else.

in Node.js, we have this everywhere:

if(err){ cb(err); } else {

it’s a truthy/falsy check, and IMO Mocha should stay with that

On Sun, Nov 27, 2016 at 11:51 PM, Scott Santucci notifications@github.com wrote:

TL;DR: Documentation would be good, but I think – because false passes in testing are serious – being stricter than convention would better, although it may have to wait for 4.x.

@vivganes https://github.com/vivganes

Wouldn’t adding a err == undefined check affect any invocations that are called without any argument? If I just call done() wouldn’t that lead to err being undefined. Correct me if i am wrong.

Currently, these pass: undefined, null, 0, ‘’/“”, false. My thought was that only these should pass: undefined, maybe null. Changing !err to err !== undefined or err != undefined – not err == undefined – would accomplish that as far as I can tell.

(Although it appears that we’d also need to look at the logic for the fn called at the end there; that done function itself appears to mainly deal with timeout logic before passing the result on to fn. And see below…)

@adityavm https://github.com/adityavm

send: (obj) => { if (obj.error) done(obj.response.message); … }

Hmm, yeah, that would probably get Mocha’s “throw an Error instead” warning (still a test failure) if response.message weren’t undefined (is there a reason it’s not using done(obj.error)?), but now that I see it, it’s pretty obviously a case where Mocha treating falsey values as passes masks a mistake in the test design – only, since it’s undefined (assuming it is undefined and not an empty string, which is also falsey), even my proposed compromise of keeping undefined passing wouldn’t improve it.

And, come to think of it, in my hypothetical “using a variable that may be set to an error or may be left unset” example, if undefined was still treated as an error you’d just have to branch on whether the variable’s set to determine whether to call done with or without it instead of just calling done with the variable; so while calling done(maybeError) may seem much simpler than maybeError ? done(maybeError) : done(), the latter is entirely doable and this probably shouldn’t win out over safety.

So I guess I am leaning toward checking arguments.length after all.

While @boneskull https://github.com/boneskull has a point about consistency with Node patterns, and we should definitely make the docs clearer about it in any case, I think a test runner is one place where being more strict than usual can be warranted. After all, anything that makes tests always pass even if they should be failing is automatically a severe issue, since it can hide other issues of any severity. And, since they’re apparently passing, it’s not even easy to tell how widespread this problem is. (Although I would hope that most failures either will be thrown or will be coming in from a similar error parameter and passed to done directly instead of anything that could be undefined, I don’t know how we’d know if this is a widespread issue until it impacts something major – or until we make done strict enough to turn them all into errors.)

Another consideration: either this needs to go behind a flag (but we’ve got a lot of flags already and have at least one major tweak to the commandline processing going already) or this would need to be a semver “major” change.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mochajs/mocha/issues/2600#issuecomment-263204532, or mute the thread https://github.com/notifications/unsubscribe-auth/AKn56JZFHHtpkYMpuKnGmzkRfBOYF7Caks5rCogbgaJpZM4K80Wi .

– Alexander Mills ORESoftware (inc.)