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: prevent serialization issues with retries using multi-domain #20628

Merged
merged 9 commits into from
Mar 16, 2022

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Mar 15, 2022

  • Closes N/A

User facing changelog

N/A

Additional details

The goal of this PR is to fix intermittent retry issues with tests that utilize switchToDomain. Previously, we were not preprocessing any values that may/may not be serializable from the primary to the secondary with toSpecBridge functions. This fix processes data before putting it through postMessage, which should guarantee the data is serializable. There are also updates to map nested object properties within objects to literals, as well as Arrays to make sure the structure of an object is still serializable.

A symptom of this is the error Cannot set property message of which has only a getter, which happens when unserializable values are fed through test:before:run:async to the spec bridge, which contains unserializable nested object keys. A data clone error is thrown, which has getter only property message on the base error prototype class, causing this error.

reproduction of error


with PR fixes

To conserve current behavior, we want to only sanitize the data we control. User defined data that is passed into switchToDomain should not be sanitized, and should error as usual.

The only known side effect of this PR is errors passed into Cypress.config are flattened to objects in the secondary, and aren't reified. This seems like an incredibly rare use case, and I am not too worried about this change.

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 Mar 15, 2022

Thanks for taking the time to open a PR!

@AtofStryker AtofStryker changed the title fix: prevent serialization issues with retries using multi-domain by … fix: prevent serialization issues with retries using multi-domain Mar 15, 2022
@cypress
Copy link

cypress bot commented Mar 15, 2022



Test summary

20471 0 298 4Flakiness 2


Run details

Project cypress
Status Passed
Commit cc0a4bd
Started Mar 16, 2022 5:24 PM
Ended Mar 16, 2022 5:38 PM
Duration 14:15 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/actions/click_spec.js Flakiness
1 src/cy/commands/actions/click > #rightclick > rightclick remove el on mouseover
reporter.hooks.spec.js Flakiness
1 hooks > can rerun without timeout error leaking into next run (due to run restart)

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 March 15, 2022 19:47
@AtofStryker AtofStryker requested a review from a team as a code owner March 15, 2022 19:47
return sanitizedValue
// convert any nested structures as well, if objects or arrays, to literals. This is needed in the case of Proxy objects
_.forEach(sanitizedValueAsLiteral as any, (value, key) => {
sanitizedValueAsLiteral[key] = preprocessForSerialization(value)
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 technically only have to do this for Objects/Arrays, but figured this is easier to maintain to do full recursion in which the property is just returned. This can be optimized a bit more if we find this highly important

@AtofStryker AtofStryker removed the request for review from a team March 15, 2022 19:51
@AtofStryker AtofStryker merged commit 193a01a into feature-multidomain Mar 16, 2022
@AtofStryker AtofStryker deleted the fix_multi_domain_retries branch March 16, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants