fix: added validation for timeout for query operations #29327
Conversation
|
|
|
percytrar
changed the title
There was a problem hiding this comment.
@percytrar Thanks for the contribution. I have some suggestions on the error message.
| @@ -142,13 +142,21 @@ 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}`)) | |||
There was a problem hiding this comment.
Typically we keep error messages within our error_messages.ts file and call into that to throw the error message (for simplicity). You can see an example of this in line 153 of this same file. Could you refactor this to put the message there?
For the error message, to match other error messages to give the user all the info they need to fix it, I would suggest:
{{cmd}} only accepts a number for its `timeout` option. You passed: \`{{timeout}}\`
So that for the user it will print:
`cy.get` only accepts a number for its `timeout` option. You passed: `abc`
There was a problem hiding this comment.
Thanks for the info, added new error msgs 👍
There was a problem hiding this comment.
This is addressed.
| @@ -18,6 +18,64 @@ describe('src/cy/commands/querying', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('should throw when timeout is not a number', () => { | |||
There was a problem hiding this comment.
Thanks for adding tests!
|
After updating error messages - |
|
How much time does it take for these runs to execute? Or they are run in batch at a particular time of the day? |
|
@percytrar We have to manually approve all contributor PRs to run to ensure there's nothing nefarious about to run in our CI machines unfortunately, so it just sits there until we push a button. |
|
@jennifer-shehane By when can we expect that this PR will be merged? We need 2 approvals. |
|
Just a small performance-related note, otherwise looks good! |
|
@percytrar I'll try to get this merged for the next release. |
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
When an invalid
timeoutvalue like{}is passed inoptionsto a query method likegetit resulted in infinite waiting, the test simply didn't timed out.Affects query methods -
get,shadowandcontainsSteps to test
Add this to the
querying.cy.jstest fileHow has the user experience changed?
Before fix -
After fix -
PR Tasks
cypress-documentation?type definitions?