-
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
New testing helpers using using
, replace old console mocking functions.
#11177
Conversation
|
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -3,13 +3,6 @@ | |||
exports[`@client @export tests should NOT refetch if an @export variable has not changed, the current fetch policy is not cache-only, and the query includes fields that need to be resolved remotely 1`] = ` |
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.
This snapshot was confusing me to no end, but it seems that for some reason, delayed console.log
calls from other tests has spilled over into these ones.
On main
, calling these tests individually, they had very different snapshots from just running the whole test file.
That seems to have somehow surfaced during this refactor.
(resolve, reject) => { | ||
it("should warn if server returns wrong data", async () => { | ||
using _consoleSpies = spyOnConsole.takeSnapshots("error"); | ||
await new Promise((resolve, reject) => { |
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'm not incredibly happy about this way of removing itAsync
, but in this case, we need a way of holding _consoleSpies
in scope until the test is finished. This was the "least code changes" way of doing so.
I don't expect this pattern to be necessary in future code, as we have generally moved away from itAsync
.
jest.SpyInstance<void, any[], any> | ||
>; | ||
|
||
/** @internal */ |
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.
By the way, I've started using these @internal
annotations because it might make sense to bring Api Extractor into the picture at some point, which would help us keep a good overview of changes in our public type surface.
using
using
, replace old console mocking functions.
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.
@phryneas and I walked through this in details offline - very excited to begin using these utilities (pun unavoidable in my defense) 🎉🎉
This PR introduces three new internal test helpers to make use of the new "Explicit Resource Management" feature with
using
andawait using
that has made it into TypeScript 5.2.to create disposable objects and the two helpers
that mock console methods until disposed on scope exit.
To make sure that
Disposable
andAsyncDisposable
objects are always initialized withusing
, I added a lint rule.It also
@deprecate
s the old mocking helper functionswithErrorSpy
,withWarningSpy
andwithLogSpy
and removes all usage of them from our code.The PR removes a most usages of manual console mocking, but not all of them (a few last one would require bigger test refactors).
This might look like a small change, but the
withCleanup
pattern will allow us to write more self-contained test without the use ofbeforeEach
,afterEach
etc.Those helpers seemed nice for setting up tests, but as the tests grew, made it really hard to reason about our tests later on.
Checklist: