From 80041d3196aeb68c222a5b098eaacb00ae56526b Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Wed, 1 Nov 2023 16:48:51 -0500 Subject: [PATCH 1/9] fix: handle download from element missing download attribute --- packages/app/src/runner/event-manager.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/app/src/runner/event-manager.ts b/packages/app/src/runner/event-manager.ts index eb02090ad72f..201d46ab056d 100644 --- a/packages/app/src/runner/event-manager.ts +++ b/packages/app/src/runner/event-manager.ts @@ -143,6 +143,7 @@ export class EventManager { Cypress.Cookies.log(data.message, data.cookie, data.removed) break case 'create:download': + Cypress.cy.isStable(true, 'download') Cypress.downloads.start(data) break case 'complete:download': From b7a4415065b41b063e2391939aa7ce88fcd8d071 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Wed, 1 Nov 2023 16:50:01 -0500 Subject: [PATCH 2/9] run ci From 4244eb0aa68ae70b90b46c68e87768a757d4e9de Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 2 Nov 2023 09:49:47 -0500 Subject: [PATCH 3/9] fix: handle page loads when download is triggered #14857 --- cli/CHANGELOG.md | 1 + packages/app/src/runner/event-manager.ts | 4 +- .../cypress/e2e/cypress/downloads.cy.ts | 52 ++++++++++++++++++- .../driver/cypress/fixtures/downloads.html | 15 ++++++ .../cypress/fixtures/downloads_records.csv | 2 + packages/driver/src/cy/commands/navigation.ts | 18 +++++++ packages/driver/src/cypress.ts | 3 ++ packages/driver/src/cypress/downloads.ts | 10 +++- packages/extension/app/v2/background.js | 16 ++++-- packages/server/lib/automation/automation.ts | 1 + packages/server/lib/browsers/chrome.ts | 17 +++--- packages/server/lib/browsers/electron.ts | 12 +++-- 12 files changed, 133 insertions(+), 18 deletions(-) create mode 100644 packages/driver/cypress/fixtures/downloads.html create mode 100644 packages/driver/cypress/fixtures/downloads_records.csv diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 4cb0cd8cf53a..5c04e2e0e359 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,6 +5,7 @@ _Released 11/7/2023 (PENDING)_ **Bugfixes:** +- Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). - Fixed an issue determining visibility when an element is hidden by an ancestor with a shared edge. Fixes [#27514](https://github.com/cypress-io/cypress/issues/27514). ## 13.4.0 diff --git a/packages/app/src/runner/event-manager.ts b/packages/app/src/runner/event-manager.ts index 201d46ab056d..13dca2e60f5a 100644 --- a/packages/app/src/runner/event-manager.ts +++ b/packages/app/src/runner/event-manager.ts @@ -143,12 +143,14 @@ export class EventManager { Cypress.Cookies.log(data.message, data.cookie, data.removed) break case 'create:download': - Cypress.cy.isStable(true, 'download') Cypress.downloads.start(data) break case 'complete:download': Cypress.downloads.end(data) break + case 'canceled:download': + Cypress.downloads.end(data, true) + break default: break } diff --git a/packages/driver/cypress/e2e/cypress/downloads.cy.ts b/packages/driver/cypress/e2e/cypress/downloads.cy.ts index 46e915836a88..46fcf0a64a6d 100644 --- a/packages/driver/cypress/e2e/cypress/downloads.cy.ts +++ b/packages/driver/cypress/e2e/cypress/downloads.cy.ts @@ -4,6 +4,7 @@ describe('src/cypress/downloads', () => { let log let snapshot let end + let error let downloads let downloadItem = { id: '1', @@ -11,13 +12,16 @@ describe('src/cypress/downloads', () => { url: 'http://localhost:1234/location.csv', mime: 'text/csv', } + let action beforeEach(() => { end = cy.stub() - snapshot = cy.stub().returns({ end }) + error = cy.stub() + snapshot = cy.stub().returns({ end, error }) log = cy.stub().returns({ snapshot }) + action = cy.stub() - downloads = $Downloads.create({ log }) + downloads = $Downloads.create({ action, log }) }) context('#start', () => { @@ -51,9 +55,21 @@ describe('src/cypress/downloads', () => { downloads.start(downloadItem) downloads.end({ id: '1' }) + expect(action).to.be.calledWith('app:download:received') + expect(snapshot).to.be.called expect(end).to.be.called }) + it('fails with snapshot if matching log exists', () => { + downloads.start(downloadItem) + downloads.end({ id: '1' }, true) + + expect(action).to.be.calledWith('app:download:received') + expect(snapshot).to.be.called + expect(end).not.to.be.called + expect(error).to.be.called + }) + it('is a noop if matching log does not exist', () => { downloads.end({ id: '1' }) @@ -62,3 +78,35 @@ describe('src/cypress/downloads', () => { }) }) }) + +describe('download behavior', () => { + beforeEach(() => { + cy.visit('/fixtures/downloads.html') + }) + + it('downloads from anchor tag with download attribute', () => { + cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/downloads_records.csv`) + cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`).should('not.exist') + + // trigger download + cy.get('[data-cy=download-csv]').click() + cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`) + .should('contain', '"Joe","Smith"') + }) + + it('downloads from anchor tag without download attribute', () => { + cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/downloads_records.csv`) + cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`).should('not.exist') + + // trigger download + cy.get('[data-cy=download-without-download-attr]').click() + cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`) + .should('contain', '"Joe","Smith"') + }) + + it('invalid download path from anchor tag with download attribute', () => { + // attempt to download + cy.get('[data-cy=invalid-download]').click() + cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_does_not_exist.csv`).should('not.exist') + }) +}) diff --git a/packages/driver/cypress/fixtures/downloads.html b/packages/driver/cypress/fixtures/downloads.html new file mode 100644 index 000000000000..b5042407e355 --- /dev/null +++ b/packages/driver/cypress/fixtures/downloads.html @@ -0,0 +1,15 @@ + + + + + +

Download CSV

+ downloads_records.csv + +

Download CSV

+ download csv from anchor tag without download attr + +

Download CSV

+ broken download link + + diff --git a/packages/driver/cypress/fixtures/downloads_records.csv b/packages/driver/cypress/fixtures/downloads_records.csv new file mode 100644 index 000000000000..54b734d103e6 --- /dev/null +++ b/packages/driver/cypress/fixtures/downloads_records.csv @@ -0,0 +1,2 @@ +"First name","Last name","Occupation","Age","City","State" +"Joe","Smith","student",20,"Boston","MA" \ No newline at end of file diff --git a/packages/driver/src/cy/commands/navigation.ts b/packages/driver/src/cy/commands/navigation.ts index 73e0c66f2daf..1d3ec096832c 100644 --- a/packages/driver/src/cy/commands/navigation.ts +++ b/packages/driver/src/cy/commands/navigation.ts @@ -302,6 +302,23 @@ const stabilityChanged = async (Cypress, state, config, stable) => { debug('waiting for window:load') const promise = new Promise((resolve) => { + const handleDownloadUnloadEvent = () => { + cy.state('onPageLoadErr', null) + cy.isStable(true, 'download') + + options._log + .set({ + message: 'download fired beforeUnload event', + consoleProps () { + return { + Note: 'This event fired when the download was initiated.', + } + }, + }).snapshot().end() + + resolve() + } + const onWindowLoad = ({ url }) => { const href = state('autLocation').href const count = getRedirectionCount(href) @@ -351,6 +368,7 @@ const stabilityChanged = async (Cypress, state, config, stable) => { } } + cy.once('download:received', handleDownloadUnloadEvent) cy.once('internal:window:load', onInternalWindowLoad) // If this request is still pending after the test run, resolve it, no commands were waiting on its result. diff --git a/packages/driver/src/cypress.ts b/packages/driver/src/cypress.ts index 279b8b9ebb14..7e459e56b2ee 100644 --- a/packages/driver/src/cypress.ts +++ b/packages/driver/src/cypress.ts @@ -712,6 +712,9 @@ class $Cypress { case 'app:navigation:changed': return this.emit('navigation:changed', ...args) + case 'app:download:received': + return this.emit('download:received') + case 'app:form:submitted': return this.emit('form:submitted', args[0]) diff --git a/packages/driver/src/cypress/downloads.ts b/packages/driver/src/cypress/downloads.ts index d33c353cee8b..25436855890c 100644 --- a/packages/driver/src/cypress/downloads.ts +++ b/packages/driver/src/cypress/downloads.ts @@ -25,11 +25,17 @@ export default { return log.snapshot() } - const end = ({ id }) => { + const end = ({ id }, isCanceled = false) => { + Cypress.action('app:download:received') + const log = logs[id] if (log) { - log.snapshot().end() + if (isCanceled) { + log.snapshot().error(new Error('Download was canceled.')) + } else { + log.snapshot().end() + } // don't need this anymore since the download has ended // and won't change anymore diff --git a/packages/extension/app/v2/background.js b/packages/extension/app/v2/background.js index e70d5729f6e5..6fe74d24dcf0 100644 --- a/packages/extension/app/v2/background.js +++ b/packages/extension/app/v2/background.js @@ -51,11 +51,19 @@ const connect = function (host, path, extraOpts) { }) browser.downloads.onChanged.addListener((downloadDelta) => { - if ((downloadDelta.state || {}).current !== 'complete') return + const state = (downloadDelta.state || {}).current - ws.emit('automation:push:request', 'complete:download', { - id: `${downloadDelta.id}`, - }) + if (state === 'complete') { + ws.emit('automation:push:request', 'complete:download', { + id: `${downloadDelta.id}`, + }) + } + + if (state === 'canceled') { + ws.emit('automation:push:request', 'canceled:download', { + id: `${downloadDelta.id}`, + }) + } }) }) diff --git a/packages/server/lib/automation/automation.ts b/packages/server/lib/automation/automation.ts index 53d4bbf0f94f..22b2abfaa15f 100644 --- a/packages/server/lib/automation/automation.ts +++ b/packages/server/lib/automation/automation.ts @@ -127,6 +127,7 @@ export class Automation { case 'change:cookie': return this.cookies.changeCookie(data) case 'create:download': + case 'canceled:download': case 'complete:download': return data default: diff --git a/packages/server/lib/browsers/chrome.ts b/packages/server/lib/browsers/chrome.ts index ea6b1e20d705..6119341b4b75 100644 --- a/packages/server/lib/browsers/chrome.ts +++ b/packages/server/lib/browsers/chrome.ts @@ -298,11 +298,17 @@ const _handleDownloads = async function (client, downloadsFolder: string, automa }) client.on('Page.downloadProgress', (data) => { - if (data.state !== 'completed') return + if (data.state === 'completed') { + automation.push('complete:download', { + id: data.guid, + }) + } - automation.push('complete:download', { - id: data.guid, - }) + if (data.state === 'canceled') { + automation.push('canceled:download', { + id: data.guid, + }) + } }) await client.send('Page.setDownloadBehavior', { @@ -521,13 +527,12 @@ export = { await pageCriClient.send('Page.enable') - await utils.handleDownloadLinksViaCDP(pageCriClient, automation) - await options['onInitializeNewBrowserTab']?.() await Promise.all([ options.videoApi && this._recordVideo(cdpAutomation, options.videoApi, Number(options.browser.majorVersion)), this._handleDownloads(pageCriClient, options.downloadsFolder, automation), + utils.handleDownloadLinksViaCDP(pageCriClient, automation), ]) await this._navigateUsingCRI(pageCriClient, url) diff --git a/packages/server/lib/browsers/electron.ts b/packages/server/lib/browsers/electron.ts index 52a7b1e33c73..a26312c0efff 100644 --- a/packages/server/lib/browsers/electron.ts +++ b/packages/server/lib/browsers/electron.ts @@ -337,7 +337,7 @@ export = { }, _handleDownloads (win, dir, automation) { - const onWillDownload = (event, downloadItem) => { + const onWillDownload = (_event, downloadItem) => { const savePath = path.join(dir, downloadItem.getFilename()) automation.push('create:download', { @@ -347,8 +347,14 @@ export = { url: downloadItem.getURL(), }) - downloadItem.once('done', () => { - automation.push('complete:download', { + downloadItem.once('done', (_event, state) => { + if (state === 'completed') { + automation.push('complete:download', { + id: downloadItem.getETag(), + }) + } + + automation.push('canceled:download', { id: downloadItem.getETag(), }) }) From 2dc2fca563571ca29ddc40ec0049b6b20fec7e04 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 2 Nov 2023 09:59:43 -0500 Subject: [PATCH 4/9] additional changelog notes --- cli/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 5c04e2e0e359..21e6cf0a8058 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,7 +5,8 @@ _Released 11/7/2023 (PENDING)_ **Bugfixes:** -- Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). +- Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Note: download behaviors in experimental Webkit are still an issue. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). +- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). - Fixed an issue determining visibility when an element is hidden by an ancestor with a shared edge. Fixes [#27514](https://github.com/cypress-io/cypress/issues/27514). ## 13.4.0 From c6edfe1c8cd7fb20216c0f1459e5cdbe82e1d802 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 2 Nov 2023 10:07:26 -0500 Subject: [PATCH 5/9] this prob fails the changelog check --- cli/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 21e6cf0a8058..ff6178013e01 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -6,7 +6,7 @@ _Released 11/7/2023 (PENDING)_ **Bugfixes:** - Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Note: download behaviors in experimental Webkit are still an issue. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). -- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). +- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixed in [#28222](https://github.com/cypress-io/cypress/issues/28222). - Fixed an issue determining visibility when an element is hidden by an ancestor with a shared edge. Fixes [#27514](https://github.com/cypress-io/cypress/issues/27514). ## 13.4.0 From b04d4b2ee2d96421cc37ef60b9d79a81faca5f7b Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 2 Nov 2023 10:07:49 -0500 Subject: [PATCH 6/9] correct link --- cli/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index ff6178013e01..10966a0d0081 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -6,7 +6,7 @@ _Released 11/7/2023 (PENDING)_ **Bugfixes:** - Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Note: download behaviors in experimental Webkit are still an issue. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857). -- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixed in [#28222](https://github.com/cypress-io/cypress/issues/28222). +- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixed in [#28222](https://github.com/cypress-io/cypress/pull/28222). - Fixed an issue determining visibility when an element is hidden by an ancestor with a shared edge. Fixes [#27514](https://github.com/cypress-io/cypress/issues/27514). ## 13.4.0 From aafc426f31f493da1adfb75a1cebea313c9d5f0d Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 2 Nov 2023 10:38:19 -0500 Subject: [PATCH 7/9] Update packages/server/lib/browsers/electron.ts --- packages/server/lib/browsers/electron.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/lib/browsers/electron.ts b/packages/server/lib/browsers/electron.ts index a26312c0efff..fd4f819b4c8c 100644 --- a/packages/server/lib/browsers/electron.ts +++ b/packages/server/lib/browsers/electron.ts @@ -349,7 +349,7 @@ export = { downloadItem.once('done', (_event, state) => { if (state === 'completed') { - automation.push('complete:download', { + return automation.push('complete:download', { id: downloadItem.getETag(), }) } From f7ada3b7db86d08f4c78894cafba132c5a5fa284 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Fri, 3 Nov 2023 09:59:12 -0500 Subject: [PATCH 8/9] missing unit test coverage --- .../test/integration/background_spec.js | 17 ++++++++++++++ .../server/test/unit/browsers/chrome_spec.js | 15 ++++++++++++ .../test/unit/browsers/electron_spec.js | 23 ++++++++++++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/packages/extension/test/integration/background_spec.js b/packages/extension/test/integration/background_spec.js index b0d4617a275b..1dd316c89198 100644 --- a/packages/extension/test/integration/background_spec.js +++ b/packages/extension/test/integration/background_spec.js @@ -231,6 +231,23 @@ describe('app/background', () => { }) }) + it('onChanged emits automation:push:request canceled:download', async function () { + const downloadDelta = { + id: '1', + state: { + current: 'canceled', + }, + } + + sinon.stub(browser.downloads.onChanged, 'addListener').yields(downloadDelta) + + const ws = await this.connect() + + expect(ws.emit).to.be.calledWith('automation:push:request', 'canceled:download', { + id: `${downloadDelta.id}`, + }) + }) + it('onChanged does not emit if state does not exist', async function () { const downloadDelta = { id: '1', diff --git a/packages/server/test/unit/browsers/chrome_spec.js b/packages/server/test/unit/browsers/chrome_spec.js index 987afc4c8313..20dc11dfe6d7 100644 --- a/packages/server/test/unit/browsers/chrome_spec.js +++ b/packages/server/test/unit/browsers/chrome_spec.js @@ -338,6 +338,21 @@ describe('lib/browsers/chrome', () => { }) }) }) + + it('pushes canceled:download when download is incomplete', function () { + const downloadData = { + guid: '1', + state: 'canceled', + } + const options = { downloadsFolder: 'downloads' } + + return this.onCriEvent('Page.downloadProgress', downloadData, options) + .then(() => { + expect(this.automation.push).to.be.calledWith('canceled:download', { + id: '1', + }) + }) + }) }) describe('adding header to AUT iframe request', function () { diff --git a/packages/server/test/unit/browsers/electron_spec.js b/packages/server/test/unit/browsers/electron_spec.js index 1fe7fc2d5c76..ecb0101d2f16 100644 --- a/packages/server/test/unit/browsers/electron_spec.js +++ b/packages/server/test/unit/browsers/electron_spec.js @@ -322,7 +322,7 @@ describe('lib/browsers/electron', () => { getFilename: () => 'file.csv', getMimeType: () => 'text/csv', getURL: () => 'http://localhost:1234/file.csv', - once: sinon.stub().yields(), + once: sinon.stub().yields({}, 'completed'), } this.win.webContents.session.on.withArgs('will-download').yields({}, downloadItem) @@ -337,6 +337,27 @@ describe('lib/browsers/electron', () => { }) }) + it('pushes canceled:download when download is incomplete', function () { + const downloadItem = { + getETag: () => '1', + getFilename: () => 'file.csv', + getMimeType: () => 'text/csv', + getURL: () => 'http://localhost:1234/file.csv', + once: sinon.stub().yields({}, 'canceled'), + } + + this.win.webContents.session.on.withArgs('will-download').yields({}, downloadItem) + this.options.downloadsFolder = 'downloads' + sinon.stub(this.automation, 'push') + + return electron._launch(this.win, this.url, this.automation, this.options, undefined, undefined, { attachCDPClient: sinon.stub() }) + .then(() => { + expect(this.automation.push).to.be.calledWith('canceled:download', { + id: '1', + }) + }) + }) + it('sets download behavior', function () { this.options.downloadsFolder = 'downloads' From 45e805e1480c4ec4ee10d2876a42cd42a7a85f53 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Fri, 3 Nov 2023 15:00:48 -0500 Subject: [PATCH 9/9] skip test that we know if broke on webkit still --- packages/driver/cypress/e2e/cypress/downloads.cy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/driver/cypress/e2e/cypress/downloads.cy.ts b/packages/driver/cypress/e2e/cypress/downloads.cy.ts index 46fcf0a64a6d..078d5199cade 100644 --- a/packages/driver/cypress/e2e/cypress/downloads.cy.ts +++ b/packages/driver/cypress/e2e/cypress/downloads.cy.ts @@ -94,7 +94,7 @@ describe('download behavior', () => { .should('contain', '"Joe","Smith"') }) - it('downloads from anchor tag without download attribute', () => { + it('downloads from anchor tag without download attribute', { browser: '!webkit' }, () => { cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/downloads_records.csv`) cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`).should('not.exist')