fix: only take snapshots if the AUT document is in state #24519

AtofStryker · 2022-11-03T13:38:02Z

Closes Improve cy.origin time travel snapshots #24506

User facing changelog

Interactive time travel snapshots no longer capture all of Cypress application when Application Under Test frame has navigated away.

Additional details

Main thing this is fixing is new behavior introduced in 10.9.0 likely with #23297 where there is now async behavior from when snapshots are taken vs what document is loaded in state. This was leading to Cypress in Cypress like snapshots, which don't provide the user a lot of value and are confusing. This PR makes sure either the document is in state or the snapshot is coming from a cross origin spec bridge (cy.origin).

Steps to test

How has the user experience changed?

before fix

after fix

PR Tasks

Have tests been added/updated? (tests to make sure no regressions in snapshot behavior are passing)
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?N/A
Have API changes been updated in the type definitions?

cypress-bot · 2022-11-03T13:38:06Z

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-11-03T15:07:04Z

Test summary

422 • 0 • 10 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`6f61da3`
Started	Nov 7, 2022 6:30 PM
Ended	Nov 7, 2022 6:41 PM
Duration	11:37 💡
OS	Linux Debian -
Browser	Chrome 106

View run in Cypress Dashboard ➡️

Flakiness

	runs.cy.ts	1
1	App: Runs > Runs - Login > when logged out, shows call to action
	specs_list_latest_runs.cy.ts	1
1	App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs

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

chrisbreiding

Is it possible to add a test for this?

AtofStryker · 2022-11-03T16:02:07Z

Is it possible to add a test for this?

I think we can but I am not sure it will be reliable. Maybe a test that executes a visit away from the primary domain and checks the log snapshot to see if one exists, and if one does, fail the test?

chrisbreiding · 2022-11-03T17:51:26Z

I think we can but I am not sure it will be reliable. Maybe a test that executes a visit away from the primary domain and checks the log snapshot to see if one exists, and if one does, fail the test?

It doesn't necessarily have to be a full e2e test. More of unit/integration test like these would still give us some coverage.

AtofStryker · 2022-11-03T19:56:21Z

I think we can but I am not sure it will be reliable. Maybe a test that executes a visit away from the primary domain and checks the log snapshot to see if one exists, and if one does, fail the test?

It doesn't necessarily have to be a full e2e test. More of unit/integration test like these would still give us some coverage.

sounds good to me. I added a small integration test to make sure the snapshots are null after the document unloads as well as an e2e test to make sure cy origin snapshots still work in 97da4aa

packages/driver/src/cy/snapshots.ts

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

emilyrohrbough · 2022-11-04T13:56:06Z

packages/driver/src/cy/snapshots.ts

+    if (!preprocessedSnapshot && !state('document')) {
+      return null
+    }


Does this fix just mean there are more missing screenshots? Can you share a few scenarios this is currently broken? I'd like to test the before & after locally. Does the comment on line 249 no long valid? This is where we are falling back to window.document....would it be more appropriate to query for the AUT iframe?

~~I am also wondering if we should update some of our origin tests to verify there are snapshots / no snapshots for various scenarios.~~

I didn't realize the updated tests were specific to origin tests for snapshot. Will be exicted to merge these someday 😄

Does the comment on line 249 no long valid? This is where we are falling back to window.document

That's a good question. Seems like these lines should be updated/removed.

would it be more appropriate to query for the AUT iframe?

I think if there's no state('document'), it won't be possible because that means we don't have synchronous access to the AUT.

This actually brings up a good point, but I think it needs to stay. Main reason is logs streaming in from the secondary need to be cloned off a document, which in this case the primary document state is unloaded. But the backup isn't the AUT document in this case, rather the actual top document, which is OK since we only using the window to clone the node. If anything, we should probably use window.document here instead of state("document")

I'll play around with this to make sure it works as expect and will add a comment to explain this logic

Does this fix just mean there are more missing screenshots? Can you share a few scenarios this is currently broken? I'd like to test the before & after locally.

@emilyrohrbough you can run the snapshot tests from the driver locally, but the impact isn't exactly super noticeable there. Do you have cypress-origin-providers set up at all?

