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.

Sign up for GitHub

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

Jump to bottom

chore: Refactor multi-domain communication lifecycle #20247

Merged
merged 27 commits into from Feb 23, 2022
Merged

chore: Refactor multi-domain communication lifecycle #20247

merged 27 commits into from Feb 23, 2022

Conversation

chrisbreiding
Copy link
Contributor

@chrisbreiding chrisbreiding commented Feb 17, 2022 •
edited

Refactors the communication lifecycle and how command and logs are handled for multi-domain.

Previously, commands in the secondary domain had proxy versions in the primary domain. A similar mechanism was in place for logs. This meant that they had to be kept in sync and that certain event listeners had to set up and torn down at the right time in order to support them. This led to various timing issues and complicated workarounds. It also required adding a convoluted pausing mechanism to the command queue in the secondary.

Since the command proxies only needed to exist in the primary domain for the purpose of sending messages to the reporter, it's easier to do that directly (more or less) from the the actual command instances in the secondary. The same applies for logs. This simplifies things greatly as the events are simply a stream and can be listened to globally, removing the need to worry about setting them up and tearing them down in sync with the switchToDomain command.

One unfortunate side effect is that this PR removes support for the done callback. It was failing with the refactor, and since we had decided not to support it inside the switchToDomain callback after all, I didn't deem it worthwhile to spend time fixing it. As demonstrated in this PR, promises can be used instead of done callbacks.

This PR also makes some file name and test name changes that aren't strictly necessary, they're just some cleanup I did while implementing the refactor.

PR Tasks

  • Have tests been added/updated?
  • N/A Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • N/A Has a PR for user-facing changes been opened in cypress-documentation?
  • N/A Have API changes been updated in the type definitions?
  • N/A 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!

@cypress
Copy link

cypress bot commented Feb 17, 2022 •
edited


Test summary

20117 • 5 • 278 • 4 • Flakiness 2

Run details
Project cypress
Status Failed
Commit 27b0e21
Started Feb 23, 2022 1:00 AM
Ended Feb 23, 2022 1:13 AM
Duration 13:04 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️

Failures

Run group: 5x-driver-electron (Linux, Electron )
e2e/multi-domain/multi_domain_cypress_api.spec.ts 1 Failed
1 multi-domain Cypress API > methods > log()
Run group: 5x-driver-firefox (Linux, Firefox )
e2e/multi-domain/multi_domain_cypress_api.spec.ts 1 Failed
1 multi-domain Cypress API > methods > log()
Run group: 5x-driver-chrome:beta (Linux, Chrome beta )
e2e/multi-domain/multi_domain_cypress_api.spec.ts 1 Failed
1 multi-domain Cypress API > methods > log()
Run group: 5x-driver-chrome (Linux, Chrome )
e2e/multi-domain/multi_domain_cypress_api.spec.ts 1 Failed
1 multi-domain Cypress API > methods > log()
Run group: 2x-desktop-gui (Linux, Electron )
settings_spec.js 1 Failed
1 Settings > file preference panel > loads preferred editor, available editors and shows spinner

Flakiness

cypress/integration/commands/net_stubbing_spec.ts 2 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

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

@chrisbreiding chrisbreiding added the topic: cy.origin Problems or enhancements related to cy.origin command label Feb 18, 2022
@chrisbreiding chrisbreiding marked this pull request as ready for review February 18, 2022 15:13
@chrisbreiding chrisbreiding requested a review from a team as a code owner February 18, 2022 15:13
chrisbreiding
chrisbreiding commented Feb 18, 2022
View reviewed changes
packages/driver/cypress/integration/e2e/multi-domain/navigation_spec.ts
})
})
.then(reifyLogs)
.then((secondaryLogs) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logs in the secondary no longer have a counterpart in the primary, so these tests needed to change accordingly.

chrisbreiding
chrisbreiding commented Feb 18, 2022
View reviewed changes
packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts
}).invoke('text')
cy.get('[data-cy="dom-check"]')
})
.then((subject) => subject.text())
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 fails quicker than invoke('text') because invoke will retry until the timeout (4000ms) whereas then does not retry.

@chrisbreiding chrisbreiding removed the request for review from a team February 18, 2022 15:34
mschile
mschile approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@mschile mschile left a comment

Choose a reason for hiding this comment

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

Looks good! Glad to see all the complexity removed!

