Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The "cause" property of Error is ignored when matching #5697

Closed
6 tasks done
kettanaito opened this issue May 9, 2024 · 14 comments · Fixed by #5876
Closed
6 tasks done

The "cause" property of Error is ignored when matching #5697

kettanaito opened this issue May 9, 2024 · 14 comments · Fixed by #5876
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@kettanaito
Copy link
Contributor

Describe the bug

Vitest ignores the cause property whenever performing any equality on Error instances.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-fq9xyu?file=test%2Fbasic.test.ts&initialPath=__vitest__/

System Info

Irrelevant. The issue is in the matcher.

Used Package Manager

npm

Validations

@sheremet-va
Copy link
Member

This is documented behavior: https://vitest.dev/api/expect.html#toequal

A deep equality will not be performed for Error objects. Only the message property of an Error is considered for equality. To customize equality to check properties other than message, use expect.addEqualityTesters. To test if something was thrown, use toThrowError assertion.

@kettanaito
Copy link
Contributor Author

Hi, @sheremet-va. Got it. Can you give me an example of using .toThrowError() to also assert the cause property, please? I find it to be a common use case, I wouldn't want to add a custom tester/matcher for it.

@kettanaito
Copy link
Contributor Author

I don't believe .toThrowError() allows me to achieve the goal here. I highly suggest Vitest adds an API to help test more complex errors easier.

Perhaps something like this?

const error = vi.throws(fn)
expect(error).toEqual({ message: '', cause: '' })

@kettanaito
Copy link
Contributor Author

@sheremet-va, also, any chance to revisit this?

Only the message property of an Error is considered for equality.

I assume message is checked because every error can have a message. The same way, every error can have a cause property.

@sheremet-va
Copy link
Member

sheremet-va commented May 9, 2024

Technically you can use chai API for this:

expect(() => {
  throw new Error('test', { cause: new Error('cause') });
})
  .throws()
  .property('cause')
  .eql(new Error('cause'))

I am not sure what the best course of action is here, I also don't really like that it only checks the message. Will put it on the board to discuss with the team. If anyone has any ideas here, feel free to suggest here.

Maybe adding toThrowStrictError matcher? 🤔 (Ignoring stack is still probably a good idea 🤔 )

@sheremet-va sheremet-va added p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels May 9, 2024
@sheremet-va sheremet-va moved this to P2 - 4 in Team Board May 9, 2024
@kettanaito
Copy link
Contributor Author

kettanaito commented May 9, 2024

I think there are a few possible directions to this:

  1. Add cause as a special case alongside message, which equality will always be checked.
  2. Add .toThrowStrictError(), although why then differentiate between this and .toThrowError? To keep the previous behavior intact? Is the intention behind .toThrowStrictError() to strictly compare all error properties?
  3. Make catching errors easier via Vitest (e.g. vi.throws()). That won't count as an assertion but really a simpler way to do try/catch with, maybe, a built-in error thrown if the given function doesn't throw.

Technically you can use chai API for this:

That's a good call! I will remember this for myself but it won't work for what I'm preparing for my students right now.

Not checking arbitrary properties is okay. But cause is making its way into our code, and it's a defined property with a clear purpose (and also value type). I'd expect this to work:

expect(fn).toThrow(new Error('message', { cause: 123 }))

Need to be careful since cause is a potentially deep property. It can reference an error which has its own cause, which points to an error, which... I can recommend taking only the cause the user explicitly added in the matcher and ignoring any nested cause.

Alternatively, if this worked it'd also be great:

// Return me the error thrown by "fn"
// or throw if "fn" doesn't throw. 
// Returning null is also fine, depends
// if we want to treat this as implicit assertion. 
const error = vi.throws(fn)
expect(error.message).toBe('message')
expect(error.cause).toBe('cause')

@sheremet-va
Copy link
Member

sheremet-va commented May 9, 2024

I see your point. It would be nice to check cause since it's in the standard now. I believe we should also check the cause's cause and so on if it wasn't validated yet (hello, recursion).

Not sure about vi.throws - we have a few non-testing utilities already, so there is nothing stopping us from adding it, but I would prefer having an explicit assertion.

@kettanaito
Copy link
Contributor Author

kettanaito commented May 9, 2024

I believe we should also check the cause's cause and so on if it wasn't validated yet

Only if explicitly specified in the assertion 👍

const error = new Error('message', {
  cause: new Error('another', {
    cause: 123
  })
})

expect(error).toEqual(new Error('message')) // OK!
expect(error).toEqual(new Error('message', { cause: new Error('another') })) // OK!
expect(error).toEqual(new Error('message', { cause: new Error('another', { cause: 123 }) })) // OK!
expect(error).toEqual(new Error('message', { cause: new Error('another', { cause: 'bad' }) })) // X

