chore: audit cross-origin related TODOs/FIXMEs #20975

chrisbreiding · 2022-04-07T19:57:53Z

I audited all TODOs and FIXMEs added in the feature-multidomain branch and did the following:

Removed it if no longer necessary
Added context where necessary
Added issue links to the comments that remain

- remove if no longer necessary - add context where necessary - add issue links

cypress-bot · 2022-04-07T19:57:54Z

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.

chrisbreiding · 2022-04-07T20:01:25Z

cli/types/cypress.d.ts

-    // TODO: when we find other options to put into the 'data' argument of cy.origin, we may want to overload this type with
-    // a 'data' parameter that contains all data options, including args, and one that contains all data options, excluding args.
-    // This will provide better typings support for whatever args is set to as opposed to an optional undefined


Adding correct typings is part of the acceptance criteria of any feature work, so I felt this comment wasn't needed in the code. I put it in the issue for log: false, since that should be where we add an additional property to options.

chrisbreiding · 2022-04-07T20:02:18Z

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

-// FIXME: sync thrown errors inside of cy.origin cause cross origin errors in firefox when using experimentalSessionSupport
-// This is likely due to cypress being re declared while on a cross origin iframe before the application navigates back.
-// To reproduce, just add "experimentalSessionSupport" = true into the describe block below
-


This seems to be an issue with multiple tests and not just this one, so they'll all needed to be evaluated as part of this issue.

chrisbreiding · 2022-04-07T20:02:46Z

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

-    // TODO: this currently only works on hashchange, but needs work
-    // for other navigation events


This work has been done, so this comment was outdated.

chrisbreiding · 2022-04-07T20:03:59Z

packages/driver/src/multi-domain/events/logs.ts

-    // TODO:
-    // - handle printing console props (need to add to runner)
-    //     this.runner.addLog(args[0], this.config('isInteractive'))
-
    Cypress.specBridgeCommunicator.toPrimary('log:added', LogUtils.getDisplayProps(attrs))
  }

  const onLogChanged = (attrs) => {
-    // TODO:
-    // - add invocation stack if error:
-    //     let parsedError = correctStackForCrossOriginError(log.get('err'), this.userInvocationStack)
-    // - notify runner? maybe not
-    //     this.runner.addLog(args[0], this.config('isInteractive'))
-


This should all be covered in this PR.

chrisbreiding · 2022-04-07T20:04:18Z

packages/driver/src/multi-domain/events/misc.ts

@@ -9,7 +9,6 @@ export const handleMiscEvents = (Cypress: Cypress.Cypress, cy: $Cy) => {
    Cypress.specBridgeCommunicator.toPrimary('viewport:changed', viewport)
  })

-  // TODO: Should state syncing be built into cy.state instead of being explicitly called?


Decided we're not going to do this in a team discussion.

chrisbreiding · 2022-04-07T20:05:12Z

system-tests/test/navigation_spec.ts

@@ -7,8 +7,6 @@ const onServer = function (app) {
  })
 }