packages/driver/cypress/integration/e2e/multi-domain/commands/multi_domain_actions.spec.ts Outdated Show resolved Hide resolved
packages/runner-shared/src/event-manager.js Show resolved Hide resolved
@mjhenkes mjhenkes mentioned this pull request Feb 18, 2022
Closed
5 tasks
AtofStryker
AtofStryker requested changes Feb 18, 2022
View reviewed changes
packages/driver/cypress/integration/e2e/multi-domain/commands/multi_domain_misc.spec.ts Outdated Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/commands/multi_domain_misc.spec.ts Outdated Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/commands/multi_domain_misc.spec.ts Outdated Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/commands/multi_domain_misc.spec.ts Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/multi_domain_events_spec.ts Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/navigation_spec.ts Outdated Show resolved Hide resolved
packages/driver/cypress/integration/e2e/multi-domain/navigation_spec.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/multi-domain/index.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/multi-domain/util.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/multi-domain/index.ts Show resolved Hide resolved
AtofStryker
AtofStryker approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

🎉

@chrisbreiding chrisbreiding merged commit 0e942ed into feature-multidomain Feb 23, 2022
@chrisbreiding chrisbreiding deleted the md-refactor-comms-lifecycle branch February 23, 2022 15:28
mschile added a commit that referenced this pull request Feb 23, 2022
@mschile
Squashed commit of the following:
96d86c7 
commit fc7149e
Merge: 0e942ed 0143e13
Author: Chris Breiding <chrisbreiding@gmail.com>
Date:   Wed Feb 23 10:28:34 2022 -0500

    Merge branch 'develop' into feature-multidomain