expect(error).toStrictEqual(new Error('message')) // X
expect(error).toStrictEqual(new Error('message', { cause: new Error('another') })) // X
expect(error).toStrictEqual(new Error('message', { cause: new Error('another', { cause: 'bad' }) })) // X
expect(error).toStrictEqual(new Error('message', { cause: new Error('another', { cause: 123 }) })) // OK!

@hi-ogawa
Copy link
Contributor

hi-ogawa commented May 9, 2024

Something similar is brought up here

Comparing only Error.message is probably a historical reason and we should probably revisit this. In the issue, I was mentioning how node:assert's deep equality does #5244 (comment) since they treat Error differently.

It turns out Node doesn't check Error.cause since it's not enumerable. They only treat Error.name and Error.message as a special non-enumerable property to check.
https://stackblitz.com/edit/vitest-dev-vitest-ma1ej3?file=test%2Fbasic.test.ts

import assert from "node:assert";

// ok
assert.deepStrictEqual(
  new Error('x', { cause: 'foo' }),
  new Error('x', { cause: 'bar' })
);

This is not stopping Vitest from checking Error.cause but I thought it's worth mentioning.

@sheremet-va sheremet-va moved this from P2 - 4 to Has plan in Team Board May 23, 2024
@sheremet-va sheremet-va added p3-minor-bug An edge case that only affects very specific usage (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) labels May 27, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jun 12, 2024

Update from my last comment, Node v22 actually started to check Error.cause and AggregateError.errros for the equality.

https://nodejs.org/docs/latest-v22.x/api/assert.html#assertdeepstrictequalactual-expected-message

Error names, messages, causes, and errors are always compared, even if these are not enumerable properties. errors is also compared.

I made a quick repro here https://github.com/hi-ogawa/reproductions/tree/main/node-error-equality and I can verify the new behavior, but they probably haven't considered adjusting error diff, so their current assertion error message is quite cryptic as it doesn't include cause diff.

It looks like this is added quite recently (and maybe prematurely?) nodejs/node#51805, so we might want to wait a bit longer how it turns out in node world.

(Vitest also need to deal with how to format error diff properly since it currently only shows error.message. I was thinking to borrow some ideas from NodeJs, but it turns out that's not there yet, so that's what I wanted to point out.)

@dreamorosi
Copy link

Hello, is there any chance to revisit this?

As mentioned above in the thread, the cause property is becoming more and more popular so having an idiomatic way of testing it would be really helpful.

ralfstx added a commit to eclipsesource/pdf-maker that referenced this issue Oct 6, 2024
Since EcmaScript 2022, `Error` objects support a `cause` property,
allowing for tracking the root cause of an error.

This commit set this property where appropriate.

Since vitest's `toThrowError()` does not support checking the `cause` of
an error [1], we use utility functions to catch and extract errors
thrown by a function. This allows checking for individual properties of
an error.

[1]: vitest-dev/vitest#5697
@adrienZ
Copy link

adrienZ commented Oct 6, 2024

This is how I do it

import { it, expect } from "vitest";

    it("should test cause", async () => {
      const error = await myPromise().catch(e => JSON.parse(JSON.stringify(e)));
      expect(error).toStrictEqual({ cause: "myCause" });
  })

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 6, 2024

expect.objectContaining({ cause: ... }) also allows assertions like:

test('timeout', async () => {
await expect(async () => {
await expect.poll(() => false, { timeout: 100, interval: 10 }).toBe(true)
}).rejects.toThrowError(expect.objectContaining({
message: 'Matcher did not succeed in 100ms',
stack: expect.stringContaining('expect-poll.test.ts:38:68'),
cause: expect.objectContaining({
message: 'expected false to be true // Object.is equality',
}),
}))
})

@RobinTail
Copy link
Contributor

RobinTail commented Nov 2, 2024

expect.objectContaining({ cause: ... })

— this approach is good, @hi-ogawa, but it does not provide a hint when such assertion fails:

AssertionError: expected error to match asymmetric matcher

- Expected
+ Received

- ObjectContaining {
-   "cause": "...",
-   "message": "...",
- }
# no cause here:
+ [ErrorName: message]

Here is my approach — a custom serializer for Error having cause, that takes it into account for serialization:

// vitest.setup.ts
expect.addSnapshotSerializer({
  test: (subject: unknown) =>
    subject instanceof Error && subject.cause !== undefined,
  serialize: ({ name, message, cause }: Error) =>
    `[${name}: ${message} (${cause})]`,
});

Then you can do

expect(someFunction).toThrowErrorMatchingSnapshot()

However, it affects the performance (23 —> 28s on single thread), because instanceof is relatively expensive operation.

I wish there was an option in vitest.config.ts so that I could specify Error properties to serialize/compare.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants