Calling done(undefined) passes a test

[adityavm]

When using done() in an async test, if it's called with an argument whose value is undefined, the test is successfully passed.

Simple example:

it("should fail, but will pass", (done) => {
  done(undefined);
});

This feels like incorrect behaviour because done was explicitly called with an argument, which happened to not have a value. A test should only pass if done is called without arguments (as per docs).

[ORESoftware]

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.

[ScottFreeCode]

I just did a quick check and the actual code is in fact using truthiness/falsiness. Good investigation there, @ORESoftware; thanks!

I don't know that mere falsiness is really the intended criteria though -- it seems like anything other than done() or done(<some Error>) is considered incorrect. For instance, in that child process example, if the process exited with a non-zero code and passed that to done Mocha would consider the test failed but warn you to use a real error instead of 1 (or whatever number the exit code is). And falsey in JS even includes empty strings (would love to know the reason for that someday), which I'm definitely sure we shouldn't treat as passing the test since non-falsey strings are treated as errors that are missing their Error wrapping.

However, I could see done(undefined) being treated like done(): undefined, after all, is more or less equivalent to no variable/property at all (modulo a few edge cases that can be quite annoying) -- that's the difference between undefined and null, isn't it, that null means "there's a variable here but it has no value" whereas undefined means "there's no variable here"? Granted, the aforementioned edge cases do show up from time to time and mess with that, but in principle I have never understood why undefined should mean "there's a variable here that's masquerading as not being a variable here but in fact it is here anyway" (I've always assumed this inconsistency, where undefined is mostly like no variable except where it's occassionally more like a redundant version of null, was just one of those mistakes from the language's early hacked-together days, like the half dozen things that "use strict" fixes ...maybe I just don't know the rationale though).

