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: added validation for timeout for query operations #29327

Merged
merged 17 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
## 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_
Expand Down
66 changes: 66 additions & 0 deletions packages/driver/cypress/e2e/commands/querying/querying.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,72 @@ describe('src/cy/commands/querying', () => {
})
})

describe('should throw when timeout is not a number', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests!

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' })

Expand Down
18 changes: 17 additions & 1 deletion packages/driver/src/cy/commands/querying/querying.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import _ from 'lodash'
import _, { isEmpty } from 'lodash'

import $dom from '../../../dom'
import $elements from '../../../dom/elements'
Expand All @@ -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)

Expand Down Expand Up @@ -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)) {
Expand All @@ -149,6 +159,8 @@ export default (Commands, Cypress, cy, state) => {
})
}

validateTimeoutFromOpts(userOptions, 'get')

const log = userOptions._log || Cypress.log({
message: selector,
type: 'parent',
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 12 additions & 0 deletions packages/driver/src/cypress/error_messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down