Remove itAsync part 2
#12207
Remove itAsync part 2
#12207
Conversation
|
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: ad4f8568faeafa24b1288118 URL: https://www.apollographql.com/docs/deploy-preview/ad4f8568faeafa24b1288118 |
commit: |
jerelmiller
force-pushed
the
jerel/remove-itasync-part-2
branch
3 times, most recently
from
8a7a769 to
3035297
Compare
jerelmiller
force-pushed
the
jerel/remove-itasync-part-2
branch
from
c976713 to
6c82bf7
Compare
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jerelmiller
force-pushed
the
jerel/remove-itasync-part-2
branch
from
6c82bf7 to
f2defe9
Compare
jerelmiller
force-pushed
the
jerel/remove-itasync-part-2
branch
from
f2defe9 to
ba16e3d
Compare
| }); | ||
|
|
||
| return new ObservableStream( | ||
| queryManager.watchQuery<any>(assign({ query, variables }, queryOptions)) |
There was a problem hiding this comment.
Just as a note, we should probably get rid of lodash in a follow-up PR
| }; | ||
| await expect(stream).toEmitError( | ||
| new ApolloError({ | ||
| graphQLErrors: [null as any], |
There was a problem hiding this comment.
Do we have another test somewhere else that test handling of the message for `null?
There was a problem hiding this comment.
Not to my knowledge, but I was more focused on updating tests to ObservableStream that I didn't pay attention if there was.
|
|
||
| stream.unsubscribe(); | ||
|
|
||
| await expect(stream).not.toEmitAnything(); |
There was a problem hiding this comment.
This tests more our implementation of ObservableStream than the underlying observable - I think we can skip this line
| await expect(stream).not.toEmitAnything(); |
There was a problem hiding this comment.
There are a few more occurances on this, but trying to comment on that file almost crashes my browser, so I'll not mention them.
There was a problem hiding this comment.
Updated in cfdffaf with a bit better way to check that polling queries are properly stopped after unsubscribing
| expect(queryManager.mutationStore).toEqual({}); | ||
| }); | ||
|
|
||
| xit("should only refetch once when we store reset", async () => { |
There was a problem hiding this comment.
Could we change this to test.skip while we're at it so we can find skipped tests more easily in the future?
| xit("should only refetch once when we store reset", async () => { | |
| it.skip("should only refetch once when we store reset", async () => { |
There was a problem hiding this comment.
Good catch! I missed this when I went through this file.
| resetStore(queryManager).then(resolve, reject); | ||
|
|
||
| await resetStore(queryManager); | ||
| // Ensure the promise doesn't reject |
There was a problem hiding this comment.
Do we end up with a promise that neither rejects nor resolves here? That would be a pretty critical memory leak 😬
There was a problem hiding this comment.
It kind of looks that way. I'm going to leave this for now, but ultimately I think this test will disappear. Its testing fetchQuery which is a private function 🤦♂️. I'd like to test this from an end user perspective. When that happens I'll refactor this to check it different.
There was a problem hiding this comment.
Ok even worse than I thought. This test is starting the fetch, then removing the query from this.queries in QueryManager, then calling resetStore to make sure the in-flight request doesn't reject.
To be honest, I'm not entirely sure the situation this is trying to check against since end users won't at all be doing this in their own apps. I believe we reject promises if you cancel them in-flight, so I don't know this is an actual real-world situation 🤦♂️
|
|
||
| await expect(stream).toEmitMatchedValue({ data }); | ||
|
|
||
| const conditional = jest.fn(() => []); |
There was a problem hiding this comment.
The name of this test is "also works with a conditional function that returns false",, why does this return []? 😢 (not yourfault, just venting!)
Continuation of #12182