fix: standardize error and object serialization #20520

AtofStryker · 2022-03-07T23:16:15Z

Closes N/A

User facing changelog

n/a

Additional details

After the large refactor changes incorporated in #20247, switchToDomain error stack invocations were temporarily removed until we could safely reincorporate them. This PR addresses re adding those stack invocations, as well as condolidating the error/thrown preprocessing logic with the config preprocessing logic.

In structuredClone, Objects are serialized to literals while omitting prototype hierarchy values as well as omitting get, set, and other properties when cloning (see here).

Errors, even though they are objects, are handled differently depending on the implementation of structuredClone. Firefox cannot serialize Errors through structuredClone and chrome/polyfill implementations can clone Errors, but only keep name, message, and stack. These Error objects are also casted to object literals.

The changes here propose that we preprocess Errors and Objects the same before sending them off to postMessage. Regardless of the context, Errors and Objects now walk to prototype tree and add any serializable value to an object literal. This way, most serializable properties are preserved without having to deal with the caveats of structuredClone. Now that these are processed the same, a single method can be used to preprocess thrown errors as well as sync config/env.

Whenever a value is thrown from the secondary domain, we try to map it to an error. If this value cannot be mapped to an error to present to the user (serializable or otherwise), a CypressError is thrown with the message 'cy.switchToDomain()' could not serialize or map the thrown value. Please make sure the value being thrown is supported by the structured clone algorithm.

A small system test was added to verify the stack trace behavior. This is likely a bit more appropriate for a cy-in-cy test, which we should convert once unification is merged.

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?

other means.

cypress-bot · 2022-03-07T23:16:16Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-03-07T23:25:27Z

Test summary

20334 • 0 • 326 • 4 • 1

Run details


Project	cypress
Status	Passed
Commit	`c339199`
Started	Mar 14, 2022 7:32 PM
Ended	Mar 14, 2022 7:45 PM
Duration	13:01 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/integration/commands/navigation_spec.js		1
1	src/cy/commands/navigation > #visit > window immediately resolves and doesn't reload when visiting the same URL with hashes

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

…rrors

…nto standardize_error_and_object_serialization

AtofStryker · 2022-03-08T22:05:36Z

I am not sure we still need this comment, unless the goal is to get invocation stacks from within the SWD callback?

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_uncaught_errors_spec.ts

packages/driver/src/multi-domain/communicator.ts

chrisbreiding · 2022-03-10T14:36:58Z

packages/driver/src/cy/multi-domain/index.ts

