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

fix: handle download from element missing download attribute #28222

Merged
merged 13 commits into from Nov 6, 2023

Conversation

emilyrohrbough
Copy link
Member

  1. Update to handle downloads from links from elements that do not specify download attribute. Without the download attribute, a beforeUnload event was triggered, causing Cypress to become "unstable" and wait for a page-load event. The download behavior does not fire a load event which caused the hang & timeout.
  • I did sync with @ryanthemanuel who indicate no protocol changes would be necessary for proxy corrollation.
  1. Fix an issue observed while testing were downloading content not found results in a forever pending download event log because we did not fail the event to reflect the canceled or failed download status.

Additional details

These fixes apply to Chrome/Electron/Firefox. This does not fix issues observed in experimental webkit. Downloads in Safari results in a beforeUnload page event, and then Safari navigates & shows the download in the browser but does not trigger navigation events. Further investigation is needed to account for this divergent behavior but time-boxed this due to webkit being experimental.

Firefox: Firefox is an odd-ball. If the download is invalid - our current FF configuration will follow the link and launch a new window (its also configurable to same tab or new tab in the same window). This results in a new Firefox window being opened on the machine with focus, however Cypress continues to executes tests in the background. Quick testing locally shows this did not seem to impact back-to-back spec executions in run mode. 🤷🏻‍♀️

PR Tasks

Comment on lines 350 to 357
downloadItem.once('done', (_event, state) => {
if (state === 'completed') {
automation.push('complete:download', {
id: downloadItem.getETag(),
})
}

automation.push('canceled:download', {
Copy link
Contributor

Choose a reason for hiding this comment

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

If state is completed, should it push both 'complete:download' and 'canceled:download'?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think so - the browser reports these separately. I pulled the canceled state from CDP's states: https://chromedevtools.github.io/devtools-protocol/tot/Browser/#event-downloadProgress

Thoughts on why pushing both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that what it's currently doing? It doesn't return early, so it's going to push twice if state is completed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see what you mean. yes it should return fast

Copy link

cypress bot commented Nov 2, 2023

29 flaky tests on run #52123 ↗︎

0 28278 1346 0 Flakiness 29

Details:

skip test that we know if broke on webkit still
Project: cypress Commit: 45e805e148
Status: Passed Duration: 17:35 💡
Started: Nov 3, 2023 8:11 PM Ended: Nov 3, 2023 8:29 PM
Flakiness  e2e/origin/commands/assertions.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
cy.origin assertions > #consoleProps > .should() and .and() Output
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test Artifacts
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
Flakiness  runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
... > displays each run with correct information Test Replay Output Screenshots
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test Artifacts
... > correctly returns currentRetry Test Replay Output
... > correctly returns currentRetry Test Replay Output
... > correctly returns currentRetry Test Replay Output
Flakiness  specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs Test Replay Output Screenshots

The first 5 flaky specs are shown, see all 15 specs in Cypress Cloud.

Review all test suite changes for PR #28222 ↗︎

@emilyrohrbough emilyrohrbough merged commit eab1730 into develop Nov 6, 2023
80 of 82 checks passed
@emilyrohrbough emilyrohrbough deleted the fix-14857 branch November 6, 2023 20:07
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 8, 2023

Released in 13.5.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.5.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking anchor link to download file causes page load timeout
3 participants