-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: only take snapshots if the AUT document is in state #24519
Conversation
Thanks for taking the time to open a PR!
|
fb480f5
to
458786b
Compare
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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 |
Co-authored-by: Chris Breiding <[email protected]>
if (!preprocessedSnapshot && !state('document')) { | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
expect(xhrLogFromSecondaryOrigin).to.not.be.undefined | ||
expect(getLogFromSecondaryOrigin).to.not.be.undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(getLogFromSecondaryOrigin).to.not.be.undefined | |
expect(getLogFromSecondaryOrigin).to.be.defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to to.exist
in 17d37f5
|
||
beforeEach(() => { | ||
logs = new Map() | ||
return (props?.consoleProps?.Selector === selector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What log is this associated with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that associated with the origin command log or an xhr log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
origin command log since the get is invoked inside the cy.origin
}) | ||
|
||
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)', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this 2x and am a little confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is missing the word visited
in the name. I can add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 1d43234
Cypress.config('isInteractive', true) | ||
|
||
cy.get(`[data-cy="assertion-header"]`).should('exist') | ||
cy.wait('@fooBarBaz') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this failing because the alias does not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM coming from beforeEach
}) | ||
|
||
// 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', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to unskip these tests? Maybe add some retries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me revisit this and see what the current state is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…/cypress into bugfix/cross-origin-snapshots
@chrisbreiding @emilyrohrbough this should be ready for another look. I forgot I needed to set |
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
cypress-documentation
?N/Atype definitions
?