fix: support snapshots of xhr/fetch and other logs generated from the primary #21552

AtofStryker · 2022-05-18T19:50:24Z

that need screenshots of the secondary

Closes Snapshot's in xhr requests for absolute URLs take a snapshot of the primary instead of the current domain #21496

User facing changelog

Additional details

The goal of this PR is to have snapshots reflect the current active origin for logs generated in the primary domain. Currently, logs generated in the primary domain, such as xhr/fetch requests, show snapshots of the primary stale domain when a secondary domain is active. This should only mainly impact xhr/fetch requests as the logs are generated in the primary, but there is potential impact elsewhere as there are changes to the snapshot function inside the log. This is gated behind the experimentalSessionAndOrigin flag, so any regressions, if any, should be behind the feature flag.

How has the user experience changed?

-- example using a site with a lot of requests

before

after

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?

cypress-bot · 2022-05-18T19:50:26Z

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.

cypress · 2022-05-18T21:18:51Z

Test summary

37840 • 0 • 615 • 0 • 7

Run details


Project	cypress
Status	Passed
Commit	`4d2e378`
Started	Aug 17, 2022 3:07 PM
Ended	Aug 17, 2022 3:23 PM
Duration	15:26 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		5
1	network stubbing > intercepting request > can add a body to a request that does not have one
2	network stubbing > intercepting request > can delay with deprecated delayMs param
3	network stubbing > intercepting request > can delay with deprecated delayMs param
4	network stubbing > intercepting request > can delay with deprecated delayMs param
5	network stubbing > intercepting request > can delay with deprecated delayMs param
This comment includes only the first 5 flaky tests. See all 7 flaky tests in the 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

mjhenkes · 2022-05-20T16:10:15Z

packages/driver/cypress/fixtures/xhr-fetch-onload.html

