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

Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
cypress-io · Apr 8, 2022 · f6975f8 · f6975f8 · cypress-bot · Apr 8, 2022
1 parent a7caf8a
commit f6975f8
Show file tree

Hide file tree

Showing 15 changed files with 34 additions and 45 deletions.
diff --git a/cli/types/cypress.d.ts b/cli/types/cypress.d.ts
@@ -1908,9 +1908,6 @@ declare namespace Cypress {
  */
  origin(urlOrDomain: string, fn: () => void): Chainable
 
- // 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
  /**
  * Enables running Cypress commands in a secondary domain
  * @see https://on.cypress.io/origin

diff --git a/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_cypress_api.spec.ts b/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_cypress_api.spec.ts
@@ -1,3 +1,5 @@
+const { assertLogLength } = require('../../../support/utils')
+
 describe('cy.origin Cypress API', () => {
  beforeEach(() => {
  cy.visit('/fixtures/multi-domain.html')
@@ -189,28 +191,24 @@ describe('cy.origin Cypress API', () => {
  })
  })
 
- // FIXME: convert to cypress-in-cypress tests once possible
  it('log()', () => {
+ const logs: Cypress.Log[] = []
+
+ cy.on('log:added', (attrs, log: Cypress.Log) => {
+ logs.push(log)
+ })
+
  cy.origin('http://foobar.com:3500', () => {
  Cypress.log({
+ name: 'log',
  message: 'test log',
  })
  })
-
- // logs in the secondary origin, skips the primary driver, going through
- // the runner to the reporter, so we have to break out of the AUT and
- // test the actual command log.
- // this is a bit convoluted since otherwise the test could falsely pass
- // by finding its own log if we simply did `.contains('test log')`
- cy.wrap(Cypress.$(window.top!.document.body))
- .find('.reporter')
- .contains('.runnable-title', 'log()')
- .closest('.runnable')
- .find('.runnable-commands-region .hook-item')
- .eq(1)
- .contains('.command-number', '2')
- .closest('.command-wrapper-text')
- .contains('test log')
+ .then(() => {
+ assertLogLength(logs, 3)
+ expect(logs[1].get('name')).to.equal('log')
+ expect(logs[1].get('message')).to.equal('test log')
+ })
  })
  })
 

diff --git a/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts b/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts
@@ -152,7 +152,9 @@ describe('cy.origin', () => {
  })
 
  describe('errors', () => {
- // TODO: Proper stack trace printing still needs to be addressed here
+ // TODO: Proper stack trace printing still needs to be addressed here
+ // with a cy-in-cy test
+ // https://github.com/cypress-io/cypress/issues/20973
  it('propagates secondary origin errors to the primary that occur within the test', (done) => {
  cy.on('fail', (err) => {
  expect(err.message).to.include('variable is not defined')

diff --git a/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_uncaught_errors_spec.ts b/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_uncaught_errors_spec.ts
@@ -1,7 +1,3 @@
-// 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
-
 describe('cy.origin - uncaught errors', () => {
  beforeEach(() => {
  cy.visit('/fixtures/multi-domain.html')
@@ -32,6 +28,7 @@ describe('cy.origin - uncaught errors', () => {
  // TODO: we likely need to change the messaging around this error to make it clear to cy.origin users that
  // this behavior is configurable with 'uncaught:exception', but it MUST be declared inside the cy.origin callback
  // and that 'uncaught:exception' will NOT be triggered outside that callback (inside the primary origin)
+ // https://github.com/cypress-io/cypress/issues/20969
  expect(err.name).to.eq('Error')
  expect(err.message).to.include('sync error')
  expect(err.message).to.include('The following error originated from your application code, not from Cypress.')

diff --git a/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_validation_specs.ts b/packages/driver/cypress/integration/e2e/multi-domain/multi_domain_validation_specs.ts
@@ -21,6 +21,7 @@ describe('cy.origin', () => {
  })
 
  // TODO: $Location does not support ipv6
+ // https://github.com/cypress-io/cypress/issues/20970
  it.skip('succeeds on an ipv6 address', () => {
  cy.origin('0000:0000:0000:0000:0000:0000:0000:0001', () => undefined)
  cy.then(() => {

diff --git a/packages/driver/cypress/integration/e2e/multi-domain/navigation_spec.ts b/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
+ // https://github.com/cypress-io/cypress/issues/20973
  it.skip('the runner url updates appropriately', () => {
  cy.origin('http://foobar.com:3500', () => {
  cy.get('a[data-cy="cross-origin-page"]').click()

diff --git a/packages/driver/src/cy/commands/navigation.ts b/packages/driver/src/cy/commands/navigation.ts
@@ -1129,7 +1129,6 @@ export default (Commands, Cypress, cy, state, config) => {
  const existingAutOrigin = $Location.create(win.location.href).origin
  const params = { remote, existing, previousUrlVisited: existingAutOrigin, log: options._log, isCrossOriginSpecBridge: true }
 
- // TODO: need a better error message
  return cannotVisitDifferentOrigin(params)
  }
 

diff --git a/packages/driver/src/cypress/cy.ts b/packages/driver/src/cypress/cy.ts
@@ -566,7 +566,9 @@ export class $Cy extends EventEmitter2 implements ITimeouts, IStability, IAssert
  // we failed setting the remote window props which
  // means the page navigated to a different origin
 
- // With cross origin support this is an expected error that may or may not be bad, we will rely on the page load timeout to throw if we don't end up where we expect to be.
+ // With cross-origin support, this is an expected error that may or may
+ // not be bad, we will rely on the page load timeout to throw if we
+ // don't end up where we expect to be.
  if (this.config('experimentalLoginFlows') && err.name === 'SecurityError') {
  return
  }

diff --git a/packages/driver/src/multi-domain/cypress.ts b/packages/driver/src/multi-domain/cypress.ts
@@ -104,6 +104,7 @@ const onBeforeAppWindowLoad = (Cypress: Cypress.Cypress, cy: $Cy) => (autWindow:
  }
 
  // TODO: DRY this up with the mostly-the-same code in src/cypress/cy.js
+ // https://github.com/cypress-io/cypress/issues/20972
  bindToListeners(autWindow, {
  onError: handleErrorEvent(cy, 'app'),
  onHistoryNav () {},
@@ -149,8 +150,6 @@ const onBeforeAppWindowLoad = (Cypress: Cypress.Cypress, cy: $Cy) => (autWindow:
 
  return Cypress.action('app:window:unload', e)
  },
- // TODO: this currently only works on hashchange, but needs work
- // for other navigation events
  onNavigation (...args) {
  return Cypress.action('app:navigation:changed', ...args)
  },

diff --git a/packages/driver/src/multi-domain/domain_fn.ts b/packages/driver/src/multi-domain/domain_fn.ts
@@ -77,8 +77,10 @@ export const handleOriginFn = (Cypress: Cypress.Cypress, cy: $Cy) => {
  }
 
  const setRunnableStateToPassed = () => {
- // TODO: 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.
+ // 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.
  cy.state('runnable').state = 'passed'
  }
 

diff --git a/packages/driver/src/multi-domain/events/logs.ts b/packages/driver/src/multi-domain/events/logs.ts
@@ -2,20 +2,10 @@ import { LogUtils } from '../../cypress/log'
 
 export const handleLogs = (Cypress: Cypress.Cypress) => {
  const onLogAdded = (attrs) => {
- // 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'))
-
  Cypress.specBridgeCommunicator.toPrimary('log:changed', LogUtils.getDisplayProps(attrs))
  }
 

diff --git a/packages/driver/src/multi-domain/events/misc.ts b/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?
  Cypress.specBridgeCommunicator.on('sync:state', (state) => {
  cy.state(state)
  })

diff --git a/packages/web-config/webpack.config.base.ts b/packages/web-config/webpack.config.base.ts
@@ -267,6 +267,7 @@ export const getSimpleConfig = () => ({
  // packages/driver/src/cypress/source_map_utils.js when creating
  // the cross origin bundle. for now, this is necessary so the build
  // doesn't fail
+ // https://github.com/cypress-io/cypress/issues/19888
  {
  test: /\.wasm$/,
  type: 'javascript/auto',

diff --git a/system-tests/test/multi_domain_error_spec.ts b/system-tests/test/multi_domain_error_spec.ts
@@ -11,6 +11,8 @@ const onServer = function (app) {
  })
 }
 
+// TODO: This is probably more appropriate for a cy-in-cy test
+// https://github.com/cypress-io/cypress/issues/20973
 describe('e2e cy.origin errors', () => {
  systemTests.setup({
  servers: [{
@@ -39,7 +41,7 @@ describe('e2e cy.origin errors', () => {
  expect(res.stdout).to.contain('AssertionError')
  expect(res.stdout).to.contain('Timed out retrying after 1000ms: Expected to find element: `#doesnotexist`, but never found it.')
 
- // check to make sure the snapshot contains the 'cy.origin' sourcemap. TODO: This is probably more appropriate for a cy-in-cy test
+ // check to make sure the snapshot contains the 'cy.origin' sourcemap
  expect(res.stdout).to.contain('http://localhost:3500/__cypress/tests?p=cypress/integration/multi_domain_error_spec.ts:102:12')
  },
  })

diff --git a/system-tests/test/navigation_spec.ts b/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', () => {
  systemTests.setup({
  servers: [{
@@ -22,7 +20,8 @@ describe('e2e cross origin navigation', () => {
  },
  })
 
- // FIXME: add test that supports this for firefox
+ // TODO: convert to cypress-in-cypress test if possible
+ // https://github.com/cypress-io/cypress/issues/20973
  systemTests.it('captures cross origin failures when "experimentalLoginFlows" config value is falsy', {
  // keep the port the same to prevent issues with the snapshot
  port: PORT,