-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
correctly test for error equality on jest #11937
Conversation
🦋 Changeset detectedLatest commit: 35856d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
+ if (val.name === 'ApolloError' || val.name === 'GraphQLError') { | ||
+ return null | ||
+ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this escape hatch, all these errors will only log [ApolloError: message]
, so we end up with error messages like Got: [ApolloError: message], expected: [ApolloError: message]
, with no idea on what's actually wrong.
06baf23
to
45fdc96
Compare
45fdc96
to
b91d34b
Compare
e instanceof Error ? | ||
GraphQLError.prototype.toJSON.apply(e) | ||
: (e as any), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessbell this is probably relevant to you - until now, the ApolloError
would be wrapped by another ApolloError
inside of ObservableQuery
. The link chain should return "plain" errors, they get wrapped later.
@@ -1060,15 +1062,14 @@ describe("useMutation Hook", () => { | |||
|
|||
expect(fetchResult).toEqual({ | |||
data: undefined, | |||
// Not sure why we unwrap errors here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoiler: we never did 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great find! Glad this will give us more confidence in our tests.
I noticed this when working on tests for subscription errors - jest is extremely lax when comparing errors:
https://github.com/jestjs/jest/blob/c04d13d7abd22e47b0997f6027886aed225c9ce4/packages/expect-utils/src/jasmineUtils.ts#L91-L94
This means that we probably have hundreds of assertions that do nothing and might just as well be wrong.
This was the code I used to make it work correctly to work on that one test - I imagine before we can merge this we'll have to adjust dozens of tests (or fix bugs that have been hidden by this).