chore: [Multi-domain] Break out separate CI tasks to test the driver with experimentalSessionAndOrigin on #21148

mjhenkes · 2022-04-20T14:09:00Z

Closes

User facing changelog

n/a

Additional details

This pr creates 4 new CI jobs to run driver tests with the experimentalSessionAndOrigin feature flag enabled.

They are creatively named:

driver-integration-tests-firefox-experimentalSessionAndOrigin
driver-integration-tests-chrome-experimentalSessionAndOrigin
driver-integration-tests-chrome-beta-experimentalSessionAndOrigin
driver-integration-tests-electron-experimentalSessionAndOrigin

These jobs only run tests in the 'e2e/multi-domain' folder

The standard jobs have be updated to run all tests not in the 'e2e/multi-domain' folder.

Before

driver-integration-tests-chrome test count was 4748

After

driver-integration-tests-chrome test count is 4419

driver-integration-tests-chrome-experimentalSessionAndOrigin test count is 329

for a total of 4748 (4419 + 329)

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?

…lder

cypress-bot · 2022-04-20T14:09:03Z

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.

mjhenkes · 2022-04-20T14:10:01Z

packages/driver/cypress/integration/commands/navigation_spec.js

@@ -2306,32 +2280,6 @@ describe('src/cy/commands/navigation', () => {
        return null
      })
    })
