-
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
Remove itAsync
from remaining relevant tests
#12210
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 30889cb03ddd87849c28058b |
|
commit: |
c976713
to
6c82bf7
Compare
6c82bf7
to
f2defe9
Compare
1e7d108
to
5bdd9db
Compare
f2defe9
to
ba16e3d
Compare
1ca4f08
to
79f6e3d
Compare
stream.unsubscribe(); | ||
|
||
await expect(stream).not.toEmitAnything(); |
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.
Same pattern as in the previous PR.
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.
if (obs === aObs) { | ||
throw new Error("aQuery should not have been updated"); | ||
} else if (obs === bObs) { | ||
expect(diff.result).toEqual({ b: "Bee" }); |
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.
Most tests in this file feel like they need an assertion on the count of expect
calls, otherwise these parts could also just never run and we wouldn't know.
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.
I agree. I started that and meant to come back. I need to do a minor update to the toEmit*
APIs because takeNext
, etc. use expect(...).toEqual
under the hood so some assertions get double counted (in other words, each expect(stream).toEmitValue(...)
counts as 2 assertions).
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.
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.
Left two comments, once those are addressed this is good to go.
79f6e3d
to
c32e923
Compare
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…counting assertions
This PR completes the work to remove
itAsync
from our tests. It is still used by the hoc and component tests, but these will be deleted with 4.0 so we can ignore these for now.itAsync
will be removed when we remove the query components and hoc on the 4.0 branch.