+ function fireXHRAndFetchRequests() {
+
+ xhr = new XMLHttpRequest();
+ xhr.open("GET", "https://jsonplaceholder.cypress.io/todos/1");


can we make a fetch request to our local server instead of to .cypress.io? I see us using this in a couple of other places already, but the seems simple enough that we could add it to the server.

Made a fixture and stubbed the body out with cy.intercept in 265617f. Seems to do what we want?

packages/driver/src/cross-origin/communicator.ts

mjhenkes · 2022-05-20T17:39:26Z

packages/driver/src/cypress/log.ts

- // insert at index 'at' or whatever is the next position
- snapshots[options.at || snapshots.length] = snapshot
+ if (options.next && shouldRebindSnapshotFn) {
+ const fn = this.snapshot


Could we rename fn to be more descriptive?

how is something like 5df6787? I changed the name to originalLogSnapshotFn.

mjhenkes · 2022-05-20T17:45:54Z

packages/driver/src/cypress/log.ts

- this.set('snapshots', snapshots)
+ if (this.config('experimentalSessionAndOrigin') && !Cypress.isCrossOriginSpecBridge) {
+ // @ts-ignore
+ const { originPolicy: activeSpecBridgeOriginPolicyIfApplicable } = this.state('currentActiveOriginPolicy') ? Cypress.Location.create(this.state('currentActiveOriginPolicy')) : {


I think the currentActiveOriginPolicy, if present, already is an originPolicy so you shouldn't have to re-create it?

https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/commands/origin/index.ts#L119

addressed in 2ee5441.

mjhenkes · 2022-05-20T17:54:38Z

packages/driver/src/cypress/log.ts


- if (options.next) {
- const fn = this.snapshot
+ if (activeSpecBridgeOriginPolicyIfApplicable && activeSpecBridgeOriginPolicyIfApplicable === originPolicyThatIsSoonToBeOrIsActive) {


In what scenarios is this not true? If we are in a cy.origin block but have begun navigating to another origin? Does it make a difference if it's the top origin or another cross origin?

Mainly the error case/operator error. If a user navigates to an origin that doesn't match the spec bridge origin, like

cy.get('[href="https://www.foobar.com"]').click() cy.origin('https://www.barbaz.com', () => { // wrong origin, don't snapshot this })

I added a test here to make sure this fails gracefully.

emilyrohrbough · 2022-05-20T18:16:57Z

packages/driver/cypress/integration/e2e/origin/snapshots.spec.ts

@@ -0,0 +1,89 @@
+// import to bind shouldWithTimeout into global cy commands


What do you mean by this? isn't this picked up in the the global via the support file?

It is not. We don't import utils inside the support directory in the index or defaults. Not entirely sure why, because this seems like it should be included there...

mjhenkes · 2022-05-20T18:32:11Z

packages/driver/src/cypress/log.ts

 }

- this.set('snapshots', snapshots)
+ if (this.config('experimentalSessionAndOrigin') && !Cypress.isCrossOriginSpecBridge) {


Would it be possible to somehow tie this to the command group the log is part of? Like add the originPolicy to the command group and use it for any events generated within that group? @emilyrohrbough, is something like that possible (or better)?

mschile · 2022-05-23T14:25:57Z

packages/driver/cypress/integration/e2e/origin/snapshots.spec.ts

+ cy.origin('http://foobar.com:3500', () => {
+ cy.get(`[data-cy="assertion-header"]`).should('exist')
+ // have some wait time to allow for the xhr/fetch requests to settle
+ cy.wait(200)


Once #21579 merges, you can wait for the actual request instead of having an arbitrary wait time. You may be able to do it now if you don't wait inside of cy.origin.

I gave it a try waiting outside. The test seems to flake about 50/50, I am guessing due to the time for the log to come back and be searchable after the alias resolves takes longer than 250ms.

Would it make sense to increase the timeout in shouldWithTimeout?

packages/driver/src/cross-origin/cypress.ts

emilyrohrbough · 2022-05-24T14:23:36Z

packages/driver/src/cypress/log.ts

- return fn.call(this, options.next)
+ Cypress.primaryOriginCommunicator.toSpecBridge(activeSpecBridgeOriginPolicyIfApplicable, 'generate:snapshot:for:log', {
+ name,
+ id: eventID,


Why can't the log's id be used? We can associate multiple snapshots to a log so I would expect this to be fine / no need to manage another unique eventID.

After revisiting, I removed the date.now() and opted for just going with the log ID, which the event handling now lives in the event manager to ask the spec bridge for the log snapshot.

AtofStryker · 2022-05-24T16:21:21Z

After talking with @emilyrohrbough , I am going to move this back to draft to see if I can figure out a better pattern then just baking the snapshot behavior into the log manager itself.

…-absolute

…en XHR requests are made, as well as used aliases for requests over arbitrary waits

…-absolute

emilyrohrbough · 2022-08-01T14:52:35Z

packages/driver/src/cross-origin/communicator.ts

@@ -191,8 +191,9 @@ export class SpecBridgeCommunicator extends EventEmitter {
 data = preprocessLogForSerialization(data as any)
 }

- // If requested by the runner, preprocess the final snapshot before sending through postMessage() to attempt to serialize the DOM body of the snapshot.
- if (FINAL_SNAPSHOT_EVENT === eventName) {
+ // If requested by the runner, preprocess the snapshot before sending through postMessage() to attempt to serialize the DOM body of the snapshot.


nit: lets specify @packages/runner since the driver has a Runner class as well

sorry this was confusing...more like, we should specific which runner. it seems like it's this runner:

https://github.com/cypress-io/cypress/tree/develop/packages/app/src/runner

but our code base also has these runners:

https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress/runner.ts

https://github.com/cypress-io/cypress/tree/develop/packages/runner

chrisbreiding · 2022-08-12T13:27:56Z

packages/app/src/runner/event-manager.ts

+ Cypress.primaryOriginCommunicator.once(`snapshot:for:log:generated:${eventID}`, (generatedCrossOriginSnapshot) => {
+ const snapshot = generatedCrossOriginSnapshot.body ? generatedCrossOriginSnapshot : null
+
+ addSnapshot.apply(log, [snapshot, options, false])
+ })
+
+ Cypress.primaryOriginCommunicator.toSpecBridge(specBridge, 'generate:snapshot:for:log', {
+ name,
+ id: eventID,
+ })


This might be one of those cases where adding a promise-based API to the communicator would be useful. Then it could be something like:

const generatedCrossOriginSnapshot = await Cypress.primaryOriginCommunicator.toSpecBridgeAsync(specBridge, 'generate:snapshot:for:log') const snapshot = generatedCrossOriginSnapshot.body ? generatedCrossOriginSnapshot : null addSnapshot.apply(log, [snapshot, options, false])

I'm not suggesting adding it in this PR, but wanted to call it out as an example. Maybe when we have another use-case, we could add that and refactor this one.

packages/driver/src/cypress/log.ts

Co-authored-by: Matt Schile <mschile@cypress.io>

AtofStryker marked this pull request as ready for review May 19, 2022 16:09

AtofStryker requested a review from a team as a code owner May 19, 2022 16:09

AtofStryker requested review from jennifer-shehane and removed request for a team May 19, 2022 16:09

jennifer-shehane removed their request for review May 19, 2022 16:10

AtofStryker requested review from chrisbreiding, emilyrohrbough, mjhenkes and mschile May 19, 2022 16:10

AtofStryker changed the title ~~support snapshots of xhr/fetch and other logs generated from the primary~~ fix: support snapshots of xhr/fetch and other logs generated from the primary May 19, 2022

mjhenkes reviewed May 20, 2022

View reviewed changes

packages/driver/src/cross-origin/communicator.ts Show resolved Hide resolved

mjhenkes reviewed May 20, 2022

View reviewed changes

packages/driver/src/cross-origin/communicator.ts Show resolved Hide resolved

mjhenkes reviewed May 20, 2022

View reviewed changes

emilyrohrbough reviewed May 20, 2022

View reviewed changes

mjhenkes reviewed May 20, 2022

View reviewed changes

AtofStryker requested review from emilyrohrbough and mjhenkes May 23, 2022 13:54

mschile requested changes May 23, 2022

View reviewed changes

AtofStryker requested a review from mschile May 23, 2022 16:22

emilyrohrbough reviewed May 24, 2022

View reviewed changes

AtofStryker marked this pull request as draft May 24, 2022 16:21

AtofStryker added 3 commits July 25, 2022 14:45

add special serialization rules for snapshot prefix

313566a

add failing regression tests

cc0f144

allow for snapshots to delegate to the active spec bridge if applicable

d90da5c

AtofStryker force-pushed the snapshot-absolute branch from dbc3106 to d90da5c Compare July 25, 2022 20:40

Merge branch 'develop' of github.com:cypress-io/cypress into snapshot…

c7ecf28

…-absolute

AtofStryker requested a review from emilyrohrbough July 25, 2022 21:48

AtofStryker added 3 commits July 25, 2022 18:02

test against consoleProps URL which is more consistent than log url

85e0036

clean up snapshot tests to set interactive mode in the spec bridge wh…

32b25f9

…en XHR requests are made, as well as used aliases for requests over arbitrary waits

Merge branch 'develop' of github.com:cypress-io/cypress into snapshot…

957cdb0

…-absolute

AtofStryker marked this pull request as ready for review July 26, 2022 15:16

emilyrohrbough reviewed Aug 1, 2022

View reviewed changes

mjhenkes approved these changes Aug 10, 2022

View reviewed changes

emilyrohrbough approved these changes Aug 11, 2022

View reviewed changes

chrisbreiding reviewed Aug 12, 2022

View reviewed changes

chrisbreiding approved these changes Aug 12, 2022

View reviewed changes

AtofStryker and others added 2 commits August 16, 2022 12:26

Merge branch 'develop' into snapshot-absolute

dd85921

Merge branch 'develop' into snapshot-absolute

43881fd

mschile reviewed Aug 17, 2022

View reviewed changes

packages/driver/src/cypress/log.ts Outdated Show resolved Hide resolved

mschile reviewed Aug 17, 2022

View reviewed changes

packages/driver/src/cypress/log.ts Outdated Show resolved Hide resolved

mschile approved these changes Aug 17, 2022

View reviewed changes

AtofStryker and others added 3 commits August 17, 2022 10:32

Update packages/driver/src/cypress/log.ts

0ad5d74

Co-authored-by: Matt Schile <mschile@cypress.io>

Update packages/driver/src/cypress/log.ts

c48fac4

Co-authored-by: Matt Schile <mschile@cypress.io>

chore: fix trailing space

4d2e378

AtofStryker merged commit 53f0a02 into develop Aug 17, 2022

AtofStryker deleted the snapshot-absolute branch August 17, 2022 15:47

cypress-bot bot mentioned this pull request Aug 17, 2022

Snapshot's in xhr requests for absolute URLs take a snapshot of the primary instead of the current domain #21496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support snapshots of xhr/fetch and other logs generated from the primary #21552

fix: support snapshots of xhr/fetch and other logs generated from the primary #21552

AtofStryker commented May 18, 2022 •

edited

Loading

cypress-bot bot commented May 18, 2022

cypress bot commented May 18, 2022 •

edited

Loading

mjhenkes May 20, 2022

AtofStryker May 23, 2022

mjhenkes May 20, 2022

AtofStryker May 23, 2022

mjhenkes May 20, 2022

AtofStryker May 23, 2022

mjhenkes May 20, 2022

AtofStryker May 23, 2022

emilyrohrbough May 20, 2022

AtofStryker May 20, 2022

mjhenkes May 20, 2022

mschile May 23, 2022

AtofStryker May 23, 2022 •

edited

Loading

mschile May 23, 2022

emilyrohrbough May 24, 2022 •

edited

Loading

AtofStryker Jul 28, 2022

AtofStryker commented May 24, 2022

emilyrohrbough Aug 1, 2022

emilyrohrbough Aug 11, 2022

chrisbreiding Aug 12, 2022

		@@ -0,0 +1,89 @@
		// import to bind shouldWithTimeout into global cy commands

fix: support snapshots of xhr/fetch and other logs generated from the primary #21552

fix: support snapshots of xhr/fetch and other logs generated from the primary #21552

Conversation

AtofStryker commented May 18, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

before

after

PR Tasks

cypress-bot bot commented May 18, 2022

cypress bot commented May 18, 2022 • edited Loading

Test summary

Run details

Flakiness

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

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

AtofStryker May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented May 18, 2022 •

edited

Loading

cypress bot commented May 18, 2022 •

edited

Loading

AtofStryker May 23, 2022 •

edited

Loading

emilyrohrbough May 24, 2022 •

edited

Loading