Non-Error Types Not Being Detected Properly by .throw

[mreinigjr]

I am working on some legacy code that throws custom errors. In Chai 3.5.0 the following simple code package will pass with mocha 4.1.2. Run npm install first and then npm test after downloading and extracting the zip file.

chai-issue-1.zip(updated)

However, if you upgrade chai to 4.1.2, the test now fails.

npm install --save-exact chai@4.1.2
npm test

Test fails with AssertionError: expected [Function: exampleIssue] to throw { Object (badThingHappend) } but 'TypeError: badThingHappend is not a constructor' was thrown

Adding this to the parameter provided to .throw in the test fixes the issue and the test passes. Again, run npm install and npm test after downloading the package.

chai-issue-2.zip(updated)

I have not dug too deep to figure out what the underlying issue is between versions of chai, but a very simple fix would be to update the documentation near the second to last sentence in the .throw section. Just incase it is relevant, I am running npm version 5.5.1 and node version 6.11.5. Thanks!

[meeber]

@mreinigjr Before I go into long-winded explanations here, let me make sure of one thing: In lib/customErrors.js are you intentionally exporting module.exports = { badThingHappend }; instead of module.exports = badThingHappened? Because as the the examples currently stand, throw new badThingHappnd(); doesn't even really work since the thing being exported is an object instead of a function, thus the TypeError you're getting. (And the reason this passes in Chai v3.5 is because the .throw logic sees that badThingHappnd is an object and it doesn't know what to do with that so it ends up silently ignoring it, which is bad behavior imo.)

[mreinigjr]

@meeber, yes the code is exporting badThingHappend using module.exports = { badThingHappend }; and you are correct, I messed up the example. However, even if you change the line
const badThingHappend = require(path.join(__dirname, "../lib/customErrors.js"))
to
const badThingHappend = require(path.join(__dirname, "../lib/customErrors.js")).badThingHappend
the same result still occurs. I have updated the zip files in the initial post to correct this. I also added a few more tests. So please get a look at those again when you have time.

Also, I am not sure what you mean by badThingHappend is not a function. I included a test for badThingHappend and it passes validation as a function for both chai 3.5 and 4.1.2. It is true that after badThingHappnd is thrown, it is an object, but it is still an instance of badThingHappened. However, before it is thrown, it is a function. I added another test for this as well, sorry for it being a bit hacky. I hope I understood your comments here correctly, please let me know if I did not.

[meeber]

Also, I am not sure what you mean by badThingHappend is not a function.

My statement was only in regard to your original example. In your original example, the imported badThingHappend was an object instead of a function, so badThingHappend() resulted in AssertionError: expected [Function: exampleIssue] to throw { Object (badThingHappend) } but 'TypeError: badThingHappend is not a constructor' was thrown. Your original example was causing the test to fail in 4.1.2 but not in 3.5 due to bad logic in Chai's .throw in v3.5 when passed an object instead of a function.

Now let's forget about the original example and take a look at your updated example, specifically chai-issue-1. It's failing in 4.1.2 but not in 3.5, except this time for a completely different reason.

The error message is now: AssertionError: expected [Function: exampleIssue] to throw 'badThingHappend' but { message: 'Bad stuff happened' } was thrown.

There were changes made to .throw from 3.5 and 4.1.2, and it looks like the ability to pass any constructor as the first parameter to .throw, even constructors that don't extend Error, is no longer supported. You're right, the documentation needs to be updated, particularly this sentence: "With that said, the throw assertion does technically support any type of value being thrown, not just Error and its derivatives." The work being done in #1021 will eventually cover this change, but I'm not opposed to updating the docs now as a stopgap measure.

If you're stuck with a legacy codebase and need to write tests to assert that the correct objects are being thrown, even if they aren't actually Error objects or derivative of Error objects, then I recommend using this construct:

expect(exampleIssue).to.throw().and.be.an.instanceof(badThingHappend);

(As for chai-issue-2, this.badThingHappend is just undefined so .throw is ignoring that parameter and just checking whether or not the function threw anything at all.)

[mreinigjr]

Oh gotcha! That makes a ton of sense now. I should have looked into that deeper.

The sentence you referenced is exactly what caused the creation of this issue. Updating the docs with your example on how to assert on non-Error objects or its derivatives would have prevented the opening of this issue. Thanks for everything!

[mreinigjr]

So I am seeing some weirdness with functions that have parameters and throw non-Error objects. I cannot quite put my finger on the issue yet, but this seems to work in some cases,
expect(exampleIssue.bind(x, y)).to.throw().and.be.an.instanceof(badThingHappend);
and in other cases that kind of assertion will fail and I will need to do this to get the tests to pass,
expect(() => exampleIssue(x,y)).to.throw().and.be.an.instanceof(badThingHappend);
The arrow function method seems to always work. So you may want to just remove the sentence in the docs that states that .throw() will assert on non-Error objects or its derivatives. That will probably be the easiest/clearest until the work associated with #1021 is completed like you have stated. Any idea why this would be occurring? It could very well be the legacy code I am working with for the examples I provided do not seem to have this issue.

[meeber]

Hard to speculate on the .bind issue without a reproducible example. I don't see any reason why .throw would behave any differently when a function is created via .bind.

I don't know if it's related to the issue you're running into, but it's worth noting that the semi-example you provided above isn't really testing the same thing, since the first argument to .bind is supposed to be the value for this. You'd need something like this instead:

expect(exampleIssue.bind(null, x, y)).to.throw().and.be.an.instanceof(badThingHappend);
expect(() => exampleIssue(x, y)).to.throw().and.be.an.instanceof(badThingHappend);

[mreinigjr]

Thanks @meeber!