fix: handle promises rejected with undefined gracefully #29454
Conversation
AtofStryker
force-pushed
the
fix/undefined_rejection_cross_origin
branch
2 times, most recently
from
8018d1e
to
aa6e8e2
Compare
| 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) => { |
There was a problem hiding this comment.
@AtofStryker If you have a global Cypress.on handler, will it not catch any uncaught:exceptions from origin? Is this documented somewhere?
There was a problem hiding this comment.
I believe it is but I am not sure where, which tells me if it is, it is not obvious.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
added here cypress-io/cypress-documentation#5821
10 flaky tests on run #55309 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| runner/cypress sessions.ui.spec > creates, not recreates, session when validation fails then succeeds |
Test Replay
Screenshots
|
|
project-setup.cy.ts • 1 flaky test • launchpad-e2e
| Test | Artifacts | |
|---|---|---|
| Launchpad: Setup Project > openLink > opens docs link in the default browser |
Test Replay
Screenshots
|
|
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| ... > stops waiting when an fetch request is canceled |
Test Replay
|
|
e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| ... > throws when foo cannot resolve |
Test Replay
|
|
commands/querying/querying.cy.js • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| ... > throws after timing out after a .wait() alias reference |
Test Replay
|
|
The first 5 flaky specs are shown, see all 9 specs in Cypress Cloud.
Review all test suite changes for PR #29454 ↗︎
AtofStryker
force-pushed
the
fix/undefined_rejection_cross_origin
branch
from
aa6e8e2
to
faeacf6
Compare
| @@ -188,23 +188,28 @@ const appendErrMsg = (err, errMsg) => { | |||
| }) | |||
| } | |||
|
|
|||
| const makeErrFromObj = (obj) => { | |||
| const makeErrFromObj = (obj: any) => { | |||
There was a problem hiding this comment.
Would this type be better?
| const makeErrFromObj = (obj: any) => { | |
| const makeErrFromObj = (obj?: string | { message: string, name: string, stack?: string }) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
unknown is also a good suggestion!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Tried to make this a bit more type safe in b294040
AtofStryker
force-pushed
the
fix/undefined_rejection_cross_origin
branch
from
3b61383
to
b294040
Compare
AtofStryker
requested review from
BertrandBordage,
cacieprins,
mschile and
jennifer-shehane
There was a problem hiding this comment.
Thanks for taking time to work on it @AtofStryker!
The code looks good to me!
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
In instances where
Promise.reject(undefined)inside of the cypress spec bridge, errors were not being recreated correctly as the promise was expected to reject with an error instance. This PR solves that by throwing back to cypress with an unknown error in the case the object cannot be recreated.Steps to test
added a driver regression test to show the failed then passing behavior with fix
without fix
with fix
How has the user experience changed?
PR Tasks
cypress-documentation? misc: add documentation for uncaught exceptions for within cy.origin() cypress-documentation#5821type definitions?