Added comments in 228cef0. I kept the current logic of using loaded document in state first before the specbridge/root document because of how many edge cases exist in snapshot code that could potentially break, but not be caught by this change. Here be 🐉 s.

I can easily setup cypress-origin-providers but even if I run the driver tests, what would I be looking for to verify this was acutally fixed? xhr requests?

If you run the snapshots.cy.ts inside the origin driver tests, hover over the (page load) or (new url) logs to see no snapshot taken, where as before it would look something like this:

emilyrohrbough · 2022-11-04T13:58:46Z

packages/driver/cypress/e2e/e2e/origin/snapshots.cy.ts


-      expect(xhrLogFromSecondaryOrigin).to.not.be.undefined
+      expect(getLogFromSecondaryOrigin).to.not.be.undefined


Suggested change

expect(getLogFromSecondaryOrigin).to.not.be.undefined

expect(getLogFromSecondaryOrigin).to.be.defined

I am not sure why but I cannot get this type to show up in a chai assertion. I don't think it is actually valid 🙁

Yeah, doesn't seem to be a bundled chai assertion, but we add on to chai with various libs, so it's possible it ends up as an assertion somehow, since we do use to.be.defined in a few other tests. I think the "correct" assertion is .to.exist.

updated to to.exist in 17d37f5

emilyrohrbough · 2022-11-04T14:00:13Z

packages/driver/cypress/e2e/e2e/origin/snapshots.cy.ts


-  beforeEach(() => {
-    logs = new Map()
+        return (props?.consoleProps?.Selector === selector)


What log is this associated with?

In this case ,we are just digging out the [data-cy="assertion-header"] get log to make sure the snapshot is sent back over and not omitted with our new logic changes

Is that associated with the origin command log or an xhr log?

origin command log since the get is invoked inside the cy.origin

emilyrohrbough · 2022-11-04T14:02:19Z

packages/driver/cypress/e2e/e2e/origin/snapshots.cy.ts

    })

-    cy.visit('http://www.foobar.com:3500/fixtures/xhr-fetch-requests.html')
+    it('Does not take snapshots of XHR/fetch requests from secondary origin if the wrong origin is / origin mismatch, but instead the primary origin (existing behavior)', {


I read this 2x and am a little confused

I think this is missing the word visited in the name. I can add it.

addressed in 1d43234

emilyrohrbough · 2022-11-04T14:02:23Z

packages/driver/cypress/e2e/e2e/origin/snapshots.cy.ts

+        Cypress.config('isInteractive', true)
+
+        cy.get(`[data-cy="assertion-header"]`).should('exist')
+        cy.wait('@fooBarBaz')


Is this failing because the alias does not exist?

NVM coming from beforeEach

emilyrohrbough · 2022-11-04T14:04:31Z

packages/driver/cypress/e2e/e2e/origin/snapshots.cy.ts

+    })
+
+    // TODO: fix flaky test https://github.com/cypress-io/cypress/issues/23437
+    it.skip('verifies fetch requests made while a secondary origin is active eventually update with snapshots of the secondary origin', () => {


Are we able to unskip these tests? Maybe add some retries?

Let me revisit this and see what the current state is

So I can get the tests to pass, but from the flake report the flake is fairly rare, like 2%. I think by default we already retry twice in CI, so I doubt it will fix the problem immediately. @mjhenkes and I were able to reproduce this a few months back, and if I remember correctly there were collisions with the proxy logging on xhr request. I could be completely wrong though. It's been a while 😅

it's possible this gets resolved by removing server/route, but i'm not 100%. I vote we revisit the flake tests after 12.0.0

… body

…/cypress into bugfix/cross-origin-snapshots

AtofStryker · 2022-11-04T19:05:33Z

@chrisbreiding @emilyrohrbough this should be ready for another look. I forgot I needed to set isInteractive inside cy.origin to receive snapshots

…ross-origin-snapshots

fix: only take snapshots if the AUT document is in state

458786b

AtofStryker force-pushed the bugfix/cross-origin-snapshots branch from fb480f5 to 458786b Compare November 3, 2022 14:16

AtofStryker self-assigned this Nov 3, 2022

AtofStryker requested review from chrisbreiding, emilyrohrbough, mjhenkes and mschile November 3, 2022 14:50

AtofStryker marked this pull request as ready for review November 3, 2022 14:50

mjhenkes approved these changes Nov 3, 2022

View reviewed changes

chrisbreiding reviewed Nov 3, 2022

View reviewed changes

test: add tests for snapshot behavior

97da4aa

AtofStryker requested a review from chrisbreiding November 3, 2022 19:56

chrisbreiding requested changes Nov 4, 2022

View reviewed changes

packages/driver/src/cy/snapshots.ts Outdated Show resolved Hide resolved

AtofStryker and others added 2 commits November 4, 2022 09:54

Update packages/driver/src/cy/snapshots.ts

d904eeb

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

Merge branch 'develop' into bugfix/cross-origin-snapshots

7c5ab8a

AtofStryker requested a review from chrisbreiding November 4, 2022 13:55

emilyrohrbough reviewed Nov 4, 2022

View reviewed changes

AtofStryker added 3 commits November 4, 2022 11:21

chore: add comment to explain fallback document when cloning snapshot…

228cef0

… body

chore: add more context to xhr test name

1d43234

Merge branch 'bugfix/cross-origin-snapshots' of github.com:cypress-io…

6a6c303

…/cypress into bugfix/cross-origin-snapshots

AtofStryker requested a review from emilyrohrbough November 4, 2022 15:28

AtofStryker added 4 commits November 4, 2022 11:29

fix: update not.to.be.undefined to to exist in snapshots.cy.ts

17d37f5

fix: attempt to patch test to prevent nullish failures

6d74227

fix: turn on interactive flag in spec bridge to get snapshots

ae2d5cd

Merge branch 'develop' into bugfix/cross-origin-snapshots

6fcb661

chrisbreiding approved these changes Nov 4, 2022

View reviewed changes

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

6f61da3

…ross-origin-snapshots

AtofStryker merged commit 3cdbeca into develop Nov 7, 2022

AtofStryker deleted the bugfix/cross-origin-snapshots branch November 7, 2022 18:37

cypress-bot bot mentioned this pull request Nov 7, 2022

Improve cy.origin time travel snapshots #24506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: only take snapshots if the AUT document is in state #24519

fix: only take snapshots if the AUT document is in state #24519

AtofStryker commented Nov 3, 2022 •

edited

cypress-bot bot commented Nov 3, 2022

cypress bot commented Nov 3, 2022 •

edited

chrisbreiding left a comment

AtofStryker commented Nov 3, 2022

chrisbreiding commented Nov 3, 2022

AtofStryker commented Nov 3, 2022

emilyrohrbough Nov 4, 2022

emilyrohrbough Nov 4, 2022 •

edited

chrisbreiding Nov 4, 2022

AtofStryker Nov 4, 2022

AtofStryker Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

chrisbreiding Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

AtofStryker Nov 4, 2022

emilyrohrbough Nov 4, 2022

emilyrohrbough Nov 4, 2022

emilyrohrbough Nov 4, 2022

AtofStryker Nov 4, 2022

AtofStryker Nov 4, 2022

mjhenkes Nov 4, 2022

AtofStryker commented Nov 4, 2022


		expect(xhrLogFromSecondaryOrigin).to.not.be.undefined
		expect(getLogFromSecondaryOrigin).to.not.be.undefined

	expect(getLogFromSecondaryOrigin).to.not.be.undefined
	expect(getLogFromSecondaryOrigin).to.be.defined

fix: only take snapshots if the AUT document is in state #24519

fix: only take snapshots if the AUT document is in state #24519

Conversation

AtofStryker commented Nov 3, 2022 • edited

User facing changelog

Additional details

Steps to test

How has the user experience changed?

before fix

after fix

PR Tasks

cypress-bot bot commented Nov 3, 2022

cypress bot commented Nov 3, 2022 • edited

Test summary

Run details

Flakiness

chrisbreiding left a comment

Choose a reason for hiding this comment

AtofStryker commented Nov 3, 2022

chrisbreiding commented Nov 3, 2022

AtofStryker commented Nov 3, 2022

Choose a reason for hiding this comment

emilyrohrbough Nov 4, 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

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

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 commented Nov 4, 2022

AtofStryker commented Nov 3, 2022 •

edited

cypress bot commented Nov 3, 2022 •

edited

emilyrohrbough Nov 4, 2022 •

edited