Speaking more practically, I could see it being potentially useful to write tests where done(<some variable that may be an error or may be undefined>) is used instead of <some variable that may be an error or may be undefined> ? done(<same variable>) : done(). (Or maybe even including null with undefined in this case, as a variable with no value is still niether a variable with an error value nor a variable with some incorrect other value.) While it might be theoretically surprising to have done(undefined) be treated as done() (unless you're like me and think of undefined as "this is not even a variable"), I can't think of a use case for passing undefined directly -- only for passing a variable that might be undefined. And if you did expect the variable to not be undefined and to fail the test no matter what, you could start off your test file with var missingError = new Error("Error variable was not set") and call done(<error variable> || missingError).

So, my inclination is to say we should change the truthiness check to !== undefined or perhaps != undefined, thus eliminating treating 0 and ""/'' as passing but retaining the ability to pass by leaving an error variable unset (or set to null, perhaps).

I could be missing something though; @adityavm could you elaborate on your use case?

[adityavm]

@ScottFreeCode sure. The actual piece of code is this :

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

This test kept passing for a while, even though error was true, until I figured out that the response.message didn't exist. I would have expected Mocha to fail the test with a message saying argument to done was undefined or something to that tune.

But doing done(response || MissingError) isn't a bad suggestion either. Just a little cumbersome.

[vivganes]

@ScottFreeCode Just thinking aloud (I may be wrong here)

So, my inclination is to say we should change the truthiness check to !== undefined or perhaps != undefined, thus eliminating treating 0 and ""/'' as passing but retaining the ability to pass by leaving an error variable unset (or set to null, perhaps).

Taking a look at the code of done() method

  // finished
  function done (err) {
    var ms = self.timeout();
    if (self.timedOut) {
      return;
    }
    if (finished) {
      return multiple(err || self._trace);
    }

    self.clearTimeout();
    self.duration = new Date() - start;
    finished = true;
    if (!err && self.duration > ms && self._enableTimeouts) {
      err = new Error('Timeout of ' + ms +
      'ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.');
    }
    fn(err);
}

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.

[adityavm]

@ScottFreeCode a quick bit of code reveals:

I think using arguments and the length of it makes more sense.

[boneskull]

The docs should be changed to be more clear about this. It's a "node-style" callback (or "errback"); typically these simply evaluate the first parameter for truthiness. Thus, passing any non-truthy value to done() is not a failure and is expected behavior.

[boneskull]

Send PR to site repo

[ScottFreeCode]

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

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

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 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.

[ORESoftware]

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 ***@***.***> 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 <#2600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKn56JZFHHtpkYMpuKnGmzkRfBOYF7Caks5rCogbgaJpZM4K80Wi> .

-- Alexander Mills ORESoftware (inc.)

[ScottFreeCode]

...always log the first argument passed...

That's still going to pass CI, and if only a fraction of tests for a given project are getting undefined it might not be easy to spot when manually looking at the results either. I think we'd need a pretty big and hard to miss warning, perhaps somewhere like the very end of the test results (in which case we'd have to hang onto the data for the warning until then), when a falsey argument is passed (and not when no argument is passed at all), at the very least.

Mocha already goes out of its way to suggest following the convention of only passing an Error instance as an error even though it sticks to the convention that falsey values are non-errors. If we're not supposed to pass something that might be 1 or "<some string here>" without wrapping it in an Error, then arguably most patterns in which you might pass a falsey value such as 0 or "" are precluded anyway -- once Mocha sees a truthy one and warns you to use an Error instead, that is. The trouble is that sometimes passing something that's supposed to have a value might accidentally never show up Mocha's warning about non-error values because it's accidentally falsey. As good as Node's conventions are, that's arguably a worse problem in a test system than breaking convention would be.

Or on the other hand, when it comes to how bad of a problem it is to break the convention: in that particular example, substituting done for cb, it would still work just as well as before with a stricter done since a falsey err would never have been passed to done. We're not actually going to cause problems unless Mocha is expected to follow a convention wherein calling done with a falsey value (but not without a parameter at all) in order to pass the test is normal.

[adityavm]

@ScottFreeCode

is there a reason it's not using done(obj.error)?

Yes, because obj.error is a boolean while response.message is a new Error(<properMsg>) which makes for a better test failure message. I probably could break it out into 2-3 tests that tests each branch separately and hence tune the failure message appropriately for each test, but the application is already designed to throw proper error messages so I don't see a reason not to use it.

[ScottFreeCode]

Yes, because obj.error is a boolean while response.message is a new Error()

Ah, I had completely misread that! That might only make the case stronger, though -- if it's simply not expected to be undefined, and is expected to be an Error (as Mocha prefers), how are we going to know if some of those "passing" tests shouldn't have passed?

[adityavm]

@ScottFreeCode Correct, it's never supposed to be undefined. Only benefit to this was the test pointing out a branch in my code that I didn't notice, since I was trying to fail the test but it kept passing. I agree with you that it should lean on being stricter, given that it's a testing framework.

[dasilvacontin]

Uhh. Tbh, that you get an unexpected behavior from done(obj.response.message) totally sucks.

But I guess it can't be helped, since a lot of people might be doing something like:

someTask(function (err, result) {
  done(err)
})

Logging the value passed to done() could be an option...

[maraisr]

With respects to issue #3134, does that solve this issue. Or is this more to do with the library itself, rather than a documentation misunderstanding?

[sandeepEverest]

Facing similar issue when used mocha + cypress

facing similar issue
Cannot read property 'passes' of undefined
at Spec.Base.epilogue (node_modules/mocha/lib/reporters/base.js:318:25)
at Object.onceWrapper (events.js:316:30)
at emitOne (events.js:120:20)
at Runner.emit (events.js:210:7)
at Reporter.emit (Cypress/3.1.5/Cypress.app/Contents/Resources/app/packages/server/lib/reporter.js:239:55)
at Object.server.startWebsockets.onMocha (Cypress/3.1.5/Cypress.app/Contents/Resources/app/packages/server/lib/project.js:296:22)
at Socket. (Cypress/3.1.5/Cypress.app/Contents/Resources/app/packages/server/lib/socket.js:237:36)
at emitTwo (events.js:125:13)
at Socket.emit (events.js:213:7)
at Cypress/3.1.5/Cypress.app/Contents/Resources/app/packages/socket/node_modules/socket.io/lib/socket.js:503:12
at _combinedTickCallback (internal/process/next_tick.js:131:7)
at process._tickCallback (internal/process/next_tick.js:180:9)

[plroebuck]

@sandeepEverest, open a new issue and repost, this time including the relevant portion of your test.

[boneskull]

This is a documentation issue. Someone should update the documentation to reflect the fact that calling done with a single, falsy parameter, is not considered an error or failure.

[plroebuck]

It is documented...

[boneskull]

great!