-            return _reject(err)
+        const onQueueFinished = ({ err, subject, unserializableSubjectType, hasMultiDomainError }) => {
+          if (hasMultiDomainError) {
+            return _reject(reifyCrossDomainError(err, userInvocationStack))


Since the switchToDomain command itself is rejected, the userInvocationStack should be attached automatically. That's how it works for every other command. I'm curious why it's not working in some cases.

It looks like the userInvocationStack is only attached when the error is a chai assertion error, but commands errors do not contain userInvocationStack. From what I can see, command errors get the userInvocationStack from current. This works the same for switchToDomain as far as I can tell.

The issue here isn't so much with finding the userInvocationStack when the command is rejected, but with correcting the stack to come from switchToDomain as opposed to the communicator event emitter. We only need the userInvocationStack for this specifically, and once the stack is corrected, the codeFrame can be built properly in the fail handler inside of cy.ts.

without reify

with reify

We could pull user invocation from current, but I feel like there are cases where the switchToDomain CB finishes, then a command fails in the secondary and current has moved on from switchToDomain. That's the only reason it is inside a closure.

Also right now we aren't reifying any cross origin uncaught errors, which I believe fails the current command. Since they are thrown on the spec window, I don't think we have to 😄

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_uncaught_errors_spec.ts

packages/driver/src/util/serialization.ts

… sending unecessary values through postMessage

mjhenkes · 2022-03-11T19:46:16Z

packages/driver/src/cy/multi-domain/index.ts

          reject(err)
        }

        const onQueueFinished = ({ err, subject, unserializableSubjectType }) => {


What would you think of checking for the special object 'err' in the communicator's event listener and reifying the error there for any events that return the err keyword. If it's a concern that someone may return err, we could have a registry of events that this should be applied to... then you would have just one place to apply the reify logic (i have another event with an error i'd like to have this applied to.

https://github.com/cypress-io/cypress/blob/feature-multidomain/packages/driver/src/multi-domain/communicator.ts#L82

I like this idea. Then it mirrors how it's handled on the spec bridge side.

I like this idea, but the only thing I think that might be a problem is setting the correct stack. I am not sure how we can get the correct userInvocationStack in the communicator unless we use current. Any ideas?

We're already using current to the get the userInvocationStack. It's guaranteed to be switchToDomain because we don't resolve switchToDomain until either the secondary queue finishes or there's an error, in which case the queue is stopped for good.

One option is we could pass the userInvocationStack into the spec bridge with run:domain:fn and replace the stack before sending it back, then we don't need to worry about it when reifying the error. This also addresses your question about this comment. I think that may be necessary because the logs bypass switchToDomain and go straight to the runner. Though now that I think about it, I'm not sure if we want the log to have the userInvocationStack, the original stack, or a combination of both. But no need to worry about that for right now. Just pointing out that this solution (passing the userInvocationStack to the spec bridge) may be needed for that use case as well.

OK I'm going to give this a shot. I'll pass userInvocationStack into run:domain:fn to set the invocation stack on the primarytDomainCommunicator and look for a special err key, and if its truthy, reify it.

@chrisbreiding @mjhenkes how about something like bc63ddc?

chrisbreiding

Looks good 👍

…nto standardize_error_and_object_serialization

packages/driver/src/multi-domain/communicator.ts

… with #20564

Refactor error serialization to rely on structuredClone over clone and

7639c8e

other means.

AtofStryker changed the title ~~Standardize error and object serialization~~ fix: standardize error and object serialization Mar 7, 2022

AtofStryker mentioned this pull request Mar 7, 2022

fix: unify error serialization with config and other values. Fix err.onFai… #20256

Closed

5 tasks

add system test to verify stack trace is corrected for multi-domain e…

9506e6b

…rrors

AtofStryker force-pushed the standardize_error_and_object_serialization branch 2 times, most recently from 3df8c6b to daab34b Compare March 8, 2022 20:14

remove redundant code and update documentation/tests

0177037

AtofStryker force-pushed the standardize_error_and_object_serialization branch from daab34b to 0177037 Compare March 8, 2022 20:16

Merge branch 'feature-multidomain' of github.com:cypress-io/cypress i…

4375c78

…nto standardize_error_and_object_serialization

AtofStryker commented Mar 8, 2022

View reviewed changes

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_uncaught_errors_spec.ts Outdated Show resolved Hide resolved

AtofStryker requested review from chrisbreiding, mjhenkes and mschile March 8, 2022 22:43

AtofStryker marked this pull request as ready for review March 8, 2022 22:43

AtofStryker requested a review from a team as a code owner March 8, 2022 22:43

AtofStryker removed the request for review from a team March 8, 2022 22:43

chrisbreiding reviewed Mar 10, 2022

View reviewed changes

AtofStryker requested a review from chrisbreiding March 10, 2022 16:00

mschile requested changes Mar 10, 2022

View reviewed changes

address comments from code review

4872216

AtofStryker requested a review from mschile March 10, 2022 21:01

AtofStryker added 3 commits March 11, 2022 10:58

remove 'to map' from serialization error message

c664865

make all errors truthy before emitting through spec bridge to prevent…

f6c6975

… sending unecessary values through postMessage

fix falsy error serialization in firefox

e575493

mjhenkes reviewed Mar 11, 2022

View reviewed changes

move reify errors into the primaryDomainCommunicator

bc63ddc

chrisbreiding approved these changes Mar 14, 2022

View reviewed changes

Merge branch 'feature-multidomain' of github.com:cypress-io/cypress i…

acab14b

…nto standardize_error_and_object_serialization

AtofStryker requested a review from mjhenkes March 14, 2022 15:02

mschile approved these changes Mar 14, 2022

View reviewed changes

Merge branch 'feature-multidomain' of github.com:cypress-io/cypress i…

b99f1de

…nto standardize_error_and_object_serialization

mjhenkes suggested changes Mar 14, 2022

View reviewed changes

packages/driver/src/multi-domain/communicator.ts Outdated Show resolved Hide resolved

address comment made in review #20520 (comment)

525d3d0

mjhenkes approved these changes Mar 14, 2022

View reviewed changes

update switchToDomain to contain fully qualified URL to be compatible…

c339199

… with #20564

AtofStryker merged commit d1c3e4e into feature-multidomain Mar 14, 2022

AtofStryker deleted the standardize_error_and_object_serialization branch March 14, 2022 20:07

AtofStryker mentioned this pull request Mar 16, 2022

fix: make the primary domain communicator a singleton per cypress instance #20653

Merged

5 tasks

fix: standardize error and object serialization #20520

fix: standardize error and object serialization #20520

Uh oh!

Conversation

AtofStryker commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User facing changelog

Additional details

PR Tasks

Uh oh!

cypress-bot bot commented Mar 7, 2022

Uh oh!

cypress bot commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Flakiness

Uh oh!

AtofStryker commented Mar 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

without reify

with reify

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbreiding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AtofStryker commented Mar 7, 2022 •

edited

Loading

cypress bot commented Mar 7, 2022 •

edited

Loading