fix: added validation for timeout for query operations (#29327)

* fix: added validation for timeout for query operations * fix(changelog): added line break * fix(err-msgs): added new error msgs for invalid timeout * fix(err-msgs): added new error msgs for invalid timeout * fix changelog * refactor(validate-timeout): moved func to global querying scope --------- Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
cypress-io · Apr 23, 2024 · c5fa260 · c5fa260 · cypress-bot · Apr 23, 2024
1 parent 05ff7df
commit c5fa260
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 2 deletions.
diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md
@@ -5,7 +5,8 @@ _Released 4/23/2024 (PENDING)_
 
 **Bugfixes:**
 
-- Fixes a bug introduced in [`13.6.0`](https://docs.cypress.io/guides/references/changelog#13-6-0) where Cypress would occasionally exit with status code 1, even when a test run was successfully, due to an unhandled WebSocket exception (`Error: WebSocket connection closed`). Addresses [#28523](https://github.com/cypress-io/cypress/issues/28523).
+- Fixed a bug introduced in [`13.6.0`](https://docs.cypress.io/guides/references/changelog#13-6-0) where Cypress would occasionally exit with status code 1, even when a test run was successfully, due to an unhandled WebSocket exception (`Error: WebSocket connection closed`). Addresses [#28523](https://github.com/cypress-io/cypress/issues/28523).
+- 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).
 
 **Dependency Updates:**
 

diff --git a/packages/driver/cypress/e2e/commands/querying/querying.cy.js b/packages/driver/cypress/e2e/commands/querying/querying.cy.js
@@ -18,6 +18,72 @@ 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', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.timeout))
+          done()
+        })
+      })
+
+      it('timeout passed as some string', (done) => {
+        options.timeout = 'abc'
+        cy.get('#some-el', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.timeout))
+          done()
+        })
+      })
+
+      it('timeout passed as null', (done) => {
+        options.timeout = null
+        cy.get('#some-el', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.timeout))
+          done()
+        })
+      })
+
+      it('timeout passed as NaN', (done) => {
+        options.timeout = NaN
+        cy.get('#some-el', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.timeout))
+          done()
+        })
+      })
+
+      it('timeout passed as Boolean', (done) => {
+        options.timeout = false
+        cy.get('#some-el', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.timeout))
+          done()
+        })
+      })
+
+      it('timeout passed as array', (done) => {
+        options.timeout = []
+        cy.get('#some-el', options)
+        cy.on('fail', (err) => {
+          expect(err.message).to.eq(getErrMsgForTimeout(options.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 = $('<div />', { id: 'missing-el' })
 

diff --git 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<Cypress.Loggable & Cypress.Timeoutable & Cypress.Withi
 type ContainsOptions = Partial<Cypress.Loggable & Cypress.Timeoutable & Cypress.CaseMatchable & Cypress.Shadow>
 type ShadowOptions = Partial<Cypress.Loggable & Cypress.Timeoutable>
 
+type QueryCommandOptions = 'get' | 'contains' | 'shadow' | ''
+
 function getAlias (selector, log, cy) {
   const alias = selector.slice(1)
 
@@ -141,6 +143,14 @@ function getAlias (selector, log, cy) {
   }
 }
 
+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)) {
@@ -149,6 +159,8 @@ export default (Commands, Cypress, cy, state) => {
       })
     }
 
+    validateTimeoutFromOpts(userOptions, 'get')
+
     const log = userOptions._log || Cypress.log({
       message: selector,
       type: 'parent',
@@ -253,6 +265,8 @@ export default (Commands, Cypress, cy, state) => {
       $errUtils.throwErrByPath('contains.empty_string')
     }
 
+    validateTimeoutFromOpts(userOptions, 'contains')
+
     // 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 +375,8 @@ export default (Commands, Cypress, cy, state) => {
       consoleProps: () => ({}),
     })
 
+    validateTimeoutFromOpts(userOptions, 'shadow')
+
     this.set('timeout', userOptions.timeout)
     this.set('onFail', (err) => {
       switch (err.type) {

diff --git 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: {