-
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: No unnecessary snapshotting #19311
Changes from 11 commits
3a12281
6f345a7
626d2dc
e099fe5
1f40528
5917287
6bceea2
68930a8
bc06bdc
ed7a776
388c71b
91c773c
b914010
30b5f49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,9 +143,19 @@ export const create = (Cypress, cy) => { | |
obj.$el = $dom.wrap(value) | ||
} | ||
|
||
// if we are simply verifying the upcoming | ||
// assertions then do not immediately end or snapshot | ||
// else do | ||
// `verifying` represents whether we're deciding whether or not to resolve | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe out of scope, but thoughts on renaming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd like to save renaming things for a later date. My real goal would be to make a flowchart of the logic and consider a refactor of this area of code - Brian spoke to some regrets that we didn't just re-implement parts of Chai, rather than weaving in and out of 3rd party code and keeping track of state internally to make it behave as we want. |
||
// a command (true) or of we're actually performing a user-facing assertion | ||
// (false). | ||
|
||
// If we're verifying upcoming assertions (implicit or explicit), | ||
// then we don't need to take a DOM snapshot - one will be taken later when | ||
// retries time out or the command otherwise entirely fails or passes. | ||
// We save the error on _error because we may use it to construct the | ||
// timeout error which we eventually do display to the user. | ||
|
||
// If we're actually performing an assertion which will be displayed to the | ||
// user though, then we want to take a DOM snapshot and display this error | ||
// (if any) in the log message on screen. | ||
if (verifying) { | ||
obj._error = error | ||
} else { | ||
|
@@ -267,6 +277,7 @@ export const create = (Cypress, cy) => { | |
} | ||
|
||
const onPassFn = () => { | ||
cy.state('overrideAssert', undefined) | ||
if (_.isFunction(callbacks.onPass)) { | ||
return callbacks.onPass.call(this, cmds, options.assertions) | ||
} | ||
|
@@ -289,6 +300,7 @@ export const create = (Cypress, cy) => { | |
err = e2 | ||
} | ||
|
||
cy.state('overrideAssert', undefined) | ||
err.isDefaultAssertionErr = isDefaultAssertionErr | ||
|
||
options.error = err | ||
|
@@ -324,6 +336,24 @@ export const create = (Cypress, cy) => { | |
// bail if we have no assertions and apply | ||
// the default assertions if applicable | ||
if (!cmds.length) { | ||
// In general in cypress, when assertions fail we want to take a DOM | ||
// snapshot to display to the user. In this case though, when we invoke | ||
// ensureExistence, we're not going to display the error (if there is | ||
// one) to the user - we're only deciding whether to resolve this current | ||
// command (assertions pass) or fail (and probably retry). A DOM snapshot | ||
// isn't necessary in either case - one will be taken later as part of the | ||
// command (if they pass) or when we time out retrying. | ||
|
||
// Chai assertions have a signature of (passed, message, value, actual, | ||
// expected, error). Our assertFn, defined earlier in the file, adds | ||
// on a 7th arg, "verifying", which defaults to false. We here override | ||
// the assert function with our own, which just invokes the old one | ||
// with verifying = true. This override is cleaned up immediately | ||
// afterwards, in either onPassFn or onFailFn. | ||
cy.state('overrideAssert', function (...args) { | ||
BlueWinds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return assertFn.apply(this, args.concat(true)) | ||
}) | ||
|
||
return Promise | ||
.try(ensureExistence) | ||
.then(onPassFn) | ||
|
@@ -477,8 +507,6 @@ export const create = (Cypress, cy) => { | |
return cy.state('overrideAssert', undefined) | ||
} | ||
|
||
// store this in case our test ends early | ||
// and we reset between tests | ||
cy.state('overrideAssert', overrideAssert) | ||
|
||
return Promise | ||
|
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.
Do we not manage / track the number of snapshots taken during a test? I assumed this data was reported to the 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.
These DOM snapshots are the ones visible when you hover over test steps and it displays how the application looked at that point in time. They're very ephemeral - we only take them in open mode, and they live in the browser running tests, not even sent to the cypress server. We discard the snapshots regularly (if the number of tests run is greater than
numTestsKeptInMemory
, whenever we rerun tests, etc).We didn't even have an internal event for counting them until I added it in this PR (
Cypress.action('cy:snapshot', name)
) so this utility could listen for it.