Skip to content
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

Network idle watcher for Cypress #173

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

skitterm
Copy link
Member

@skitterm skitterm commented Jul 5, 2024

Issue: #AP-3843

What Changed

Created a NetworkIdleWatcher class for Cypress that approximates what page.waitForLoadState('networkIdle') does in Playwright.

It's a very simplistic class since we're only using it after the Cypress test has run, to wait longer if there are any pending requests.

Limitations

This class isn't used anywhere yet. In a following PR I'll hook it up so the onRequest and onResponse callbacks are accepted as parameters into the ResourceArchiver() constructor, so that when the CDP detects requests or responses, there's a callback to the NetworkIdleWatcher. Then, when the test is done, we'll call await networkIdleWatcher.idle(), like we do for Playwright.

How to test

Run the unit tests! As in, have CI run them for you 😜

@skitterm skitterm marked this pull request as ready for review July 5, 2024 17:14
@skitterm skitterm requested a review from tevanoff July 5, 2024 17:15
Copy link

codacy-production bot commented Jul 5, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.31% (target: -1.00%) 100.00% (target: 80.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (64f1936) 260 246 94.62%
Head commit (604ef92) 276 (+16) 262 (+16) 94.93% (+0.31%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#173) 16 16 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences


🚀 Don’t miss a bit, follow what’s new on Codacy.

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link
Contributor

@tevanoff tevanoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skitterm I'm curious about your description of this change:

since we're only using it after the Cypress test has run

What do you mean by that? Snapshots happen during a test run, so how would that help ensure that everything is loaded?

// resolve immediately if the in-flight request amount is now zero
if (this.numInFlightRequests === 0) {
clearTimeout(this.idleTimer);
this.exitIdleOnResponse?.();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, I didn't know you could invoke a function using optional chaining like this.

@skitterm
Copy link
Member Author

skitterm commented Jul 5, 2024

What do you mean by that? Snapshots happen during a test run, so how would that help ensure that everything is loaded?

@tevanoff good question. I meant that we're only checking for idleness after the test is run, not during. For the time being, that enabled me to avoid doing a more elaborate idle-watcher (like declaring things idle if more than 200ms has elapsed between requests) and instead stuck with just waiting until all requests have responses (unless the timeout is hit).

@tevanoff
Copy link
Contributor

tevanoff commented Jul 5, 2024

@tevanoff good question. I meant that we're only checking for idleness after the test is run, not during. For the time being, that enabled me to avoid doing a more elaborate idle-watcher (like declaring things idle if more than 200ms has elapsed between requests) and instead stuck with just waiting until all requests have responses (unless the timeout is hit).

@skitterm Does cypress tear down the browser after the test is run? I guess I'm wondering if any requests that are still in flight may just be canceled after the test finishes, leaving us in the same state of not having the assets archived. Is that a possibility?

@skitterm
Copy link
Member Author

skitterm commented Jul 5, 2024

@skitterm Does cypress tear down the browser after the test is run? I guess I'm wondering if any requests that are still in flight may just be canceled after the test finishes, leaving us in the same state of not having the assets archived. Is that a possibility?

@tevanoff that's a fair point. I'm operating on the assumption that the browser is still around when I check idleness (which starts from here), but I'll confirm if that is the case.

Copy link
Contributor

@tevanoff tevanoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the question about the browser tear down, this looks good to me!

@skitterm
Copy link
Member Author

skitterm commented Jul 5, 2024

@tevanoff I checked (by manually delaying an image request in our test server, and manually delaying in our Cypress plugin to stall for the same amount of time), and the image is archived! So we should be able to archive all resources without Cypress cutting off requests.

@skitterm skitterm merged commit da2fca6 into main Jul 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants