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: unify error serialization with config and other values. Fix err.onFai… #20256

Closed

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Feb 17, 2022

…l issues

User facing changelog

n/a

Additional details

The goal of this PR is to fix the err.onFail is not a function bug we are seeing while writing multi-domain unit tests, as well as unify the error serialization across domains.

err.onFail was failing due to non serializable properties being stringified back to the primary domain, causing the error to fail. Instead of trying to send non serializable values as a serialized string, we now just omit these properties before sending the error (similar to env and config). Now that we process errors in the ran:domain:fn context, we need to also account for scenarios when throw 'oops' or something is thrown in the secondary that may/may not be an error object. This PR should cover this at a basic level.

I would have liked to use the structuredClone algorithm to clone Errors where applicable, but interestingly all properties declared on the error are removed when trying to clone, either with native Chrome implementations or the ponyfill. Once the Error is sent through postMessage, it loses it's prototype hierarchy and ultimately just becomes an object. Therefor, I figured the method we had to pre/post process errors how we do it now is likely preferable.

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 17, 2022

Thanks for taking the time to open a PR!

@AtofStryker AtofStryker changed the title unify error serialization with config and other values. Fix err.onFai… bugfix: unify error serialization with config and other values. Fix err.onFai… Feb 17, 2022
@AtofStryker AtofStryker changed the title bugfix: unify error serialization with config and other values. Fix err.onFai… fix: unify error serialization with config and other values. Fix err.onFai… Feb 17, 2022
@AtofStryker AtofStryker force-pushed the fix-and-refactor-multi-domain-errors branch from 8e97755 to 12b0769 Compare February 17, 2022 23:46
@cypress
Copy link

cypress bot commented Feb 17, 2022



Test summary

19974 0 322 4Flakiness 4


Run details

Project cypress
Status Passed
Commit 12b0769
Started Feb 17, 2022 11:52 PM
Ended Feb 18, 2022 12:05 AM
Duration 12:50 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/net_stubbing_spec.ts Flakiness
1 network stubbing > intercepting response > can 'delay' a proxy response using Promise.delay
2 network stubbing > intercepting response > can 'delay' a proxy response using Promise.delay
3 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
cypress/proxy-logging_spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@AtofStryker AtofStryker marked this pull request as ready for review February 18, 2022 00:16
@AtofStryker AtofStryker requested a review from a team as a code owner February 18, 2022 00:16
@AtofStryker AtofStryker removed the request for review from a team February 18, 2022 00:16
@chrisbreiding chrisbreiding added the topic: cy.origin Problems or enhancements related to cy.origin command label Feb 18, 2022
@@ -132,7 +103,7 @@ export class SpecBridgeDomainCommunicator extends EventEmitter {

try {
// We always want to make sure errors are posted, so clean it up to send.
const preProcessedError = preprocessErrorForPostMessage(err)
const preProcessedError = _.isError(err) ? preprocessErrorForPostMessage(err) : err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This becomes a problem if someone tries to throw something that is unserializable. Do we think this is a valid concern?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is a valid concern. In general i think we should add a basic try-catch around postMessage for any unserializable data, so we can at least send a message with an error.

@mjhenkes
Copy link
Member

It might be good to hold off on this PR until after #20247 lands.

Base automatically changed from feature/sync-config-and-env to feature-multidomain February 22, 2022 17:01
@AtofStryker
Copy link
Contributor Author

going to move this to draft until #20247 is merged in

@AtofStryker AtofStryker marked this pull request as draft February 22, 2022 22:18
@AtofStryker
Copy link
Contributor Author

closing in favor of #20520

@AtofStryker AtofStryker closed this Mar 7, 2022
@AtofStryker AtofStryker deleted the fix-and-refactor-multi-domain-errors branch March 25, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cy.origin Problems or enhancements related to cy.origin command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants