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 8 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).

## 13.4.0
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', () => {
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
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') {
automation.push('complete:download', {
emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved
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

id: downloadItem.getETag(),
})
})
Expand Down