From e18f83088a57652d79aee61ec3e2531c3dfd342f Mon Sep 17 00:00:00 2001 From: Prashant Trar Date: Sun, 14 Apr 2024 12:51:11 +0530 Subject: [PATCH 1/6] fix: added validation for timeout for query operations --- cli/CHANGELOG.md | 7 +++ .../e2e/commands/querying/querying.cy.js | 58 +++++++++++++++++++ .../src/cy/commands/querying/querying.ts | 12 ++++ 3 files changed, 77 insertions(+) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index b9b541bd3bd8..a87fb1f74cbe 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,4 +1,11 @@ +## 13.7.4 +_Released 4/21/2024 (PENDING)_ + +**Bugfixes** + +- Fixed an issue where timeout throw was not happening when user provided invalid timeout value. Fixes [#29323](https://github.com/cypress-io/cypress/issues/29323). + ## 13.7.3 _Released 4/11/2024_ diff --git a/packages/driver/cypress/e2e/commands/querying/querying.cy.js b/packages/driver/cypress/e2e/commands/querying/querying.cy.js index 1b13a61a8bfe..69d856177788 100644 --- a/packages/driver/cypress/e2e/commands/querying/querying.cy.js +++ b/packages/driver/cypress/e2e/commands/querying/querying.cy.js @@ -18,6 +18,64 @@ describe('src/cy/commands/querying', () => { }) }) + describe('should throw when timeout is not a number', () => { + it('timeout passed as plain object {}', (done) => { + cy.get('#some-el', { timeout: {} }) + cy.on('fail', (err) => { + expect(err.message).to.eq('contains invalid timeout - [object Object]') + done() + }) + }) + + it('timeout passed as some string', (done) => { + cy.get('#some-el', { timeout: 'abc' }) + cy.on('fail', (err) => { + expect(err.message).to.eq('contains invalid timeout - abc') + done() + }) + }) + + it('timeout passed as null', (done) => { + cy.get('#some-el', { timeout: null }) + cy.on('fail', (err) => { + expect(err.message).to.eq('contains invalid timeout - null') + done() + }) + }) + + it('timeout passed as NaN', (done) => { + cy.get('#some-el', { timeout: NaN }) + cy.on('fail', (err) => { + expect(err.message).to.eq('contains invalid timeout - NaN') + done() + }) + }) + + it('timeout passed as Boolean', (done) => { + cy.get('#some-el', { timeout: false }) + cy.on('fail', (err) => { + expect(err.message).to.eq('contains invalid timeout - false') + done() + }) + }) + + it('timeout passed as array', (done) => { + cy.get('#some-el', { timeout: [] }) + cy.on('fail', (err) => { + expect(err.message).to.eq(`contains invalid timeout - ${[]}`) + done() + }) + }) + }) + + it('should timeout when element can\'t be found', (done) => { + cy.get('#some-el', { timeout: 100 }) + cy.on('fail', (err) => { + expect(err.message).to.contain('Timed out retrying after 100ms') + done() + }) + }) + it('can increase the timeout', () => { const missingEl = $('
', { id: 'missing-el' }) diff --git a/packages/driver/src/cy/commands/querying/querying.ts b/packages/driver/src/cy/commands/querying/querying.ts index 8e2530610b1f..89931c179b53 100644 --- a/packages/driver/src/cy/commands/querying/querying.ts +++ b/packages/driver/src/cy/commands/querying/querying.ts @@ -142,6 +142,12 @@ function getAlias (selector, log, cy) { } export default (Commands, Cypress, cy, state) => { + function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}) { + if (_.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { + $errUtils.throwErr(new Error(`contains invalid timeout - ${options.timeout}`)) + } + } + Commands.addQuery('get', function get (selector, userOptions: GetOptions = {}) { if ((userOptions === null) || _.isArray(userOptions) || !_.isPlainObject(userOptions)) { $errUtils.throwErrByPath('get.invalid_options', { @@ -149,6 +155,8 @@ export default (Commands, Cypress, cy, state) => { }) } + validateTimeoutFromOpts(userOptions) + const log = userOptions._log || Cypress.log({ message: selector, type: 'parent', @@ -253,6 +261,8 @@ export default (Commands, Cypress, cy, state) => { $errUtils.throwErrByPath('contains.empty_string') } + validateTimeoutFromOpts(userOptions) + // find elements by the :cy-contains pseudo selector // and any submit inputs with the attributeContainsWord selector const selector = $dom.getContainsSelector(text, filter, { matchCase: true, ...userOptions }) @@ -361,6 +371,8 @@ export default (Commands, Cypress, cy, state) => { consoleProps: () => ({}), }) + validateTimeoutFromOpts(userOptions) + this.set('timeout', userOptions.timeout) this.set('onFail', (err) => { switch (err.type) { From d3ee8c022fbdef43acaa7a80219c641af118cff6 Mon Sep 17 00:00:00 2001 From: Prashant Trar Date: Sun, 14 Apr 2024 14:18:11 +0530 Subject: [PATCH 2/6] fix(changelog): added line break --- cli/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index a87fb1f74cbe..f1bc24cf218b 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,8 +1,9 @@ ## 13.7.4 + _Released 4/21/2024 (PENDING)_ -**Bugfixes** +**Bugfixes:** - Fixed an issue where timeout throw was not happening when user provided invalid timeout value. Fixes [#29323](https://github.com/cypress-io/cypress/issues/29323). From 4e9021375e3da860a4ae2e8df2d0458669097faa Mon Sep 17 00:00:00 2001 From: Prashant Trar Date: Mon, 15 Apr 2024 23:56:26 +0530 Subject: [PATCH 3/6] fix(err-msgs): added new error msgs for invalid timeout --- .../e2e/commands/querying/querying.cy.js | 32 ++++++++++++------- .../src/cy/commands/querying/querying.ts | 14 +++++--- packages/driver/src/cypress/error_messages.ts | 12 +++++++ 3 files changed, 41 insertions(+), 17 deletions(-) diff --git a/packages/driver/cypress/e2e/commands/querying/querying.cy.js b/packages/driver/cypress/e2e/commands/querying/querying.cy.js index 69d856177788..6b4c751be2c6 100644 --- a/packages/driver/cypress/e2e/commands/querying/querying.cy.js +++ b/packages/driver/cypress/e2e/commands/querying/querying.cy.js @@ -19,50 +19,58 @@ describe('src/cy/commands/querying', () => { }) describe('should throw when timeout is not a number', () => { + const options = { timeout: {} } + const getErrMsgForTimeout = (timeout) => `\`cy.get()\` only accepts a \`number\` for its \`timeout\` option. You passed: \`${timeout}\`` + it('timeout passed as plain object {}', (done) => { - cy.get('#some-el', { timeout: {} }) + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - [object Object]') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as some string', (done) => { - cy.get('#some-el', { timeout: 'abc' }) + options.timeout = 'abc' + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - abc') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as null', (done) => { - cy.get('#some-el', { timeout: null }) + options.timeout = null + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - null') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as NaN', (done) => { - cy.get('#some-el', { timeout: NaN }) + options.timeout = NaN + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - NaN') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as Boolean', (done) => { - cy.get('#some-el', { timeout: false }) + options.timeout = false + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - false') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as array', (done) => { - cy.get('#some-el', { timeout: [] }) + options.timeout = [] + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq(`contains invalid timeout - ${[]}`) + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) diff --git a/packages/driver/src/cy/commands/querying/querying.ts b/packages/driver/src/cy/commands/querying/querying.ts index 89931c179b53..781f8774efba 100644 --- a/packages/driver/src/cy/commands/querying/querying.ts +++ b/packages/driver/src/cy/commands/querying/querying.ts @@ -1,4 +1,4 @@ -import _ from 'lodash' +import _, { isEmpty } from 'lodash' import $dom from '../../../dom' import $elements from '../../../dom/elements' @@ -16,6 +16,8 @@ type GetOptions = Partial type ShadowOptions = Partial +type QueryCommandOptions = 'get' | 'contains' | 'shadow' | '' + function getAlias (selector, log, cy) { const alias = selector.slice(1) @@ -142,9 +144,11 @@ function getAlias (selector, log, cy) { } export default (Commands, Cypress, cy, state) => { - function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}) { - if (_.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { - $errUtils.throwErr(new Error(`contains invalid timeout - ${options.timeout}`)) + function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}, queryCommand: QueryCommandOptions = '') { + if (!isEmpty(queryCommand) && _.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { + $errUtils.throwErrByPath(`${queryCommand}.invalid_option_timeout`, { + args: { timeout: options.timeout }, + }) } } @@ -155,7 +159,7 @@ export default (Commands, Cypress, cy, state) => { }) } - validateTimeoutFromOpts(userOptions) + validateTimeoutFromOpts(userOptions, 'get') const log = userOptions._log || Cypress.log({ message: selector, diff --git a/packages/driver/src/cypress/error_messages.ts b/packages/driver/src/cypress/error_messages.ts index eafe770c003f..d77a28deaf29 100644 --- a/packages/driver/src/cypress/error_messages.ts +++ b/packages/driver/src/cypress/error_messages.ts @@ -300,6 +300,10 @@ export default { message: `You passed a regular expression with the case-insensitive (_i_) flag and \`{ matchCase: true }\` to ${cmd('contains')}. Those options conflict with each other, so please choose one or the other.`, docsUrl: 'https://on.cypress.io/contains', }, + invalid_option_timeout: { + message: `${cmd('contains')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/contains', + }, }, cookies: { @@ -573,6 +577,10 @@ export default { message: `${cmd('get')} only accepts an options object for its second argument. You passed {{options}}`, docsUrl: 'https://on.cypress.io/get', }, + invalid_option_timeout: { + message: `${cmd('get')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/get', + }, }, getCookie: { @@ -1873,6 +1881,10 @@ export default { message: 'Expected the subject to host a shadow root, but never found it.', docsUrl: 'https://on.cypress.io/shadow', }, + invalid_option_timeout: { + message: `${cmd('shadow')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/shadow', + }, }, should: { From 4b01d7d9727cf4400aa106c78b83583b5dcd73b1 Mon Sep 17 00:00:00 2001 From: Prashant Trar Date: Tue, 16 Apr 2024 00:09:42 +0530 Subject: [PATCH 4/6] fix(err-msgs): added new error msgs for invalid timeout --- .../e2e/commands/querying/querying.cy.js | 32 ++++++++++++------- .../src/cy/commands/querying/querying.ts | 18 +++++++---- packages/driver/src/cypress/error_messages.ts | 12 +++++++ 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/packages/driver/cypress/e2e/commands/querying/querying.cy.js b/packages/driver/cypress/e2e/commands/querying/querying.cy.js index 69d856177788..6b4c751be2c6 100644 --- a/packages/driver/cypress/e2e/commands/querying/querying.cy.js +++ b/packages/driver/cypress/e2e/commands/querying/querying.cy.js @@ -19,50 +19,58 @@ describe('src/cy/commands/querying', () => { }) describe('should throw when timeout is not a number', () => { + const options = { timeout: {} } + const getErrMsgForTimeout = (timeout) => `\`cy.get()\` only accepts a \`number\` for its \`timeout\` option. You passed: \`${timeout}\`` + it('timeout passed as plain object {}', (done) => { - cy.get('#some-el', { timeout: {} }) + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - [object Object]') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as some string', (done) => { - cy.get('#some-el', { timeout: 'abc' }) + options.timeout = 'abc' + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - abc') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as null', (done) => { - cy.get('#some-el', { timeout: null }) + options.timeout = null + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - null') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as NaN', (done) => { - cy.get('#some-el', { timeout: NaN }) + options.timeout = NaN + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - NaN') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as Boolean', (done) => { - cy.get('#some-el', { timeout: false }) + options.timeout = false + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq('contains invalid timeout - false') + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) it('timeout passed as array', (done) => { - cy.get('#some-el', { timeout: [] }) + options.timeout = [] + cy.get('#some-el', options) cy.on('fail', (err) => { - expect(err.message).to.eq(`contains invalid timeout - ${[]}`) + expect(err.message).to.eq(getErrMsgForTimeout(options.timeout)) done() }) }) diff --git a/packages/driver/src/cy/commands/querying/querying.ts b/packages/driver/src/cy/commands/querying/querying.ts index 89931c179b53..092fdfa5f6fe 100644 --- a/packages/driver/src/cy/commands/querying/querying.ts +++ b/packages/driver/src/cy/commands/querying/querying.ts @@ -1,4 +1,4 @@ -import _ from 'lodash' +import _, { isEmpty } from 'lodash' import $dom from '../../../dom' import $elements from '../../../dom/elements' @@ -16,6 +16,8 @@ type GetOptions = Partial type ShadowOptions = Partial +type QueryCommandOptions = 'get' | 'contains' | 'shadow' | '' + function getAlias (selector, log, cy) { const alias = selector.slice(1) @@ -142,9 +144,11 @@ function getAlias (selector, log, cy) { } export default (Commands, Cypress, cy, state) => { - function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}) { - if (_.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { - $errUtils.throwErr(new Error(`contains invalid timeout - ${options.timeout}`)) + function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}, queryCommand: QueryCommandOptions = '') { + if (!isEmpty(queryCommand) && _.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { + $errUtils.throwErrByPath(`${queryCommand}.invalid_option_timeout`, { + args: { timeout: options.timeout }, + }) } } @@ -155,7 +159,7 @@ export default (Commands, Cypress, cy, state) => { }) } - validateTimeoutFromOpts(userOptions) + validateTimeoutFromOpts(userOptions, 'get') const log = userOptions._log || Cypress.log({ message: selector, @@ -261,7 +265,7 @@ export default (Commands, Cypress, cy, state) => { $errUtils.throwErrByPath('contains.empty_string') } - validateTimeoutFromOpts(userOptions) + validateTimeoutFromOpts(userOptions, 'contains') // find elements by the :cy-contains pseudo selector // and any submit inputs with the attributeContainsWord selector @@ -371,7 +375,7 @@ export default (Commands, Cypress, cy, state) => { consoleProps: () => ({}), }) - validateTimeoutFromOpts(userOptions) + validateTimeoutFromOpts(userOptions, 'shadow') this.set('timeout', userOptions.timeout) this.set('onFail', (err) => { diff --git a/packages/driver/src/cypress/error_messages.ts b/packages/driver/src/cypress/error_messages.ts index eafe770c003f..d77a28deaf29 100644 --- a/packages/driver/src/cypress/error_messages.ts +++ b/packages/driver/src/cypress/error_messages.ts @@ -300,6 +300,10 @@ export default { message: `You passed a regular expression with the case-insensitive (_i_) flag and \`{ matchCase: true }\` to ${cmd('contains')}. Those options conflict with each other, so please choose one or the other.`, docsUrl: 'https://on.cypress.io/contains', }, + invalid_option_timeout: { + message: `${cmd('contains')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/contains', + }, }, cookies: { @@ -573,6 +577,10 @@ export default { message: `${cmd('get')} only accepts an options object for its second argument. You passed {{options}}`, docsUrl: 'https://on.cypress.io/get', }, + invalid_option_timeout: { + message: `${cmd('get')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/get', + }, }, getCookie: { @@ -1873,6 +1881,10 @@ export default { message: 'Expected the subject to host a shadow root, but never found it.', docsUrl: 'https://on.cypress.io/shadow', }, + invalid_option_timeout: { + message: `${cmd('shadow')} only accepts a \`number\` for its \`timeout\` option. You passed: \`{{timeout}}\``, + docsUrl: 'https://on.cypress.io/shadow', + }, }, should: { From c0e22f9b9242d69b28435871eb87ad68ce4bd85a Mon Sep 17 00:00:00 2001 From: Jennifer Shehane Date: Mon, 22 Apr 2024 10:56:17 -0400 Subject: [PATCH 5/6] fix changelog --- cli/CHANGELOG.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index c016723f7935..d18f58ea7062 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,4 +1,12 @@ +## 13.8.1 + +_Released 4/23/2024 (PENDING)_ + +**Bugfixes:** + +- Fixed an issue where Cypress would hang on some commands when an invalid `timeout` option was provided. Fixes [#29323](https://github.com/cypress-io/cypress/issues/29323). + ## 13.8.0 _Released 4/18/2024_ @@ -11,10 +19,6 @@ _Released 4/18/2024_ - Fixed a regression introduced in [`13.7.3`](https://docs.cypress.io/guides/references/changelog#13-7-3) where Cypress could hang handling long assertion messages. Fixes [#29350](https://github.com/cypress-io/cypress/issues/29350). -**Bugfixes:** - -- Fixed an issue where Cypress would hang on some commands when an invalid `timeout` option was provided. Fixes [#29323](https://github.com/cypress-io/cypress/issues/29323). - **Misc:** - The [`SEMAPHORE_GIT_PR_NUMBER`](https://docs.semaphoreci.com/ci-cd-environment/environment-variables/#semaphore_git_pr_number) environment variable from [Semaphore](https://semaphoreci.com/) CI is now captured to display the linked PR number in the Cloud. Addressed in [#29314](https://github.com/cypress-io/cypress/pull/29314). From afecc40820d33329a49771b2d3888f9ba72d8f17 Mon Sep 17 00:00:00 2001 From: Prashant Trar Date: Mon, 22 Apr 2024 21:20:54 +0530 Subject: [PATCH 6/6] refactor(validate-timeout): moved func to global querying scope --- .../driver/src/cy/commands/querying/querying.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/driver/src/cy/commands/querying/querying.ts b/packages/driver/src/cy/commands/querying/querying.ts index 092fdfa5f6fe..5416f1084054 100644 --- a/packages/driver/src/cy/commands/querying/querying.ts +++ b/packages/driver/src/cy/commands/querying/querying.ts @@ -143,15 +143,15 @@ function getAlias (selector, log, cy) { } } -export default (Commands, Cypress, cy, state) => { - function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}, queryCommand: QueryCommandOptions = '') { - if (!isEmpty(queryCommand) && _.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { - $errUtils.throwErrByPath(`${queryCommand}.invalid_option_timeout`, { - args: { timeout: options.timeout }, - }) - } +function validateTimeoutFromOpts (options: GetOptions | ContainsOptions | ShadowOptions = {}, queryCommand: QueryCommandOptions = '') { + if (!isEmpty(queryCommand) && _.isPlainObject(options) && options.hasOwnProperty('timeout') && !_.isFinite(options.timeout)) { + $errUtils.throwErrByPath(`${queryCommand}.invalid_option_timeout`, { + args: { timeout: options.timeout }, + }) } +} +export default (Commands, Cypress, cy, state) => { Commands.addQuery('get', function get (selector, userOptions: GetOptions = {}) { if ((userOptions === null) || _.isArray(userOptions) || !_.isPlainObject(userOptions)) { $errUtils.throwErrByPath('get.invalid_options', {