-// FIXME: This partially solves https://github.com/cypress-io/cypress/issues/19632 but only when "experimentalLoginFlows" is false.
-// TODO: This will be further solved by https://github.com/cypress-io/cypress/issues/20428
 describe('e2e cross origin navigation', () => {


This was handled in this PR.

cypress · 2022-04-07T20:18:00Z

Test summary

20655 • 0 • 266 • 4 • 0

Run details


Project	cypress
Status	Passed
Commit	`071db97`
Started	Apr 8, 2022 6:36 PM
Ended	Apr 8, 2022 6:53 PM
Duration	16:48 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

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

emilyrohrbough · 2022-04-08T13:30:41Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_cypress_api.spec.ts

-      .closest('.command-wrapper-text')
-      .contains('test log')
+      .then(() => {
+        assertLogLength(logs, 3)


What should the first & the third log be?

cy.origin and (new url)

emilyrohrbough · 2022-04-08T13:31:50Z

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

@@ -190,6 +190,7 @@ describe('navigation events', () => {
    })

    // TODO: this test should re revisited with the cypress in cypress tests available in 10.0


Suggested change

// TODO: this test should re revisited with the cypress in cypress tests available in 10.0

// TODO: this test should revisited with the cypress in cypress tests available in 10.0

Any reason we can't add this test to the packages/runner tests? Those tests are running Cypress in Cypress in 9.x form what I can tell.

The multi-origin stuff doesn't really work with the isolated runner tests in 9.x due to there being another version of the driver in the spec bridge that's not currently taken into account in the isolated runner. It's not really worth adding the support for it in the isolated runner since it's being replaced in 10.0.

emilyrohrbough · 2022-04-08T13:34:25Z

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

+    // HACK: We're telling the runnable that it has passed to avoid a timeout
+    // on the last (empty) command. Normally this would be set inherently by
+    // running runnable.run() the test. Set this to passed regardless of the
+    // state of the test, the runnable isn't responsible for reporting success.


what is responsible here?

The runnable is the test itself, so it's passed or failed by everything in the test passing or something failing, be it a command or assertion or an error being thrown.

The runnable in the secondary driver is mostly stubbed out and its state doesn't influence the actual runnable in the primary driver.

It exists at all in the secondary driver because various parts of the driver rely on it having certain properties and methods. Ideally, it wouldn't need to exist in the secondary driver at all, but removing that would potentially mean a fairly broad refactor of the driver. For the sake of completing the experimental release and reducing the broader impact of the cross-origin support changes, we've not done that as of yet.

We plan to refactor and optimize the code that goes into the secondary driver, so we might be able to refactor out the need for the runnable in the secondary driver as part of that effort.

packages/driver/src/cy/commands/navigation.ts

chore: audit cross-origin related TODOs/FIXMEs

ff624b0

- remove if no longer necessary - add context where necessary - add issue links

chrisbreiding requested review from a team as code owners April 7, 2022 19:57

chrisbreiding requested review from jennifer-shehane, mschile, AtofStryker, mjhenkes and emilyrohrbough and removed request for a team and jennifer-shehane April 7, 2022 19:57

chrisbreiding commented Apr 7, 2022

View reviewed changes

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

Merge branch 'feature-multidomain' into md-todos-fixmes

fda4cbf

emilyrohrbough reviewed Apr 8, 2022

View reviewed changes

emilyrohrbough approved these changes Apr 8, 2022

View reviewed changes

mjhenkes approved these changes Apr 8, 2022

View reviewed changes

mschile requested changes Apr 8, 2022

View reviewed changes

packages/driver/src/cy/commands/navigation.ts Outdated Show resolved Hide resolved

chrisbreiding and others added 3 commits April 8, 2022 14:03

remove comment

3f27d59

Merge branch 'feature-multidomain' into md-todos-fixmes

5efae15

Merge branch 'feature-multidomain' into md-todos-fixmes

071db97

mschile approved these changes Apr 8, 2022

View reviewed changes

chrisbreiding merged commit f6975f8 into feature-multidomain Apr 8, 2022

chrisbreiding deleted the md-todos-fixmes branch April 8, 2022 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: audit cross-origin related TODOs/FIXMEs #20975

chore: audit cross-origin related TODOs/FIXMEs #20975

chrisbreiding commented Apr 7, 2022

cypress-bot bot commented Apr 7, 2022

chrisbreiding Apr 7, 2022 •

edited

chrisbreiding Apr 7, 2022

chrisbreiding Apr 7, 2022

chrisbreiding Apr 7, 2022

chrisbreiding Apr 7, 2022

chrisbreiding Apr 7, 2022

cypress bot commented Apr 7, 2022 •

edited

emilyrohrbough Apr 8, 2022 •

edited

chrisbreiding Apr 8, 2022

emilyrohrbough Apr 8, 2022

emilyrohrbough Apr 8, 2022 •

edited

chrisbreiding Apr 8, 2022

emilyrohrbough Apr 8, 2022

chrisbreiding Apr 8, 2022

		// TODO: this currently only works on hashchange, but needs work
		// for other navigation events

		@@ -190,6 +190,7 @@ describe('navigation events', () => {
		})

		// TODO: this test should re revisited with the cypress in cypress tests available in 10.0

	// TODO: this test should re revisited with the cypress in cypress tests available in 10.0
	// TODO: this test should revisited with the cypress in cypress tests available in 10.0

chore: audit cross-origin related TODOs/FIXMEs #20975

chore: audit cross-origin related TODOs/FIXMEs #20975

Conversation

chrisbreiding commented Apr 7, 2022

cypress-bot bot commented Apr 7, 2022

chrisbreiding Apr 7, 2022 • edited

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

cypress bot commented Apr 7, 2022 • edited

Test summary

Run details

emilyrohrbough Apr 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Apr 8, 2022 • edited

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

chrisbreiding Apr 7, 2022 •

edited

cypress bot commented Apr 7, 2022 •

edited

emilyrohrbough Apr 8, 2022 •

edited

emilyrohrbough Apr 8, 2022 •

edited