fix: remove ctx.cloud.reset in tests, handle infinite loop in stale results #22483
Conversation
|
Thanks for taking the time to open a PR!
|
| }) | ||
| }, | ||
| invalidateCache () { |
There was a problem hiding this comment.
Not super happy with the naming of this, might change to onInvalidateCache, and centralize the client & server invalidation calls as like ctx.invalidateGraphQLCache().
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||||||||||||||||||||||
| if (toPush.invalidateCache) { | ||
| cache.invalidate({ __typename: 'Query' }) | ||
| continue | ||
| } |
There was a problem hiding this comment.
I'm probably overthinking this, but is there any potential perf advantage to first checking pushFragment for any entries with invalidateCache and, if there are any, filtering out all entries prior to the last one since they'll just get invalidated as we progress through this loop?
Of course there's a real possibility I just don't understand the cache structure here, or maybe there won't ever be enough entries to make this an issue?
There was a problem hiding this comment.
I don't think we'd ever expect to get into a situation where we'd see both here
| * by an additional fetch to the server. This means we've gotten into a bad state | ||
| * and we need to clear both the server & client side cache | ||
| */ | ||
| invalidateCache: () => any |
There was a problem hiding this comment.
Do we ever return anything "actionable" from this function? Should it be void? Or is there a vision that there may be circumstances where this could be chained off of?
There was a problem hiding this comment.
Nah it should be void, will updated
| @@ -52,10 +54,12 @@ describe('CloudDataSource', () => { | |||
| it('returns immediately with { data: null } when no user is defined', () => { | |||
| getUserStub.returns(null) | |||
| const result = cloudDataSource.executeRemoteGraphQL({ | |||
There was a problem hiding this comment.
This feels pretty repetitive, any advantage to pulling out into a reusable beforeEach or function?
There was a problem hiding this comment.
Yeah I guess we could add a little helper fn here, though I'm always wary of doing too much abstracting where this is the actual function being tested, comes at the cost of indirection - it was simple enough to just do a find/replace here
…esser/CLOUD-577-spec-list-display-latest-runs-batching * muaz/CLOUD-577-spec-list-display-latest-runs: fix: Update "Request Access" button state after requesting access (ACI) (#22499) feat: Support "Queued" latest run status (#22497) fix: remove ctx.cloud.reset in tests, handle infinite loop in stale results (#22483) chore: add reporter webpack to gulp watch script (#22386) fix: Increase timeout for npm-webpack-dev-server tests (#22489) fix: Time out unmatched prerequests in proxy to avoid leaking memory (#22462) fix: Sort results in findCrossOriginLogs test helper to deterministic (#22481) fix: memory leak caused by storing base64 encoded files recieved by CDP `Network.requestWillBeSent` (#22460) fix: Improve cross-origin cookie handling (#22320) feat: Add button to clear value from search fields (#22202) chore: Add test to verify settings panels are collapsed by default (#22382) fix: process_profiler follow up work for v10 (#22363) chore: Update Chrome (stable) to 103.0.5060.53 (#22441) refactor: use design system windicss config (#21503) chore: update readme logo (#22433) chore: Update Chrome (beta) to 103.0.5060.53 (#22351) chore: updating version (#22432) Trigger Build
Related to #21250
Additional details
Correctly fixes tests failing due to the introduction of the new queries on the specs page, properly handles an infinite loop that was occurring when an incomplete response was sent back for a
stalefetch result. This would cause an infinite re-fetch loop while the server was looking to fulfill the stale value. Instead, now if we detect this situation we clear the urql cache in both the electron process and the client.Steps to test
Existing tests should be passing with only minor, expected modifications
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?