From e78531820a0a38b9eee9290a490848c6e4e266ea Mon Sep 17 00:00:00 2001 From: Cacie Prins Date: Fri, 8 Mar 2024 11:09:41 -0500 Subject: [PATCH] fix: screenshot() times out when the main Cypress tab is not focused (#29038) * activate main cypress tab before taking a screenshot * new tests to cover page activation behavior * updates changelog * whitespace * fix check-ts * reduce extension failure timeout to 500ms to account for origin bridge timeout * only use tab activation workaround in chrome; default to Page.bringToFront in headless mode * update unit tests * swap order of tests in 5016 system test * some debugging to try and hunt down firefox issue * rm debug prev added - looks like sys test passed that time? * rm debug emit from v2 extension --- cli/CHANGELOG.md | 1 + packages/extension/app/v2/background.js | 1 - .../server/lib/browsers/cdp_automation.ts | 70 +++++++++++++++-- packages/server/lib/browsers/chrome.ts | 2 +- .../test/unit/browsers/cdp_automation_spec.ts | 76 ++++++++++++++++--- system-tests/__snapshots__/issue_5016_spec.js | 74 ++++++++++++++++++ .../cypress.config.js | 6 ++ .../cypress/e2e/issue-5016.cy.js | 18 +++++ .../cypress/fixtures/issue-5016/index.html | 5 ++ .../cypress/fixtures/issue-5016/new.html | 3 + system-tests/test/issue_5016_spec.js | 13 ++++ 11 files changed, 252 insertions(+), 17 deletions(-) create mode 100644 system-tests/__snapshots__/issue_5016_spec.js create mode 100644 system-tests/projects/config-screenshot-on-failure-enabled/cypress.config.js create mode 100644 system-tests/projects/config-screenshot-on-failure-enabled/cypress/e2e/issue-5016.cy.js create mode 100644 system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/index.html create mode 100644 system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/new.html create mode 100644 system-tests/test/issue_5016_spec.js diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 46477195b7c3..009a814edb9b 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -9,6 +9,7 @@ _Released 2/27/2024 (PENDING)_ **Bugfixes:** +- Changed screenshot capture behavior in Chromium to activate the main Cypress tab before capturing. This prevents screenshot capture from timing out in certain situations. Fixed in [#29038](https://github.com/cypress-io/cypress/pull/29038). Fixes [#5016](https://github.com/cypress-io/cypress/issues/5016) - Fixed an issue where `.click()` commands on children of disabled elements would still produce "click" events -- even without `{ force: true }`. Fixes [#28788](https://github.com/cypress-io/cypress/issues/28788). - Changed RequestBody type to allow for boolean and null literals to be passed as body values. [#28789](https://github.com/cypress-io/cypress/issues/28789) diff --git a/packages/extension/app/v2/background.js b/packages/extension/app/v2/background.js index 6fe74d24dcf0..97ae70e24b42 100644 --- a/packages/extension/app/v2/background.js +++ b/packages/extension/app/v2/background.js @@ -337,7 +337,6 @@ const automation = { }) .then(fn) }, - } module.exports = automation diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts index cc13967646cf..873435e59424 100644 --- a/packages/server/lib/browsers/cdp_automation.ts +++ b/packages/server/lib/browsers/cdp_automation.ts @@ -165,10 +165,10 @@ export class CdpAutomation implements CDPClient { on: OnFn off: OffFn send: SendDebuggerCommand - private frameTree: any - private gettingFrameTree: any + private frameTree: Protocol.Page.FrameTree | undefined + private gettingFrameTree: Promise | undefined | null - private constructor (private sendDebuggerCommandFn: SendDebuggerCommand, private onFn: OnFn, private offFn: OffFn, private sendCloseCommandFn: SendCloseCommand, private automation: Automation) { + private constructor (private sendDebuggerCommandFn: SendDebuggerCommand, private onFn: OnFn, private offFn: OffFn, private sendCloseCommandFn: SendCloseCommand, private automation: Automation, private focusTabOnScreenshot: boolean = false, private isHeadless: boolean = false) { onFn('Network.requestWillBeSent', this.onNetworkRequestWillBeSent) onFn('Network.responseReceived', this.onResponseReceived) onFn('Network.requestServedFromCache', this.onRequestServedFromCache) @@ -197,14 +197,62 @@ export class CdpAutomation implements CDPClient { await this.sendDebuggerCommandFn('Page.startScreencast', screencastOpts) } - static async create (sendDebuggerCommandFn: SendDebuggerCommand, onFn: OnFn, offFn: OffFn, sendCloseCommandFn: SendCloseCommand, automation: Automation, protocolManager?: ProtocolManagerShape): Promise { - const cdpAutomation = new CdpAutomation(sendDebuggerCommandFn, onFn, offFn, sendCloseCommandFn, automation) + static async create (sendDebuggerCommandFn: SendDebuggerCommand, onFn: OnFn, offFn: OffFn, sendCloseCommandFn: SendCloseCommand, automation: Automation, protocolManager?: ProtocolManagerShape, focusTabOnScreenshot: boolean = false, isHeadless?: boolean): Promise { + const cdpAutomation = new CdpAutomation(sendDebuggerCommandFn, onFn, offFn, sendCloseCommandFn, automation, focusTabOnScreenshot, isHeadless) await sendDebuggerCommandFn('Network.enable', protocolManager?.networkEnableOptions ?? DEFAULT_NETWORK_ENABLE_OPTIONS) return cdpAutomation } + private async activateMainTab () { + const ActivationTimeoutMessage = 'Unable to communicate with Cypress Extension' + + const sendActivationMessage = ` + (() => { + if (document.defaultView !== top) { return Promise.resolve() } + return new Promise((res) => { + const onMessage = (ev) => { + if (ev.data.message === 'cypress:extension:main:tab:activated') { + window.removeEventListener('message', onMessage) + res() + } + } + + window.addEventListener('message', onMessage) + window.postMessage({ message: 'cypress:extension:activate:main:tab' }) + }) + })()` + + if (this.isHeadless) { + debugVerbose('Headless, so bringing page to front instead of negotiating with extension') + await this.sendDebuggerCommandFn('Page.bringToFront') + } else { + try { + debugVerbose('sending activation message ', sendActivationMessage) + await Promise.race([ + this.sendDebuggerCommandFn('Runtime.evaluate', { + expression: sendActivationMessage, + awaitPromise: true, + }), + new Promise((_, reject) => { + setTimeout(() => reject(new Error(ActivationTimeoutMessage)), 500) + }), + ]) + } catch (e) { + debugVerbose('Error occurred while attempting to activate main tab: ', e) + // If rejected due to timeout, fall back to bringing the main tab to focus - + // this will steal window focus, so it is a last resort. If any other error + // was thrown, re-throw as it was unexpected. + if ((e as Error).message === ActivationTimeoutMessage) { + await this.sendDebuggerCommandFn('Page.bringToFront') + } else { + throw e + } + } + } + } + private onNetworkRequestWillBeSent = async (params: Protocol.Network.RequestWillBeSentEvent) => { debugVerbose('received networkRequestWillBeSent %o', params) @@ -420,7 +468,7 @@ export class CdpAutomation implements CDPClient { client.on('Page.frameDetached', this._updateFrameTree(client, 'Page.frameDetached')) } - onRequest = (message, data) => { + onRequest = async (message, data) => { let setCookie switch (message) { @@ -494,6 +542,16 @@ export class CdpAutomation implements CDPClient { case 'remote:debugger:protocol': return this.sendDebuggerCommandFn(data.command, data.params, data.sessionId) case 'take:screenshot': + debugVerbose('capturing screenshot') + + if (this.focusTabOnScreenshot) { + try { + await this.activateMainTab() + } catch (e) { + debugVerbose('Error while attempting to activate main tab: %O', e) + } + } + return this.sendDebuggerCommandFn('Page.captureScreenshot', { format: 'png' }) .catch((err) => { throw new Error(`The browser responded with an error when Cypress attempted to take a screenshot.\n\nDetails:\n${err.message}`) diff --git a/packages/server/lib/browsers/chrome.ts b/packages/server/lib/browsers/chrome.ts index 3cc6421ba09b..4be0e7eba4cd 100644 --- a/packages/server/lib/browsers/chrome.ts +++ b/packages/server/lib/browsers/chrome.ts @@ -325,7 +325,7 @@ const _handleDownloads = async function (client, downloadsFolder: string, automa let onReconnect: (client: CriClient) => Promise = async () => undefined const _setAutomation = async (client: CriClient, automation: Automation, resetBrowserTargets: (shouldKeepTabOpen: boolean) => Promise, options: BrowserLaunchOpts) => { - const cdpAutomation = await CdpAutomation.create(client.send, client.on, client.off, resetBrowserTargets, automation, options.protocolManager) + const cdpAutomation = await CdpAutomation.create(client.send, client.on, client.off, resetBrowserTargets, automation, options.protocolManager, true, options.isTextTerminal) automation.use(cdpAutomation) diff --git a/packages/server/test/unit/browsers/cdp_automation_spec.ts b/packages/server/test/unit/browsers/cdp_automation_spec.ts index e8e062df62b5..975321ee214b 100644 --- a/packages/server/test/unit/browsers/cdp_automation_spec.ts +++ b/packages/server/test/unit/browsers/cdp_automation_spec.ts @@ -465,20 +465,78 @@ context('lib/browsers/cdp_automation', () => { }) describe('take:screenshot', () => { - it('resolves with base64 data URL', function () { + beforeEach(function () { this.sendDebuggerCommand.withArgs('Browser.getVersion').resolves({ protocolVersion: '1.3' }) - this.sendDebuggerCommand.withArgs('Page.captureScreenshot').resolves({ data: 'foo' }) + }) + + describe('when tab focus behavior default (disabled)', function () { + it('resolves with base64 data URL', function () { + this.sendDebuggerCommand.withArgs('Page.captureScreenshot').resolves({ data: 'foo' }) + + return expect(this.onRequest('take:screenshot')) + .to.eventually.equal('data:image/png;base64,foo') + }) + + it('rejects nicely if Page.captureScreenshot fails', function () { + this.sendDebuggerCommand.withArgs('Page.captureScreenshot').rejects() - return expect(this.onRequest('take:screenshot')) - .to.eventually.equal('data:image/png;base64,foo') + return expect(this.onRequest('take:screenshot')) + .to.be.rejectedWith('The browser responded with an error when Cypress attempted to take a screenshot.') + }) }) - it('rejects nicely if Page.captureScreenshot fails', function () { - this.sendDebuggerCommand.withArgs('Browser.getVersion').resolves({ protocolVersion: '1.3' }) - this.sendDebuggerCommand.withArgs('Page.captureScreenshot').rejects() + describe('when tab focus behavior is enabled', function () { + let requireTabFocus + let isHeadless - return expect(this.onRequest('take:screenshot')) - .to.be.rejectedWith('The browser responded with an error when Cypress attempted to take a screenshot.') + beforeEach(() => { + requireTabFocus = true + }) + + describe('when headless', () => { + beforeEach(() => { + isHeadless = true + }) + + it('does not try to comm with extension, simply brings page to front', async function () { + cdpAutomation = await CdpAutomation.create(this.sendDebuggerCommand, this.onFn, this.offFn, this.sendCloseTargetCommand, this.automation, undefined, requireTabFocus, isHeadless) + this.sendDebuggerCommand.withArgs('Page.captureScreenshot').resolves({ data: 'foo' }) + + expect(cdpAutomation.onRequest('take:screenshot', undefined)).to.eventually.equal('data:image/png;base64,foo') + expect(this.sendDebuggerCommand).not.to.be.calledWith('Runtime.evaluate') + expect(this.sendDebuggerCommand).to.be.calledWith('Page.bringToFront') + }) + }) + + describe('when not headless', () => { + beforeEach(async function () { + isHeadless = false + cdpAutomation = await CdpAutomation.create(this.sendDebuggerCommand, this.onFn, this.offFn, this.sendCloseTargetCommand, this.automation, undefined, requireTabFocus, isHeadless) + this.sendDebuggerCommand.withArgs('Page.captureScreenshot').resolves({ data: 'foo' }) + }) + + describe('and the extension activates the tab', function () { + beforeEach(function () { + this.sendDebuggerCommand.withArgs('Runtime.evaluate').resolves() + this.sendDebuggerCommand.withArgs('Page.captureScreenshot').resolves({ data: 'foo' }) + }) + + it('captures the screenshot', function () { + expect(cdpAutomation.onRequest('take:screenshot', undefined)).to.eventually.equal('data:image/png;base64,foo') + }) + }) + + describe('and the extension fails to activate the tab', function () { + beforeEach(function () { + this.sendDebuggerCommand.withArgs('Runtime.evaluate').rejects(new Error('Unable to communicate with Cypress Extension')) + this.sendDebuggerCommand.withArgs('Page.bringToFront').resolves() + }) + + it('captures the screenshot', function () { + expect(cdpAutomation.onRequest('take:screenshot', undefined)).to.eventually.equal('data:image/png;base64,foo') + }) + }) + }) }) }) diff --git a/system-tests/__snapshots__/issue_5016_spec.js b/system-tests/__snapshots__/issue_5016_spec.js new file mode 100644 index 000000000000..8744f3e0f5e2 --- /dev/null +++ b/system-tests/__snapshots__/issue_5016_spec.js @@ -0,0 +1,74 @@ +exports['e2e issue 5016 - screenshot times out after clicking target _blank / fails but does not timeout taking screenshot'] = ` + +==================================================================================================== + + (Run Starting) + + ┌────────────────────────────────────────────────────────────────────────────────────────────────┐ + │ Cypress: 1.2.3 │ + │ Browser: FooBrowser 88 │ + │ Specs: 1 found (issue-5016.cy.js) │ + │ Searched: cypress/e2e/**/*.cy.{js,jsx,ts,tsx} │ + └────────────────────────────────────────────────────────────────────────────────────────────────┘ + + +──────────────────────────────────────────────────────────────────────────────────────────────────── + + Running: issue-5016.cy.js (1 of 1) + + + issue 5016 + ✓ should take a normal screenshot + 1) should fail but not timeout while taking the screenshot + ✓ should not timeout taking screenshot when not failing + + + 2 passing + 1 failing + + 1) issue 5016 + should fail but not timeout while taking the screenshot: + AssertionError: Timed out retrying after 4050ms: expected '' to have attribute 'foo' + [stack trace lines] + + + + + (Results) + + ┌────────────────────────────────────────────────────────────────────────────────────────────────┐ + │ Tests: 3 │ + │ Passing: 2 │ + │ Failing: 1 │ + │ Pending: 0 │ + │ Skipped: 0 │ + │ Screenshots: 3 │ + │ Video: false │ + │ Duration: X seconds │ + │ Spec Ran: issue-5016.cy.js │ + └────────────────────────────────────────────────────────────────────────────────────────────────┘ + + + (Screenshots) + + - /XXX/XXX/XXX/cypress/screenshots/issue-5016.cy.js/issue 5016 -- should take a no (YxX) + rmal screenshot.png + - /XXX/XXX/XXX/cypress/screenshots/issue-5016.cy.js/issue 5016 -- should fail but (YxX) + not timeout while taking the screenshot (failed).png + - /XXX/XXX/XXX/cypress/screenshots/issue-5016.cy.js/issue 5016 -- should not timeo (YxX) + ut taking screenshot when not failing.png + + +==================================================================================================== + + (Run Finished) + + + Spec Tests Passing Failing Pending Skipped + ┌────────────────────────────────────────────────────────────────────────────────────────────────┐ + │ ✖ issue-5016.cy.js XX:XX 3 2 1 - - │ + └────────────────────────────────────────────────────────────────────────────────────────────────┘ + ✖ 1 of 1 failed (100%) XX:XX 3 2 1 - - + + +` diff --git a/system-tests/projects/config-screenshot-on-failure-enabled/cypress.config.js b/system-tests/projects/config-screenshot-on-failure-enabled/cypress.config.js new file mode 100644 index 000000000000..27dfa92ab932 --- /dev/null +++ b/system-tests/projects/config-screenshot-on-failure-enabled/cypress.config.js @@ -0,0 +1,6 @@ +module.exports = { + screenshotOnRunFailure: true, + e2e: { + supportFile: false, + }, +} diff --git a/system-tests/projects/config-screenshot-on-failure-enabled/cypress/e2e/issue-5016.cy.js b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/e2e/issue-5016.cy.js new file mode 100644 index 000000000000..63d738c2f2aa --- /dev/null +++ b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/e2e/issue-5016.cy.js @@ -0,0 +1,18 @@ +// https://github.com/cypress-io/cypress/issues/5016 +describe('issue 5016', { + screenshotOnRunFailure: true, +}, function () { + it('should take a normal screenshot', function () { + cy.visit('/cypress/fixtures/issue-5016/index.html').screenshot() + }) + + it('should fail but not timeout while taking the screenshot', function () { + cy.visit('/cypress/fixtures/issue-5016/index.html') + cy.get('a').click().should('have.attr', 'foo') + }) + + it('should not timeout taking screenshot when not failing', function () { + cy.visit('/cypress/fixtures/issue-5016/index.html') + cy.get('a').click().screenshot() + }) +}) diff --git a/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/index.html b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/index.html new file mode 100644 index 000000000000..632edd88d001 --- /dev/null +++ b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/index.html @@ -0,0 +1,5 @@ + + + + New Tab + diff --git a/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/new.html b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/new.html new file mode 100644 index 000000000000..04543a93e09a --- /dev/null +++ b/system-tests/projects/config-screenshot-on-failure-enabled/cypress/fixtures/issue-5016/new.html @@ -0,0 +1,3 @@ + + + diff --git a/system-tests/test/issue_5016_spec.js b/system-tests/test/issue_5016_spec.js new file mode 100644 index 000000000000..774f45a2ba75 --- /dev/null +++ b/system-tests/test/issue_5016_spec.js @@ -0,0 +1,13 @@ +const systemTests = require('../lib/system-tests').default + +describe('e2e issue 5016 - screenshot times out after clicking target _blank', function () { + systemTests.setup() + + systemTests.it('fails but does not timeout taking screenshot', { + project: 'config-screenshot-on-failure-enabled', + sanitizeScreenshotDimensions: true, + snapshot: true, + expectedExitCode: 1, + browser: '!webkit', + }) +})