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: handle promises rejected with undefined gracefully #29454

Merged
merged 2 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ _Released 5/7/2024 (PENDING)_

**Bugfixes:**

- Fixed a bug where promises rejected with `undefined` were failing inside `cy.origin()`. Addresses [#23937](/~https://github.com/cypress-io/cypress/issues/23937).
- We now pass the same default Chromium flags to Electron as we do to Chrome. As a result of this change, the application under test's `navigator.webdriver` property will now correctly be `true` when testing in Electron. Fixes [#27939](/~https://github.com/cypress-io/cypress/issues/27939).

**Misc:**
Expand Down
15 changes: 15 additions & 0 deletions packages/driver/cypress/e2e/issues/23927.cy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// /~https://github.com/cypress-io/cypress/issues/23927
describe('issue 23927', { browser: '!webkit' }, () => {
it('Fails gracefully if origin page throws undefined', () => {
cy.visit('http://barbaz.com:3500/fixtures/generic.html')
cy.origin('http://foobar.com:3500', () => {
Cypress.on('uncaught:exception', (err, runnable) => {
Copy link
Member

Choose a reason for hiding this comment

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

@AtofStryker If you have a global Cypress.on handler, will it not catch any uncaught:exceptions from origin? Is this documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is but I am not sure where, which tells me if it is, it is not obvious.

Copy link
Member

Choose a reason for hiding this comment

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

@AtofStryker Can you update the docs to include this information here: https://docs.cypress.io/api/cypress-api/catalog-of-events#Uncaught-Exceptions I think this was the main problem people were running into since they just wanted to ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expect(err.message).to.contain('An unknown error has occurred: undefined')

return false
})

cy.visit('http://foobar.com:3500/fixtures/throws-undefined.html')
})
})
})
13 changes: 13 additions & 0 deletions packages/driver/cypress/fixtures/throws-undefined.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>
<html>
<head>
<title>Promise.reject undefined</title>
</head>

<body>
<h1> I am going to reject undefined in the document </h1>
</body>
<script>
Promise.reject(undefined)
</script>
</html>
31 changes: 21 additions & 10 deletions packages/driver/src/cypress/error_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,23 +188,34 @@ const appendErrMsg = (err, errMsg) => {
})
}

const makeErrFromObj = (obj) => {
const makeErrFromObj = (obj: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this type be better?

Suggested change
const makeErrFromObj = (obj: any) => {
const makeErrFromObj = (obj?: string | { message: string, name: string, stack?: string }) => {

Choose a reason for hiding this comment

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

obj? means the type can be undefined and the argument can be unfilled.
In our case it must always be filled, even with undefined.

Another issue: this typing does not match against objects without message or name properties. But like undefined, it is technically valid to pass any object like {} or {something: 123} in throw Error(…). So this function should accept anything, and do case by case type handling with a generic fallback.

Maybe use unknown instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this case we might not be sure what a promise can reject with, so I think any is the appropriate type. But we might want to improve the type checking to make sure message, name, and optional stack exist when trying to reconstruct. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unknown is also a good suggestion!

Choose a reason for hiding this comment

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

Can we really improve the type checking, since we have to accept anything?
Maybe you meant the return type, but that would be Error, which provides name, message and optional stack properties in TypeScript:
/~https://github.com/microsoft/TypeScript/blob/7a38980a7e04bee0e273163f26c91d5c99f28298/src/lib/es5.d.ts#L1057

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can improve it slightly where we either expect a string, or an object with the name, message, and optional stack props. If we don't know what it is, we can still throw with the unknown error we have defined since it at least lets end users handle the exception Cypress side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to make this a bit more type safe in b294040

if (_.isString(obj)) {
return new Error(obj)
}

const err2 = new Error(obj.message)
if (_.isObject(obj) && _.isString((obj as any).message) && _.isString((obj as any).name)) {
obj = obj as {
message: string
name: string
stack?: string
}

err2.name = obj.name
err2.stack = obj.stack
const err2 = new Error(obj.message)

_.each(obj, (val, prop) => {
if (!err2[prop]) {
err2[prop] = val
}
})
err2.name = (obj as any).name
err2.stack = (obj as any).stack

_.each(obj, (val, prop) => {
if (!err2[prop]) {
err2[prop] = val
}
})

return err2
}

return err2
// handle all other errors gracefully (e.g. a promise is rejected with undefined)
return new Error(`An unknown error has occurred: ${obj}`)
}

const makeErrFromErr = (err, options: any = {}) => {
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -14334,10 +14334,10 @@ ee-first@1.1.1:
resolved "https://registry.yarnpkg.com/ee-first/-/ee-first-1.1.1.tgz#590c61156b0ae2f4f0255732a158b266bc56b21d"
integrity sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=

ejs@^3.1.7, ejs@^3.1.8:
version "3.1.9"
resolved "https://registry.yarnpkg.com/ejs/-/ejs-3.1.9.tgz#03c9e8777fe12686a9effcef22303ca3d8eeb361"
integrity sha512-rC+QVNMJWv+MtPgkt0y+0rVEIdbtxVADApW9JXrUVlzHetgcyczP/E7DJmWJ4fJCZF2cPcBk0laWO9ZHMG3DmQ==
ejs@^3.1.10, ejs@^3.1.7:
version "3.1.10"
resolved "https://registry.yarnpkg.com/ejs/-/ejs-3.1.10.tgz#69ab8358b14e896f80cc39e62087b88500c3ac3b"
integrity sha512-UeJmFfOrAQS8OJWPZ4qtgHyWExa088/MtK5UEyoJGFH67cDEXkZSviOiKRCZ4Xij0zxI3JECgYs3oKx+AizQBA==
dependencies:
jake "^10.8.5"

Expand Down
Loading