chore: Refactor assertion logging #23354
Conversation
|
Thanks for taking the time to open a PR!
|
| @@ -790,19 +789,18 @@ describe('src/cy/commands/assertions', () => { | |||
| cy.noop({}).should('have.property', 'foo') | |||
| }) | |||
|
|
|||
| it('ends and snapshots immediately and sets child', (done) => { | |||
| it('snapshots immediately and sets child', (done) => { | |||
There was a problem hiding this comment.
This PR does introduce one test change - the assertion log is in a pending state when initially added, and is passed asynchronously as part of the finishAssertions() function, which happens slightly later in the lifecycle of the command.
Other tests verify that it moves from the pending state to resolved before the next command executes - it's just slightly later in resolving the same promise chain than log:added. I don't believe this is a meaningful difference, and so this can remain a chore even though I had to update one test. 🤷♀️
BlueWinds
changed the title
|
Test summary
Run 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 |
||||||||||||||||||||||||||||||||||||||||||||||
|
Actually, this might be incorrect? I am seeing a lot of red in the command log. I ran a spec in app -
|
Oof, yeah, I see the same. Investigating, will fix and add another driver test once I figure out what's going on here. Wondering if it has to do with cypress-in-cypress. Fixed, see below. |
Ah, figured it out. I've fixed the issue - the 'deduplicate logs' logic just needed to be moved slightly higher in the function - and added an assertion to an existing test to prevent future regressions in the area. See 6849134 for the exact changes, and good catch! |
packages/driver/src/cypress/log.ts
Outdated
| let commandLogId = log.get('commandLogId') | ||
|
|
||
| if (commandLogId != null) { |
There was a problem hiding this comment.
This logic is specific to commands/asserting.ts and commands are responsible for managing their own logs. Theres no need to move this logic here. This also puts additional attributes on the log that don't seem necessary which are stored on on the runner and the command.
There was a problem hiding this comment.
There's a good chance I'll want to end up reusing this in other selectors as part of the Detached DOM work, but I've moved it to asserting.ts for now - you're absolutely right that a comment saying "this is used one place, read about it there" is a code smell.
…rAssertionLogging
BlueWinds
force-pushed
the
issue-7306-refactorAssertionLogging
branch
from
0a50757
to
443bd53
Compare
|
There was a recent merge conflict with develop I had to resolve, took a few commits to get everything cleared up. Should be clean now with all review comments addressed. |
| const originalLogSnapshotFn = this.snapshot | ||
|
|
||
| this.snapshot = function () { | ||
| // restore the original snapshot function | ||
| this.snapshot = originalLogSnapshotFn | ||
|
|
||
| // call orig fn with next as name | ||
| return originalLogSnapshotFn.call(this, options.next) | ||
| } |
There was a problem hiding this comment.
We need to maintain this for cross-origin snapshots.
There was a problem hiding this comment.
I merged in develop, which included a bunch of cross-origin changes, and fixed several merge conflicts; the tests were initially failing, but after some playing around I got them passing again.
Do you have a specific instance in mind where it's failing to snapshot cross-origin in this branch, so I can add a test?
There was a problem hiding this comment.
@emilyrohrbough @BlueWinds I don't believe there is an impact here to cross origin snapshots.
In the case of asking the cross origin spec bridge for a snapshot, this should only happen in the case that something in the primary instance of cypress is requesting a snapshot while a cy.origin block is active or is about to be active, which means it's highly unlikely to be a command asking for the snapshot (ex: XHR's and fetch requests that ask for snapshots as a generated log that aren't direct command invocations). The tests in snapshots.cy.ts should cover this for known cases.
In the case were we DO need to rebind the original snapshot function, this usually happens on interaction events which should almost always be commands. Commands that are run within cy.origin take snapshots within the spec bridge itself, so no asking spec bridges for snapshots should be occuring. This should be covered by the various command tests that assert on consoleProps and other log attributes to verify the snapshot is correct.
Since we don't have tests that verify snapshots interactively (can't leverage the system test and cy-in-cy doesn't support snapshots), I ran this manually to double check and everything looks good from what I can see 😄 .
There was a problem hiding this comment.
Thank you for double checking! I'd hate to have accidentally broken something you merged in just a few days ago.
There was a problem hiding this comment.
Of course! Honestly wish we had a better way to test snapshots moving forward. Getting snapshot testing into cy-in-cy I think would cover a lot of ground.
| @@ -7,6 +7,30 @@ import $errUtils from '../../cypress/error_utils' | |||
| const reExistence = /exist/ | |||
| const reHaveLength = /length/ | |||
|
|
|||
| const onBeforeLog = (log, command, commandLogId) => { | |||
| log.set('commandLogId', commandLogId) | |||
There was a problem hiding this comment.
Any reason we didn't just provide the log id instead of adding a new attribute on the log?
There was a problem hiding this comment.
I tried to use the log id initially, but you can't set a log's ID:
// dont ever allow existing id's to be mutated
if (this.attributes.id) {
delete this.obj.id
}
because the ID is meant to be completely unique - to identify a log instance, across the entire test. Updating it in the log would also require updating the mapping of id -> Log instance that the LogManager maintains, which begins to look like the same level of complexity as having its own attribute.
Also important to note that in the current implementation, the commandLogId isn't globally unique - it's unique (and reproducible) only within the context of a single command. Different instances of .should() can generate the same commandLogId. This isn't a problem in the current implementation, because we only search command.get('logs') for a matching commandLogId.
There was a problem hiding this comment.
Basically, this just serves a different purpose than the log's id. It's a bit of state-tracking for an instance of .should().
User facing changelog
No change in behavior. Refactoring only.
Additional details
As part of the Detached DOM work, I've run into a lot of issues with the way assertion logging currently works. This PR simplifies our use of the "onBeforeLog" hook, moving the logic for "is this the same log message" into easier to find and understand places.
Steps to test
The automated tests are pretty thorough in this regard. Feel free to play around with any existing project, or especially
assertions.cy.tsto verify that nothing has changed.How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?