commit 0e942ed
Author: Chris Breiding <chrisbreiding@users.noreply.github.com>
Date:   Wed Feb 23 10:28:03 2022 -0500

    chore: Refactor multi-domain communication lifecycle (#20247)

commit 0143e13
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Mon Feb 21 23:01:35 2022 +0000

    fix(deps): update dependency url-parse to v1.5.6 [security] (#20270)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 1fb16b0
Author: Zach Bloomquist <git@chary.us>
Date:   Sun Feb 20 16:22:08 2022 -0500

    chore: fix cache keys to include PLATFORM (#20279)

    * chore: fix cache keys to include PLATFORM

    * build this branch

    * try using platform_key

commit 65ea8f7
Author: Ryan Manuel <ryanm@cypress.io>
Date:   Fri Feb 18 17:57:34 2022 -0600

    chore: fix server performance flake (#20271)

commit ad2f4de
Author: Zach Bloomquist <git@chary.us>
Date:   Fri Feb 18 18:37:22 2022 -0500

    test(system-tests): support docker-based tests against built binary (#20250)

    Co-authored-by: Ryan Manuel <ryanm@cypress.io>

commit 75c8750
Author: Ryan Manuel <ryanm@cypress.io>
Date:   Fri Feb 18 12:54:26 2022 -0600

    chore: fix system-test-firefox screenshots_spec flake (#20268)

commit 8d28261
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Fri Feb 18 10:33:27 2022 -0600

    chore(deps): update dependency fs-extra to v9 🌟 (#19939)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>
    Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
    Co-authored-by: David Munechika <david@cypress.io>

commit ad50062
Author: Johannes Ewald <mail@johannesewald.de>
Date:   Thu Feb 17 19:21:08 2022 +0100

    fix: Wait for child process to be ready (#19792)

    Co-authored-by: Zach Bloomquist <github@chary.us>
    Co-authored-by: Zach Bloomquist <git@chary.us>

commit 0d3e645
Author: Marco Lauinger <marco.lauinger@gmail.com>
Date:   Thu Feb 17 19:20:33 2022 +0100

    fix: treat form-data bodies as binary (#20144)

commit 42b0fce
Author: Zach Bloomquist <git@chary.us>
Date:   Wed Feb 16 13:53:04 2022 -0500

    test: replace cypress-test-example-repos coverage + remove bump (#20186)

commit e55974c
Merge: f84bac5 ce956de
Author: Blue F <blue@cypress.io>
Date:   Wed Feb 16 09:47:06 2022 -0800

    Merge pull request #20079 from cypress-io/issue-19403-perf-reporter-changes

    chore: Performance reporter changes

commit f84bac5
Author: Ali Kireçligöl <alikirecligol@gmail.com>
Date:   Wed Feb 16 20:22:57 2022 +0300

    fix(driver): update wrapErr to ignore number and boolean values (#20172)

    Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

commit 1e6f51a
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 15 10:18:49 2022 -0800

    release 9.5.0 [skip ci]

commit 507b96f
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Tue Feb 15 06:35:08 2022 -0600

    chore: Update Chrome (stable) to 98.0.4758.102 (#20192)

    Co-authored-by: cypress-bot[bot] <2f0651858c6e38e0+cypress-bot[bot]@users.noreply.github.com>

commit e8d7120
Merge: dff45ca bb26e98
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 14 14:15:27 2022 -0800

    Merge remote-tracking branch 'origin/master' into develop

commit dff45ca
Merge: a3f0d63 2bad703
Author: Blue F <blue@cypress.io>
Date:   Mon Feb 14 12:59:52 2022 -0800

    Merge pull request #20142 from cypress-io/9b967e06f5-master-into-develop

    chore: merge master (9b967e0) into develop

commit 2bad703
Merge: 9b967e0 a3f0d63
Author: Blue F <blue@cypress.io>
Date:   Mon Feb 14 12:26:14 2022 -0800

    Merge branch 'develop' into 9b967e0-master-into-develop

commit ce956de
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 14 08:21:15 2022 -0800

    Add span names, merge develop

commit ac1faf4
Merge: 961e764 2c88f0c
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 14 08:19:17 2022 -0800

    Merge remote-tracking branch 'origin/develop' into issue-19403-perf-reporter-changes

commit 961e764
Author: BlueWinds <blue@everblue.info>
Date:   Thu Feb 10 14:21:44 2022 -0800

    Move sending root event to own script

commit bb26e98
Author: semantic-release-bot <semantic-release-bot@martynus.net>
Date:   Thu Feb 10 15:16:39 2022 -0500

    chore: release @cypress/vue-v3.1.1

    [skip ci]

commit 6a96ca5
Author: semantic-release-bot <semantic-release-bot@martynus.net>
Date:   Thu Feb 10 15:16:23 2022 -0500

    chore: release @cypress/react-v5.12.3

    [skip ci]

commit 9b967e0
Author: Lachlan Miller <lachlan.miller.1990@outlook.com>
Date:   Thu Feb 10 16:26:20 2022 +1000

    fix: set correct default when using react-scripts plugin (#20141)

commit e709184
Merge: e0bf811 97e6c14
Author: Barthélémy Ledoux <bart@cypress.io>
Date:   Wed Feb 9 20:03:05 2022 -0600

    Merge pull request #20132 from cypress-io/elevatebart/trigger-vue-release

commit 97e6c14
Author: ElevateBart <ledouxb@gmail.com>
Date:   Wed Feb 9 17:52:38 2022 -0600

    fix: create a dummy commit to trigger release

commit fa0b68a
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 14:50:04 2022 -0800

    Fix path

commit f7c46fc
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 14:35:00 2022 -0800

    Refactor async data into more convenient helper

commit 57decf4
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 13:21:57 2022 -0800

    Include honeycomb key so we can send root event

commit 3cf4a2b
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 12:55:50 2022 -0800

    Reduce event duplication

commit 9d433c7
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 12:50:24 2022 -0800

    Send root honeycomb event even if node_modules cache already exists

commit 705262f
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 10:48:17 2022 -0800

    Fix

commit b9c0d5e
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 10:37:52 2022 -0800

    fix indentation

commit f694bd2
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 10:33:15 2022 -0800

    Another attempt to wrap all events into one trace

commit 5e96b5c
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 09:52:17 2022 -0800

    Attepmt to associate all circleci jobs for a given build into one trace

commit 980b951
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 09:34:59 2022 -0800

    Tweak honeycomb events further

commit b8f1b4f
Merge: 0d834bc 52ed6ed
Author: BlueWinds <blue@everblue.info>
Date:   Tue Feb 8 09:12:20 2022 -0800

    Merge remote-tracking branch 'origin/develop' into issue-19403-perf-reporter-changes

commit 0d834bc
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 7 12:25:35 2022 -0800

    Add data to all events sent, not just top level

commit ac5bfd3
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 7 11:37:55 2022 -0800

    Use honeycomb tracing

commit 304d8bd
Author: BlueWinds <blue@everblue.info>
Date:   Mon Feb 7 10:22:04 2022 -0800

    Make performance-reporter for system tests use get-next-version script
@AtofStryker AtofStryker mentioned this pull request Mar 8, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschile mschile mschile approved these changes

@mjhenkes mjhenkes mjhenkes approved these changes

@AtofStryker AtofStryker AtofStryker approved these changes

Assignees
No one assigned
Labels
topic: cy.origin Problems or enhancements related to cy.origin command
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chrisbreiding @mjhenkes @mschile @AtofStryker