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 9 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
Original file line number Diff line number Diff line change
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).

## 13.4.0
Expand Down
3 changes: 3 additions & 0 deletions packages/app/src/runner/event-manager.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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', () => {
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
1 change: 1 addition & 0 deletions packages/server/lib/automation/automation.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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