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

Cypress doesn't wait for idle timeout #54

Merged
merged 9 commits into from
Jan 9, 2024
Merged

Conversation

skitterm
Copy link
Member

@skitterm skitterm commented Jan 9, 2024

What Changed

Cypress doesn't wait 10 seconds (the global timeout amount) to finish tests anymore.

Instead of using the Watcher.idle() to know when Cypress archives can be complete, I'm not waiting at all. This is because cy.visit() (the command to visit a page) already waits for all resources to be loaded before it completes.

This also means users' Cypress tests aren't any slower when they add Chromatic since we're not waiting a minimum of 500ms after each test.

Also changed

  • Watcher.idle() is pulled out and into the Playwright-specific code since 1) Cypress doesn't need it and 2) it doesn't need access to any Watcher class variables
  • I removed a couple Watcher methods that were either 1) unused (setResponse()) or 2) were only used for logging and weren't actually ever even hit (requestWillBeSent() and responseReceived())

How to test

  • Watch the tests pass below
  • Locally, run Playwright tests
  • Verify that Playwright tests still finish once network is idle
  • Verify that Playwright tests still wait until global timeout (10 seconds) if network isn't idle
  • Locally, run Cypress tests
  • Verify that Cypress tests run quickly (not waiting 10 seconds between tests as they did before).
  • Author QA

Change Type

  • maintenance
  • documentation
  • patch
  • minor
  • major
📦 Published PR as canary version: 0.0.55--canary.54.b107684.0

✨ Test out this PR locally via:

npm install @chromaui/test-archiver@0.0.55--canary.54.b107684.0
# or 
yarn add @chromaui/test-archiver@0.0.55--canary.54.b107684.0

@skitterm skitterm changed the title Steven/cypress network idle Cypress doesn't wait for idle timeout Jan 9, 2024
@skitterm skitterm marked this pull request as ready for review January 9, 2024 17:57
@skitterm skitterm mentioned this pull request Jan 9, 2024
7 tasks
@@ -1,9 +1,6 @@
import type { Page } from 'playwright';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! We just need to get that Playwright protocol out of here now!

// tack on the protocol so we can properly check if requests are cross-origin
this.allowedArchiveOrigins = (allowedDomains || []).map((domain) => `https://${domain}`);
}

async watch() {
this.client.on('Network.requestWillBeSent', this.requestWillBeSent.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see below that the handlers only logged 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, these only logged, and in fact they didn't end up getting fired when I put in the logger.

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.

Looks good!

@skitterm skitterm merged commit 5a4fca4 into main Jan 9, 2024
8 checks passed
@thafryer
Copy link
Member

thafryer commented Jan 9, 2024

🚀 PR was released in v0.0.55 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants