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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions cli/CHANGELOG.md
Expand Up @@ -5,6 +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. 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/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).
- Fixed an issue where network requests made from tabs/windows other than the main Cypress tab would be delayed. Fixes [#28113](https://github.com/cypress-io/cypress/issues/28113).
- Stopped processing CDP events at the end of a spec when Test Isolation is off and Test Replay is enabled. Addressed in [#28213](https://github.com/cypress-io/cypress/pull/28213).
Expand Down
3 changes: 3 additions & 0 deletions packages/app/src/runner/event-manager.ts
Expand Up @@ -148,6 +148,9 @@ export class EventManager {
case 'complete:download':
Cypress.downloads.end(data)
break
case 'canceled:download':
Cypress.downloads.end(data, true)
break
default:
break
}
Expand Down
52 changes: 50 additions & 2 deletions packages/driver/cypress/e2e/cypress/downloads.cy.ts
Expand Up @@ -4,20 +4,24 @@ describe('src/cypress/downloads', () => {
let log
let snapshot
let end
let error
let downloads
let downloadItem = {
id: '1',
filePath: '/path/to/save/location.csv',
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', () => {
Expand Down Expand Up @@ -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' })

Expand All @@ -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', { browser: '!webkit' }, () => {
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')
})
})
15 changes: 15 additions & 0 deletions packages/driver/cypress/fixtures/downloads.html
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<h3>Download CSV</h3>
<a data-cy="download-csv" href="downloads_records.csv" download>downloads_records.csv</a>

<h3>Download CSV</h3>
<a data-cy="download-without-download-attr" href="downloads_records.csv">download csv from anchor tag without download attr</a>

<h3>Download CSV</h3>
<a data-cy="invalid-download" href="downloads_does_not_exist.csv" download>broken download link</a>
</body>
</html>
2 changes: 2 additions & 0 deletions 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"
18 changes: 18 additions & 0 deletions packages/driver/src/cy/commands/navigation.ts
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions packages/driver/src/cypress.ts
Expand Up @@ -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])

Expand Down
10 changes: 8 additions & 2 deletions packages/driver/src/cypress/downloads.ts
Expand Up @@ -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
Expand Down
16 changes: 12 additions & 4 deletions packages/extension/app/v2/background.js
Expand Up @@ -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}`,
})
}
})
})

Expand Down
17 changes: 17 additions & 0 deletions packages/extension/test/integration/background_spec.js
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions packages/server/lib/automation/automation.ts
Expand Up @@ -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:
Expand Down
17 changes: 11 additions & 6 deletions packages/server/lib/browsers/chrome.ts
Expand Up @@ -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', {
Expand Down Expand Up @@ -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)
Expand Down
12 changes: 9 additions & 3 deletions packages/server/lib/browsers/electron.ts
Expand Up @@ -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', {
Expand All @@ -347,8 +347,14 @@ export = {
url: downloadItem.getURL(),
})

downloadItem.once('done', () => {
automation.push('complete:download', {
downloadItem.once('done', (_event, state) => {
if (state === 'completed') {
return automation.push('complete:download', {
id: downloadItem.getETag(),
})
}

automation.push('canceled:download', {
id: downloadItem.getETag(),
})
})
Expand Down
15 changes: 15 additions & 0 deletions packages/server/test/unit/browsers/chrome_spec.js
Expand Up @@ -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 () {
Expand Down
23 changes: 22 additions & 1 deletion packages/server/test/unit/browsers/electron_spec.js
Expand Up @@ -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)
Expand All @@ -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'

Expand Down