[Bug]: toThrow discards non-Error objects when used with promises, while accepting them in synchronuous code

[michkot]

Version

26.4.2

Steps to reproduce

describe("set", () => {
	test("syncTest", () => {
		expect(
			() => {throw {message: "testval"};}
		)
			.toThrow("testval");
	});
	test("asyncTest", async () => {
		await expect(
			async () => {throw {message: "testval"};}
		).rejects
			.toThrow("testval");
	});
});

run these tests

Expected behavior

I expect both tests to succeed. Alternatively, I would expect both tests to fail, as they are doing the same, +- the use of Promise.

Actual behavior

Synchronous (non-promise) tests succeeds, asynchronous tests fails with a misleading error Received function did not throw.

This happens because of #5670 merged in 27a1dc6, which added a condition (a I will be linking code from latest version) https://github.com/facebook/jest/blob/01c278019726d7b01c924dd185e10b79a1f97610/packages/expect/src/toThrowMatchers.ts#L93-L95 that the rejection-reason must be an Error, https://github.com/facebook/jest/blob/01c278019726d7b01c924dd185e10b79a1f97610/packages/expect/src/utils.ts#L376-L384 otherwise the received is plainly ignored and thrown is kept just initialized to null, and the code path continues as if the reason was null. Later on, an internal function (in this specific case toThrowExpectedString) is called, and see thrown to be null and fails the matcher with the mentioned misleading error Received function did not throw.

Note that the synchronous code path, where the received is a function (passed to expect()) to be called and which should throw, does not have any such condition!:
https://github.com/facebook/jest/blob/01c278019726d7b01c924dd185e10b79a1f97610/packages/expect/src/toThrowMatchers.ts#L108-L112

The full code: https://github.com/facebook/jest/blob/01c278019726d7b01c924dd185e10b79a1f97610/packages/expect/src/toThrowMatchers.ts#L91-L114

Additional context

It is completely legal to throw "non-Errors" (see the implementation of the isError check above) in JS, and the toThrow Jest API does not document that the thrown object/rejection must satisfy any special conditions (currently implemented to have Error in prot chain or has an overloaded toStringTag / be a special native object to satisfy the Object.prototype.toString test).

In my case, I am working with production code that has custom exceptions that do not have Error in their prototype chain, but they are compliant with the Error (typescript) interface. Jest toThrow was working flawlessly for synchronous code with them, but I got really puzzled when I tried to use it with async code!

I propose to simply drop the isError check, as there is no technical need for it and the presence of that check is counter intuitive.
I was reading through #5670 which did not indicate any reason for this addition expect linking to older #4884 (comment) (to a specific comment that does not show up to me?), which also does not mention anyting about a need to limit this functionality only to "Error instances" - #4884 mainly fixes the matcher itself to work correctly in promise/async variants and updates docs to use an example with Error (which I think was good because it teaches good practices) instead of string.
#4884 links to an older #3601 which initally added promise/async support to all matches, but it did not work correctly for toThrow.

Tagging @peterdanis , an author of #5670 which caused this inconsistency.

Environment

irrelevant

[peterdanis]

I am very sorry for any inconvenience caused. To be honest, .toThrow matcher does not make much sense in terms of async code, as in the end there is no way (at least I am not aware of any) to distinguish between throwing and simply rejecting the promise.

Without isError check toThrow matcher would be the same as toBe / toEqual matchers, checking whether resolved/rejected value matches expected value (before #5670 it even worked on resolves.toThrow).

In your case you can use rejects.toBe / rejects.toEqual instead, but I agree with you that I should have documented that rejects.toThrow chain expects an actual Error object to be thrown.

[michkot]

First, let me thank you for a superb fast reaction!

To be honest, .toThrow matcher does not make much sense in terms of async code, as in the end there is no way (at least I am not aware of any) to distinguish between throwing and simply rejecting the promise.

Right, I am also not aware of a way to distinguish whether something from within async function throw an exception which triggered a rejection of the implicit promise, or if it there was a non-async function returning (in the future) rejected Promise.
But with the uptake of async/await that's exactly why I would expect reejcts.toThrow to work the same way as if it works with a synchronous function throwing trough call-stack.

Without isError check toThrow matcher would be the same as toBe / toEqual matchers, checking whether resolved/rejected value matches expected value ...

Not true AFAIK!. It actually has it's own logic https://github.com/facebook/jest/blob/01c278019726d7b01c924dd185e10b79a1f97610/packages/expect/src/toThrowMatchers.ts#L116-L141 to compare the thrown object/rejection "reason", based on what is pass in the "expected" value. If it's string, it compares it with message property of the thrown object. If it's object, it compares its message property with that property on the thrown object, there is variant with prototype check...

If these are valid use-cases for "non-errors" in the synchronous toThrow (without isError restriction), I don't see why they should be invalid for "non-errors" in the rejects.toThrow. Am I missing something?

(before #5670 it even worked on resolves.toThrow)

From the diff, I would assume that
await expect( async () => {throw new Error()} ).resolves .toThrow(Error);
still worked after it that change?
If the problem is that fromPromise is set equally by .resolves and .rejects, I would suggest we make a "better fix" 👍.

but I agree with you that I should have documented that rejects.toThrow chain expects an actual Error object to be thrown.

Frankly, I haven't known about Jest at that time, but I still don't see a point for a) doing change at all b) doing it just for the async case. I would make it at least less surprising if it would be done as a breaking change for the synchronous toThrow too back in the day....

[michkot]

//edited:

I would like to add that after some more playing with the toThrow matcher, it's capability, which is actually also listed here: https://jestjs.io/docs/expect#tothrowerror (I tried to reverse engineer it in the previous message), is very limited, so I will likely aim for a custom matcher that is able to check something else then the message! (that's actually easier with async flow, just .rejects.toMatchObject(..) for example)

It seems I hit the same thing as #8140 :/

[wtwhite]

I was just bitten by this, and being a JS noob, spent too many hours assuming the problem was somewhere in my code-under-test.

If code changes are out of consideration, could we please at least update the Jest docs to make the Error requirement explicit, and recommend .rejects.toBe()/toEqual() for handling "general" (i.e., possibly non-Error) exceptions? Happy to make a docs PR for this if there's some chance it will be merged. Thanks!

[the-ress]

In your case you can use rejects.toBe / rejects.toEqual instead, but I agree with you that I should have documented that rejects.toThrow chain expects an actual Error object to be thrown.

Also the message should be changed from the absolutely misleading "Received function did not throw" to something else.

[github-actions]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

[Expertus777]

I encountered the same issue. It would be nice to fix it or at least document it.

[distante]

This error cost me some time debugging. 😭