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.

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

chore: develop (including cy.origin) merge into 10.0 #21237

Merged
merged 266 commits into from May 9, 2022

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Apr 28, 2022

DO NOT SQUASH!!!!

Closes #19887

User facing changelog (as seen in #18075)

  • Removed experimentalSessionSupport Flag
  • Added experimentalSessionAndOrigin Flag containing:
    • All features from the experimentalSessionSupportFlag
    • A new Cypress command called origin which allows users to run commands within multiple origins from a single test.
    • Cross-domain requests will no longer fail immediately, but instead, time out based on pageLoadTimeout.
    • Tests will no longer wait on page loads before moving on to the next test.

Additional details

The goal of this PR is to merge develop containing the cy.origin command and a few other updates, into 10.0. For original PR into develop, please see #18075. To recap important changes in the development environment:

  • Driver integration tests in CI now have two jobs per browser:
    • 1 each with experimentalSessionAndOrigin off (with the tests restructured to use a visit between tests to be compatible with sessions). The cy.origin tests are not run in this job
    • 1 each with experimentalSessionAndOrigin on with cy.origin tests run in this job. When this feature becomes GA, the goal is to consolidate back into 1 integration job per browser.

For merging with 10.0, main conflicts/changes from the develop branch into 10.0:

Driver

  • Test file restructuring (from integration to e2e and following *.cy.js paradigm over *.spec.js) and various snapshot updates. Also updating tests that look for cypress.json to look for new cypress.config.js paradigm.
  • Renaming createSpecIframe to generic addIframe as we need to now add multiple iframes to create the spec bridges for cy.origin.

Runner / App

  • In develop, we were able to reference Cypress to remove/create the iframes and inject them into the page. Since it doesn't look to be instantiated yet, the same functionality has been replaced with native DOM methods.
  • Moving all the logic from the old runner -> app that is AUT/iframe/event driven:
    • event-manager (new websocket/communicator methods, screenshot methods (NEEDS TO BE VERIFIED)
    • aut-iframe changes for cross origin snapshotting behavior and removal of snapshot responsibilities (NEEDS TO BE VERIFIED)
    • iframe-model changes for cross origin snapshotting behavior

Server / Data-Context

  • Addition of remoteStates concept needs to be consumed by the data-context package when serving the app. runner.serve is no longer used in 10.0 and makeServeConfig is the method where remoteStates likely needs to be reset. When the config is served, remote should otherwise be set to current.
  • CRI client changes to account for frame tree updates to attach X-Cypress-Is-AUT-Frame header to inform the server to inject the cross origin driver, as well as passing the feature flag into the CDP automation static constructor.
  • IMPORTANT: Resolving the to runner directory in e2e mode and the runner-ct directory otherwise to make sure the runner routes can serve the cross origin bundle. (THIS NEEDS TO BE VERIFIED IF THIS CHANGE IS CORRECT)

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) N/A
  • Has a PR for user-facing changes been opened in cypress-documentation? N/A: Docs for cy.origin are merged into cypress-documentation and shipped with 9.6.0
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json? Applicable item changes from cypress.schema.json were migrated to frontend-shared locales to be referenced on the settings page (see screenshot below)

Screen Shot 2022-05-05 at 3 33 29 PM
Screen Shot 2022-05-05 at 3 34 59 PM

chrisbreiding and others added 30 commits July 13, 2021 09:14
@@ -0,0 +1,14 @@
declare namespace Cypress {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

likely the changes here need to be overwritten to satisfy is comment

@@ -176,13 +178,13 @@ const ffToStandardResourceTypeMap: { [ff: string]: ResourceType } = {
}

export class CdpAutomation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

original in 9.x

let frameTree
let gettingFrameTree

const onReconnect = (client: CRIWrapper.Client) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not exactly a 1:1 here, but this callback handler is defined here and passed into browserCRIClient static constructor which is ultimately referenced in the generic criClient static contructor. original in 9.x

@@ -147,6 +147,10 @@ export const create = async (target: string, onAsynchronousError: Function, host
})

enqueuedCommands = []

if (onReconnect) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see original in 9.x

}

// eslint-disable-next-line @cypress/dev/arrow-body-multiline-braces
const _updateFrameTree = (client: CRIWrapper.Client, eventName) => async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see original in 9.x

@@ -450,16 +570,21 @@ export = {
])

await this._navigateUsingCRI(pageCriClient, options.url)

if (options.experimentalSessionAndOrigin) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we have a bit of a different process now for attaching to target and getting the browser CRI client, we need to make sure that the appropriate even listeners are applied every time we attach to a new target, as well as turn on any necessary CDP events.

@@ -324,7 +324,7 @@ const onReconnect = (client: CRIWrapper.Client) => {
// if the client disconnects (e.g. due to a computer sleeping), update
// the frame tree on reconnect in cases there were changes while
// the client was disconnected
_updateFrameTree(client)()
return _updateFrameTree(client)()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this asynchronous? Do we need to await it in criClient.create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is asynchronous, but we do not need to await it. The return statement here is to appease the linter. The waiting gets handled by the _isAUTFrame function itself here.

@@ -107,7 +107,10 @@ const getAllFn = (...aliases) => {
)
}

const shouldWithTimeout = (cb, timeout = 250) => {
// FIXME: currently increasing the timeout here from 250ms to 1 second
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we log an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should. @mschile logged #21361 for some of the proxy issues happening in electron from this branch. I'll go ahead and create and issue and update the thread here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue logged in #21375

Copy link
Contributor

@flotwig flotwig 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, nothing jumped out at me as being a mismerge.

@lmiller1990
Copy link
Contributor

lmiller1990 commented May 9, 2022

I tried to write some tests using cy.origin to see if it's working correctly, but I could not get it working with our dashboard. It worked with the simple and boring http://acme.com.

I also noticed we can't use cy.origin in packages.app - I'm not sure if this is because of some hacks we did for cypress-in-cypress.

I tried writing some tests in packages/launchpad. It worked better there, but I am still having some problems. Here's a link to the test I wrote: link. The screenshot is below.

image

I tried in develop, and I get the same problem, so I guess it's not a problem introduced by this back merge, so I'll ✅ .

I am curious if I'm doing something wrong - this should work, correct?

image

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

I tried it out and compared between develop and this branch and it seems good.

I had some problems with using cy.origin that seem unrelated, but I posted a comment in case they are: https://cypressio.slack.com/archives/C01TQQCQLJY/p1652071156301199

@AtofStryker
Copy link
Contributor Author

I tried it out and compared between develop and this branch and it seems good.

I had some problems with using cy.origin that seem unrelated, but I posted a comment in case they are: https://cypressio.slack.com/archives/C01TQQCQLJY/p1652071156301199

@lmiller1990 we talked about this standup. It looks like the dashboard code tries to reference window.parent within the dashboard code to set a react devtools flag for when Cypress is on the window. My guess is this is used to test the dashboard code. This fails because parent is in a cross origin context. I bet if you set chromeWebSecurity to false in cypress.config.ts it will start working. We have some issues created to spike into some of the modifyObstructiveCode #21342, but I think that mostly tries to handlewindow.top instead of window.parent

@AtofStryker AtofStryker requested a review from tbiethman May 9, 2022 16:34
@AtofStryker AtofStryker merged commit 6c81ac3 into 10.0-release May 9, 2022
@AtofStryker AtofStryker deleted the md-10.0-merge branch May 9, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cy.origin Problems or enhancements related to cy.origin command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multi-domain]: Merge with 10.0