-
-    it('does not wait for stability at the end of the command queue when not stable with experimentalSessionAndOrigin', (done) => {


test moved to the stability test file

Now it's been combined with the test here, so it flexes the test based on the experimental flag

mjhenkes · 2022-04-20T14:10:27Z

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

+    })
+  })
+
+  context('#page loading', () => {


this test was added from the navigation test file.

mjhenkes · 2022-04-20T14:11:55Z

packages/driver/package.json

@@ -5,7 +5,9 @@
  "scripts": {
    "clean-deps": "rm -rf node_modules",
    "cypress:open": "node ../../scripts/cypress open",
-    "cypress:run": "node ../../scripts/cypress run",
+    "cypress:run": "node ../../scripts/cypress run --spec \"cypress/integration/*/*\",\"cypress/integration/*/!(multi-domain)/**/*\"",


I had to do this in two passes. The first glob includes all test files at the level of the multi-domain folder, the second glob includes all tests files beyond the multi-domain folder but not including it.

I think the serialization unit tests outside the multi-domain directory will run twice once #20949 makes it way in. They should work in both scenarios with the flag on/off, but one of the runs will be redundant. I don't think it's a big deal, but I figure I would surface it here.

mjhenkes · 2022-04-20T14:12:38Z

packages/driver/package.json

-    "cypress:run": "node ../../scripts/cypress run",
+    "cypress:run": "node ../../scripts/cypress run --spec \"cypress/integration/*/*\",\"cypress/integration/*/!(multi-domain)/**/*\"",
+    "cypress:open-experimentalSessionAndOrigin": "node ../../scripts/cypress open --config experimentalSessionAndOrigin=true",
+    "cypress:run-experimentalSessionAndOrigin": "node ../../scripts/cypress run --config experimentalSessionAndOrigin=true  --spec \"cypress/integration/e2e/multi-domain/**/*\"",


the 'multi-domain' folder should probably be renamed, but that is a task for another PR

packages/driver/cypress/integration/commands/navigation_spec.js

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

packages/driver/package.json

Co-authored-by: Matt Schile <mschile@gmail.com>

flotwig

This implementation does not spark joy. We are creating 4 new jobs bringing us to 90 jobs once 10.0-release merges, and there is a hard limit of 100. Also, it's just... not the most elegant solution 😄

@mjhenkes tells me that the target date for x-origin is ~2 months out, so it's not the worst thing to have this temporarily. But if this was for longer-term, I'd prefer to see a neater solution, like using an internal Cypress.backend() to signal to the proxy that cy.origin is enabled for these tests.

@mjhenkes I see that this doesn't revert most of the changes made to the driver-integration test suite in feature-multidomain, will there be a future PR to revert those changes?

cypress · 2022-04-20T16:52:53Z

Test summary

38414 • 0 • 465 • 4 • 1

Run details


Project	cypress
Status	Passed
Commit	`c832235`
Started	Apr 21, 2022 1:02 PM
Ended	Apr 21, 2022 1:19 PM
Duration	16:48 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/integration/cypress/proxy-logging_spec.ts		1
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

I think the e2e cross origin navigation system tests and some of the other cy.origin system needs needs to still be system tests because they may completely fail depending on whether the experimentalSessionAndOrigin is on or off, but we can migrate these tests back into integration tests once we have a solution similar to what @flotwig is talking about?

AtofStryker · 2022-04-20T16:46:56Z

packages/driver/package.json

@@ -5,7 +5,9 @@
  "scripts": {
    "clean-deps": "rm -rf node_modules",
    "cypress:open": "node ../../scripts/cypress open",
-    "cypress:run": "node ../../scripts/cypress run",
+    "cypress:run": "node ../../scripts/cypress run --spec \"cypress/integration/*/*\",\"cypress/integration/*/!(multi-domain)/**/*\"",


I think the serialization unit tests outside the multi-domain directory will run twice once #20949 makes it way in. They should work in both scenarios with the flag on/off, but one of the runs will be redundant. I don't think it's a big deal, but I figure I would surface it here.

emilyrohrbough · 2022-04-20T16:56:34Z

circle.yml

+  driver-integration-tests-firefox-experimentalSessionAndOrigin:
+    <<: *defaults
+    resource_class: medium
+    parallelism: 5


Should we bump this to 6 or 7 since so many tests for multi-domain since we were concerned with longer CI times?

It's hard to get a direct comparison but with current parallelization the CI job seems to run for roughly the same duration as our existing job. So I don't think we need to bump these right now.

mjhenkes · 2022-04-20T17:34:17Z

@flotwig

@mjhenkes I see that this doesn't revert most of the changes made to the driver-integration test suite in feature-multidomain, will there be a future PR to revert those changes?

I was not planning on reverting changes made to the driver-integration test suite that were not addressed in this PR. The changes made are preparing us to be able to enable this feature flag in the future and in general align with our best practice stance of isolating tests from each other. Additionally, with this setup we are proving that the tests will work with the feature flag both on and off.

flotwig · 2022-04-20T18:02:58Z

The changes made are preparing us to be able to enable this feature flag in the future and in general align with our best practice stance of isolating tests from each other

@mjhenkes It does change the nature of a lot of assertions though... I don't really think it's a good idea to change a bunch of unrelated tests in a PR. If we're merging the experimental version of a feature, why would it need to change tests for non-experimental code? We can always cherry-pick the test changes back in, not like we'd lose much.

Also, it was a conscious choice to not follow "best practices" of using cy.visit() in each test for some of the changed tests, a lot of the integration tests are very small and simple and take an unnecessary amount of time to run with a cy.visit() that doesn't add additional coverage. I'm not particularly married to the idea of doing it this way, but there is a reason.

mjhenkes · 2022-04-20T19:36:57Z

@flotwig

It does change the nature of a lot of assertions though

Could you point out some examples of this? The changes to enable sessions should in theory largely be visiting for each test instead of visiting once in a before. If we're changing what we're asserting on, we should give that a deeper look.

It does change the nature of a lot of assertions though... I don't really think it's a good idea to change a bunch of unrelated tests in a PR.

You're right. If we could do it over we would have made these changes in develop then merged them down into our brach. Unfortunately at this point we're running up against a time constraint, and while I don't think it's ideal to have these test changes, I also don't think it should prevent the PR from being merged as we have test case coverage.

If we're merging the experimental version of a feature, why would it need to change tests for non-experimental code? We can always cherry-pick the test changes back in, not like we'd lose much.

We do need these changes, with this PR we are running all driver tests with experimentalSessionAndOrigin enabled as well as a subset (minus the 'multi-domain' folder) with the flag disabled. This gives us excellent coverage and reduces friction to enable the feature flag as any added feature tests will have to pass with the flag in both states. I see this as a feature of this approach.

Also, it was a conscious choice to not follow "best practices" of using cy.visit() in each test for some of the changed tests, a lot of the integration tests are very small and simple and take an unnecessary amount of time to run with a cy.visit() that doesn't add additional coverage. I'm not particularly married to the idea of doing it this way, but there is a reason.

I pass no judgement on either approach, however, once cy.sessions is enabled you will no longer be able to visit once in a before block and run multiple tests against it. This decision was made before my time. If you have strong feelings about this, we should make sure to address them before this feature becomes generally available. I think bringing up these topics is another feature of this approach 😀

I also really really wish github would let me respond to general comments so I didn't have to do all this quoting nonsense. 😞

mjhenkes added 14 commits April 19, 2022 16:07

Attempt to run tests without the feature flag and the multi-domain fo…

c0d942b

…lder

do it for real this time dummy

29cf4b9

trying circleci changes

ca1235a

valid file???

1cfd471

moar changes!

12045e2

i am a master of bash scripting

40e0b12

try another exclude pattern

e5b5734

one more time with feeling

cca9134

lets do it again

922e084

maybe this?

c10a8d3

different parallel group

f05a88a

lets try this

a2556b4

updating tests phase 1

31d73c6

updating tests phase 2

3a0e1e7

mjhenkes added the topic: cy.origin Problems or enhancements related to cy.origin command label Apr 20, 2022

mjhenkes requested review from flotwig, chrisbreiding, mschile, AtofStryker and emilyrohrbough April 20, 2022 14:09

mjhenkes self-assigned this Apr 20, 2022

mjhenkes requested a review from a team as a code owner April 20, 2022 14:09

mjhenkes removed the request for review from a team April 20, 2022 14:09

mjhenkes commented Apr 20, 2022

View reviewed changes

mschile requested changes Apr 20, 2022

View reviewed changes

packages/driver/cypress/integration/commands/navigation_spec.js Outdated Show resolved Hide resolved

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

packages/driver/package.json Outdated Show resolved Hide resolved

Apply suggestions from code review

1407109

Co-authored-by: Matt Schile <mschile@gmail.com>

Update tests

dab4ceb

flotwig approved these changes Apr 20, 2022

View reviewed changes

Run more tests

8930351

AtofStryker approved these changes Apr 20, 2022

View reviewed changes

emilyrohrbough reviewed Apr 20, 2022

View reviewed changes

mschile approved these changes Apr 20, 2022

View reviewed changes

AtofStryker mentioned this pull request Apr 20, 2022

feat: Cross Origin Testing Support #18075

Merged

5 tasks

Re-locate system test

2c98eb1

mjhenkes requested a review from a team as a code owner April 20, 2022 18:18

Flex message for firefox

7990696

Merge branch 'feature-multidomain' into mo-isolate-multi-domain-tests

c832235

chrisbreiding approved these changes Apr 21, 2022

View reviewed changes

mjhenkes removed the request for review from a team April 21, 2022 14:24

emilyrohrbough approved these changes Apr 21, 2022

View reviewed changes

mjhenkes merged commit e3161a0 into feature-multidomain Apr 21, 2022

mjhenkes deleted the mo-isolate-multi-domain-tests branch April 21, 2022 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: [Multi-domain] Break out separate CI tasks to test the driver with experimentalSessionAndOrigin on #21148

chore: [Multi-domain] Break out separate CI tasks to test the driver with experimentalSessionAndOrigin on #21148

mjhenkes commented Apr 20, 2022

cypress-bot bot commented Apr 20, 2022

mjhenkes Apr 20, 2022

mjhenkes Apr 20, 2022

mjhenkes Apr 20, 2022

mjhenkes Apr 20, 2022

AtofStryker Apr 20, 2022

mjhenkes Apr 20, 2022

flotwig left a comment

cypress bot commented Apr 20, 2022 •

edited

AtofStryker left a comment

AtofStryker Apr 20, 2022

emilyrohrbough Apr 20, 2022

mjhenkes Apr 20, 2022

mjhenkes commented Apr 20, 2022

flotwig commented Apr 20, 2022

mjhenkes commented Apr 20, 2022

chore: [Multi-domain] Break out separate CI tasks to test the driver with experimentalSessionAndOrigin on #21148

chore: [Multi-domain] Break out separate CI tasks to test the driver with experimentalSessionAndOrigin on #21148

Conversation

mjhenkes commented Apr 20, 2022

User facing changelog

Additional details

Before

After

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig left a comment

Choose a reason for hiding this comment

cypress bot commented Apr 20, 2022 • edited

Test summary

Run details

Flakiness

AtofStryker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjhenkes commented Apr 20, 2022

flotwig commented Apr 20, 2022

mjhenkes commented Apr 20, 2022

cypress bot commented Apr 20, 2022 •

edited