From 2ab7eb069c2e94f7c322c8a320260f9eb741f0d9 Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Mon, 15 Aug 2022 13:44:19 -0700 Subject: [PATCH 1/9] chore: Refactor assertion logging --- .../cypress/e2e/commands/assertions.cy.js | 20 ++- packages/driver/src/cy/assertions.ts | 124 ++---------------- packages/driver/src/cy/commands/asserting.ts | 55 +++++--- packages/driver/src/cy/commands/waiting.ts | 4 +- packages/driver/src/cy/ensures.ts | 23 +--- packages/driver/src/cypress/log.ts | 64 +++++---- 6 files changed, 102 insertions(+), 188 deletions(-) diff --git a/packages/driver/cypress/e2e/commands/assertions.cy.js b/packages/driver/cypress/e2e/commands/assertions.cy.js index f69f28289212..7f8b7a69d215 100644 --- a/packages/driver/cypress/e2e/commands/assertions.cy.js +++ b/packages/driver/cypress/e2e/commands/assertions.cy.js @@ -715,7 +715,6 @@ describe('src/cy/commands/assertions', () => { it('does not log ensureElExistence errors', function (done) { cy.on('fail', (err) => { assertLogLength(this.logs, 1) - done() }) @@ -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) => { cy.on('log:added', (attrs, log) => { - if (attrs.name === 'assert') { - cy.removeAllListeners('log:added') + if (attrs.name !== 'assert') { + return + } - expect(log.get('ended')).to.be.true - expect(log.get('state')).to.eq('passed') - expect(log.get('snapshots').length).to.eq(1) - expect(log.get('snapshots')[0]).to.be.an('object') - expect(log.get('type')).to.eq('child') + cy.removeAllListeners('log:added') + expect(log.get('snapshots').length).to.eq(1) + expect(log.get('snapshots')[0]).to.be.an('object') + expect(log.get('type')).to.eq('child') - done() - } + done() }) cy.get('body').then(() => { diff --git a/packages/driver/src/cy/assertions.ts b/packages/driver/src/cy/assertions.ts index e2f1f6ab6f4c..a8e72a48b113 100644 --- a/packages/driver/src/cy/assertions.ts +++ b/packages/driver/src/cy/assertions.ts @@ -222,9 +222,11 @@ export const create = (Cypress: ICypress, cy: $Cy) => { return null } - const finishAssertions = (assertions) => { - return _.each(assertions, (log) => { - log.snapshot() + const finishAssertions = () => { + return _.each(cy.state('current').get('logs'), (log) => { + if (!log.get('snapshots')) { + log.snapshot() + } const e = log.get('_error') @@ -238,7 +240,6 @@ export const create = (Cypress: ICypress, cy: $Cy) => { type VerifyUpcomingAssertionsCallbacks = { ensureExistenceFor?: 'subject' | 'dom' | boolean - onPass?: Function onFail?: (err?, isDefaultAssertionErr?: boolean, cmds?: any[]) => void onRetry?: () => any } @@ -256,10 +257,6 @@ export const create = (Cypress: ICypress, cy: $Cy) => { // case where there are no upcoming assertion commands const isDefaultAssertionErr = cmds.length === 0 - if (options.assertions == null) { - options.assertions = [] - } - _.defaults(callbacks, { ensureExistenceFor: 'dom', }) @@ -296,9 +293,6 @@ export const create = (Cypress: ICypress, cy: $Cy) => { const onPassFn = () => { cy.state('overrideAssert', undefined) - if (_.isFunction(callbacks.onPass)) { - return callbacks.onPass.call(this, cmds, options.assertions) - } return subject } @@ -342,8 +336,9 @@ export const create = (Cypress: ICypress, cy: $Cy) => { onFail.call(this, err, isDefaultAssertionErr, cmds) } } catch (e3) { - finishAssertions(options.assertions) throw e3 + } finally { + finishAssertions() } if (_.isFunction(onRetry)) { @@ -380,108 +375,7 @@ export const create = (Cypress: ICypress, cy: $Cy) => { .catch(onFailFn) } - let i = 0 - - const cmdHasFunctionArg = (cmd) => { - return _.isFunction(cmd.get('args')[0]) - } - const overrideAssert = function (...args) { - let cmd = cmds[i] - const setCommandLog = (log) => { - // our next log may not be an assertion - // due to page events so make sure we wait - // until we see page events - if (log.get('name') !== 'assert') { - return - } - - // when we do immediately unbind this function - cy.state('onBeforeLog', null) - - const insertNewLog = (log) => { - cmd.log(log) - - return options.assertions.push(log) - } - - // its possible a single 'should' will assert multiple - // things such as the case with have.property. we can - // detect when that is happening because cmd will be null. - // if thats the case then just set cmd to be the previous - // cmd and do not increase 'i' - // this will prevent 2 logs from ever showing up but still - // provide errors when the 1st assertion fails. - if (!cmd) { - cmd = cmds[i - 1] - } else { - i += 1 - } - - // if our command has a function argument - // then we know it may contain multiple - // assertions - if (cmdHasFunctionArg(cmd)) { - let index = cmd.get('assertionIndex') - let assertions = cmd.get('assertions') - - // https://github.com/cypress-io/cypress/issues/4981 - // `assertions` is undefined because assertions added by - // `should` command are not handled yet. - // So, don't increase i and go back to the last command. - if (!assertions) { - i -= 1 - cmd = cmds[i - 1] - index = cmd.get('assertionIndex') - assertions = cmd.get('assertions') - } - - // always increase the assertionIndex - // so our next assertion matches up - // to the correct index - const incrementIndex = () => { - return cmd.set('assertionIndex', index += 1) - } - - // if we dont have an assertion at this - // index then insert a new log - const assertion = assertions[index] - - if (!assertion) { - assertions.push(log) - incrementIndex() - - return insertNewLog(log) - } - - // else just merge this log - // into the previous assertion log - incrementIndex() - assertion.merge(log) - - // dont output a new log - return false - } - - // if we already have a log - // then just update its attrs from - // the new log - const l = cmd.getLastLog() - - if (l) { - l.merge(log) - - // and make sure we return false - // which prevents a new log from - // being added - return false - } - - return insertNewLog(log) - } - - cy.state('onBeforeLog', setCommandLog) - // send verify=true as the last arg return assertFn.apply(this, args.concat(true) as any) } @@ -537,7 +431,7 @@ export const create = (Cypress: ICypress, cy: $Cy) => { setSubjectAndSkip() - finishAssertions(options.assertions) + finishAssertions() return onPassFn() }) @@ -547,7 +441,7 @@ export const create = (Cypress: ICypress, cy: $Cy) => { // when we're told not to retry if (err.retry === false) { // finish the assertions - finishAssertions(options.assertions) + finishAssertions() // and then push our command into this err try { diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index 4f4bc2dfbf5d..ae52cbb627ed 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -24,8 +24,22 @@ export default function (Commands, Cypress, cy, state) { } const shouldFn = function (subject, chainers, ...args) { + const command = cy.state('current') + const assertionIndex = cy.state('upcomingAssertions') ? cy.state('upcomingAssertions').indexOf(command.get('currentAssertionCommand')) : 0 + let logIndex = 0 + if (_.isFunction(chainers)) { - return shouldFnWithCallback.apply(this, [subject, chainers]) + cy.state('onBeforeLog', (log) => { + logIndex++ + log.set('command', command) + log.set('commandLogId', `${assertionIndex}-${logIndex}`) + }) + + try { + return shouldFnWithCallback.apply(this, [subject, chainers]) + } finally { + cy.state('onBeforeLog', undefined) + } } let exp = cy.expect(subject).to @@ -35,6 +49,7 @@ export default function (Commands, Cypress, cy, state) { // since we are throwing our own error // without going through the assertion we need // to ensure our .should command gets logged + logIndex++ const log = Cypress.log({ name: 'should', type: 'child', @@ -58,28 +73,38 @@ export default function (Commands, Cypress, cy, state) { const isCheckingLengthOrExistence = isCheckingExistence || reHaveLength.test(chainers) const applyChainer = function (memo, value) { + logIndex++ if (value === lastChainer && !isCheckingExistence) { // https://github.com/cypress-io/cypress/issues/16006 // Referring some commands like 'visible' triggers assert function in chai_jquery.js // It creates duplicated messages and confuses users. const cmd = memo[value] - if (_.isFunction(cmd)) { - try { - return cmd.apply(memo, args) - } catch (err: any) { - // if we made it all the way to the actual - // assertion but its set to retry false then - // we need to log out this .should since there - // was a problem with the actual assertion syntax - if (err.retry === false) { - return throwAndLogErr(err) + cy.state('onBeforeLog', (log) => { + log.set('command', command) + log.set('commandLogId', `${assertionIndex}-${logIndex}`) + }) + + try { + if (_.isFunction(cmd)) { + try { + return cmd.apply(memo, args) + } catch (err: any) { + // if we made it all the way to the actual + // assertion but its set to retry false then + // we need to log out this .should since there + // was a problem with the actual assertion syntax + if (err.retry === false) { + return throwAndLogErr(err) + } + + throw err } - - throw err + } else { + return cmd } - } else { - return cmd + } finally { + cy.state('onBeforeLog', undefined) } } else { return memo[value] diff --git a/packages/driver/src/cy/commands/waiting.ts b/packages/driver/src/cy/commands/waiting.ts index b0d19ff0fd56..fef9bc0c812c 100644 --- a/packages/driver/src/cy/commands/waiting.ts +++ b/packages/driver/src/cy/commands/waiting.ts @@ -300,16 +300,14 @@ export default (Commands, Cypress, cy, state) => { if (responsesOrErr.hasSpecBridgeError) { delete responsesOrErr.hasSpecBridgeError if (options.log) { + // skip this 'wait' log since it was already added through the primary Cypress.state('onBeforeLog', (log) => { - // skip this 'wait' log since it was already added through the primary if (log.get('name') === 'wait') { // unbind this function so we don't impact any other logs cy.state('onBeforeLog', null) return false } - - return }) } diff --git a/packages/driver/src/cy/ensures.ts b/packages/driver/src/cy/ensures.ts index 481906ae0e22..274da64d40ad 100644 --- a/packages/driver/src/cy/ensures.ts +++ b/packages/driver/src/cy/ensures.ts @@ -14,8 +14,6 @@ const VALID_POSITIONS = 'topLeft top topRight left center right bottomLeft botto // they may need to work with both element arrays, or specific items // such as a single element, a single document, or single window -let returnFalse = () => false - export const create = (state: StateFunc, expect: $Cy['expect']) => { // TODO: we should probably normalize all subjects // into an array and loop through each and verify @@ -246,27 +244,14 @@ export const create = (state: StateFunc, expect: $Cy['expect']) => { } const ensureExistence = (subject) => { - returnFalse = () => { - cleanup() - - return false - } - - const cleanup = () => { - return state('onBeforeLog', null) - } - - // prevent any additional logs this is an implicit assertion - state('onBeforeLog', returnFalse) + // prevent any additional logs since this is an implicit assertion + state('onBeforeLog', () => false) // verify the $el exists and use our default error messages - // TODO: always unbind if our expectation failed try { expect(subject).to.exist - } catch (err) { - cleanup() - - throw err + } finally { + state('onBeforeLog', null) } } diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index 7346dbf178fa..af24bb445a74 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -596,24 +596,6 @@ class LogManager { log.set(options) - // if snapshot was passed - // in, go ahead and snapshot - if (log.get('snapshot')) { - log.snapshot() - } - - // if end was passed in - // go ahead and end - if (log.get('end')) { - log.end() - } - - if (log.get('error')) { - log.error(log.get('error')) - } - - log.wrapConsoleProps() - const onBeforeLog = state('onBeforeLog') // dont trigger log if this function @@ -624,13 +606,46 @@ class LogManager { } } - // set the log on the command - const current = state('current') + let command = log.get('command') - if (current) { - current.log(log) + if (!command) { + command = state('current') + log.set('command', command) } + if (command) { + let commandLogId = log.get('commandLogId') + + if (commandLogId != null) { + const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) + + if (previousLogInstance) { + log.set('snapshots', previousLogInstance.get('snapshots')) + previousLogInstance.merge(log) + + if (previousLogInstance.get('end')) { + previousLogInstance.end() + } + + return + } + } + + command.log(log) + } + + // if snapshot was passed + // in, go ahead and snapshot + if (log.get('snapshot') && !log.get('snapshots')) { + log.snapshot() + } + + if (log.get('error')) { + log.error(log.get('error')) + } + + log.wrapConsoleProps() + this.addToLogs(log) if (options.sessionInfo) { @@ -643,9 +658,8 @@ class LogManager { this.triggerLog(log) - // if not current state then the log is being run - // with no command reference, so just end the log - if (!current) { + // if the log isn't associated with a command, then we know it won't be retrying and we should just end it. + if (!state('current') || log.get('end')) { log.end() } From f8b0f40897c35c8e489e128c973bbd8cca7cea2b Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Mon, 15 Aug 2022 14:35:39 -0700 Subject: [PATCH 2/9] Remove duplicate snapshot on command failure; Prevent circular json error when inspecting commands --- packages/driver/src/cy/commands/waiting.ts | 2 ++ packages/driver/src/cy/retries.ts | 13 +------------ packages/driver/src/cypress/log.ts | 2 +- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/driver/src/cy/commands/waiting.ts b/packages/driver/src/cy/commands/waiting.ts index fef9bc0c812c..1c0a4873b2f8 100644 --- a/packages/driver/src/cy/commands/waiting.ts +++ b/packages/driver/src/cy/commands/waiting.ts @@ -308,6 +308,8 @@ export default (Commands, Cypress, cy, state) => { return false } + + return true }) } diff --git a/packages/driver/src/cy/retries.ts b/packages/driver/src/cy/retries.ts index 94ba98515f82..4f69f473298c 100644 --- a/packages/driver/src/cy/retries.ts +++ b/packages/driver/src/cy/retries.ts @@ -55,18 +55,7 @@ export const create = (Cypress: ICypress, state: StateFunc, timeout: $Cy['timeou // if our total exceeds the timeout OR the total + the interval // exceed the runnables timeout, then bail if ((total + interval) >= options._runnableTimeout) { - // snapshot the DOM since we are bailing - // so the user can see the state we're in - // when we fail - if (log) { - log.snapshot() - } - - const assertions = options.assertions - - if (assertions) { - finishAssertions(assertions) - } + finishAssertions() let onFail diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index af24bb445a74..bf1bfe41cb71 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -280,7 +280,7 @@ export class Log { toJSON () { return _ .chain(this.attributes) - .omit('error') + .omit(['error', 'command']) .omitBy(_.isFunction) .extend({ err: $errUtils.wrapErr(this.get('error')), From 70b70863c5c9d08d94dc5d846a8ebf04bc95e87c Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Mon, 15 Aug 2022 15:17:30 -0700 Subject: [PATCH 3/9] Fix for 'next' attribute in logs --- packages/driver/src/cy/assertions.ts | 4 ++-- packages/driver/src/cypress/log.ts | 18 +++++------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/packages/driver/src/cy/assertions.ts b/packages/driver/src/cy/assertions.ts index a8e72a48b113..1fee4b240db7 100644 --- a/packages/driver/src/cy/assertions.ts +++ b/packages/driver/src/cy/assertions.ts @@ -223,8 +223,8 @@ export const create = (Cypress: ICypress, cy: $Cy) => { } const finishAssertions = () => { - return _.each(cy.state('current').get('logs'), (log) => { - if (!log.get('snapshots')) { + cy.state('current').get('logs').forEach((log) => { + if (log.get('next') || !log.get('snapshots')) { log.snapshot() } diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index bf1bfe41cb71..028eaf4981a2 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -348,10 +348,10 @@ export class Log { return this } - _.defaults(options, { - at: null, - next: null, - }) + if (this.get('next')) { + name = this.get('next') + this.set('next', null) + } const snapshot = this.cy.createSnapshot(name, this.get('$el')) @@ -368,15 +368,7 @@ export class Log { this.set('snapshots', snapshots) if (options.next) { - const fn = this.snapshot - - this.snapshot = function () { - // restore the fn - this.snapshot = fn - - // call orig fn with next as name - return fn.call(this, options.next) - } + this.set('next', options.next) } return this From 7951a02dd9c0c5c90792e3c371025dac38e4f759 Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Mon, 15 Aug 2022 16:05:23 -0700 Subject: [PATCH 4/9] Add comments about logging and assertions --- packages/driver/src/cy/commands/asserting.ts | 25 +++++++++++++++++++- packages/driver/src/cypress/log.ts | 7 ++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index ae52cbb627ed..c18da3b63e8a 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -25,14 +25,37 @@ export default function (Commands, Cypress, cy, state) { const shouldFn = function (subject, chainers, ...args) { const command = cy.state('current') + + // Most commands are responsible for creating and managing their own log messages directly. + // .should(), however, is an exception - it is invoked by earlier commands, as part of + // `verifyUpcomingAssertions`. This callback can also be invoked any number of times, but we only want + // to display a few log messages (one for each assertion). + + // Therefore, we each time Cypress.log() is called, we need a way to identify: is this log call + // a duplicate of a previous one that's just being retried? This is the purpose of `commandLogId` - it should + // remain the same across multiple invocations of verifyUpcomingAssertions(). + + // It is composed of two parts: assertionIndex and logIndex. Assertion index is "which .should() command are we + // inside". Consider the following case: + // `cy.noop(3).should('be.lessThan', 4).should('be.greaterThan', 2)` + // cy.state('current') is always the 'noop' command, which rolls up the two upcoming assertions, lessThan and + // greaterThan. `assertionIndex` lets us tell them apart even though they have the same logIndex of 0 (since it + // resets each time .should() is called). + + // As another case, consider: + // cy.noop(3).should((n) => { expect(n).to.be.lessThan(4); expect(n).to.be.greaterThan(2); }) + // Here, assertionIndex is 0 for both - one .should() block generates two log messages. In this case, logIndex is + // used to tell them apart, since it increments each time Cypress.log() is called within a single retry of a single + // .should(). const assertionIndex = cy.state('upcomingAssertions') ? cy.state('upcomingAssertions').indexOf(command.get('currentAssertionCommand')) : 0 let logIndex = 0 if (_.isFunction(chainers)) { cy.state('onBeforeLog', (log) => { - logIndex++ log.set('command', command) log.set('commandLogId', `${assertionIndex}-${logIndex}`) + + logIndex++ }) try { diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index 028eaf4981a2..b4bda720ec16 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -606,12 +606,19 @@ class LogManager { } if (command) { + // See commands/asserting.ts for a detailed explanation of commandLogId. + // Short version: If onBeforeLog sets the same commandLogId as an existing log on the current command, + // then we want to merge the current instance into the existing one, rather than display a new log to + // the user. let commandLogId = log.get('commandLogId') if (commandLogId != null) { const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) if (previousLogInstance) { + // log.merge unsets any keys that aren't set on the new log instance. We + // copy over 'snapshots' beforehand so that existing snapshots aren't lost + // in the merge operation. log.set('snapshots', previousLogInstance.get('snapshots')) previousLogInstance.merge(log) From 6849134c4a4b27ba72d06e0da2112eae322361dd Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Tue, 16 Aug 2022 12:13:56 -0700 Subject: [PATCH 5/9] Fix duplicate logs on not.exist assertions --- .../cypress/e2e/commands/assertions.cy.js | 6 ++++- packages/driver/src/cy/commands/asserting.ts | 24 +++++++++---------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/packages/driver/cypress/e2e/commands/assertions.cy.js b/packages/driver/cypress/e2e/commands/assertions.cy.js index 7f8b7a69d215..7e6d62e59724 100644 --- a/packages/driver/cypress/e2e/commands/assertions.cy.js +++ b/packages/driver/cypress/e2e/commands/assertions.cy.js @@ -329,11 +329,15 @@ describe('src/cy/commands/assertions', () => { it('resolves eventually not exist', () => { const button = cy.$$('button:first') - cy.on('command:retry', _.after(2, _.once(() => { + cy.on('command:retry', _.after(3, _.once(() => { button.remove() }))) cy.get('button:first').click().should('not.exist') + + cy.then(function () { + assertLogLength(this.logs, 3) + }) }) it('resolves all 3 assertions', (done) => { diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index c18da3b63e8a..e5ce22f98a92 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -97,18 +97,18 @@ export default function (Commands, Cypress, cy, state) { const applyChainer = function (memo, value) { logIndex++ - if (value === lastChainer && !isCheckingExistence) { + cy.state('onBeforeLog', (log) => { + log.set('command', command) + log.set('commandLogId', `${assertionIndex}-${logIndex}`) + }) + + try { + if (value === lastChainer && !isCheckingExistence) { // https://github.com/cypress-io/cypress/issues/16006 // Referring some commands like 'visible' triggers assert function in chai_jquery.js // It creates duplicated messages and confuses users. - const cmd = memo[value] - - cy.state('onBeforeLog', (log) => { - log.set('command', command) - log.set('commandLogId', `${assertionIndex}-${logIndex}`) - }) + const cmd = memo[value] - try { if (_.isFunction(cmd)) { try { return cmd.apply(memo, args) @@ -126,11 +126,11 @@ export default function (Commands, Cypress, cy, state) { } else { return cmd } - } finally { - cy.state('onBeforeLog', undefined) + } else { + return memo[value] } - } else { - return memo[value] + } finally { + cy.state('onBeforeLog', undefined) } } From 8029429e8ae2fb4561607dc3776d6bdcd4248089 Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Wed, 17 Aug 2022 08:28:19 -0700 Subject: [PATCH 6/9] Move command-merging logic into asserting.ts --- packages/driver/src/cy/commands/asserting.ts | 34 +++++++++++++++---- packages/driver/src/cypress/log.ts | 35 ++------------------ 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index e5ce22f98a92..a6e0da764a76 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -7,6 +7,28 @@ import $errUtils from '../../cypress/error_utils' const reExistence = /exist/ const reHaveLength = /length/ +const onBeforeLog = (log, command, commandLogId) => { + log.set('commandLogId', commandLogId) + + if (command) { + const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) + + if (previousLogInstance) { + // log.merge unsets any keys that aren't set on the new log instance. We + // copy over 'snapshots' beforehand so that existing snapshots aren't lost + // in the merge operation. + log.set('snapshots', previousLogInstance.get('snapshots')) + previousLogInstance.merge(log) + + if (previousLogInstance.get('end')) { + previousLogInstance.end() + } + + return false + } + } +} + export default function (Commands, Cypress, cy, state) { const shouldFnWithCallback = function (subject, fn) { state('current')?.set('followedByShouldCallback', true) @@ -31,8 +53,8 @@ export default function (Commands, Cypress, cy, state) { // `verifyUpcomingAssertions`. This callback can also be invoked any number of times, but we only want // to display a few log messages (one for each assertion). - // Therefore, we each time Cypress.log() is called, we need a way to identify: is this log call - // a duplicate of a previous one that's just being retried? This is the purpose of `commandLogId` - it should + // Therefore, we each time Cypress.log() is called, we need a way to identify if this log call + // a duplicate of a previous one that's just being retried. This is the purpose of `commandLogId` - it should // remain the same across multiple invocations of verifyUpcomingAssertions(). // It is composed of two parts: assertionIndex and logIndex. Assertion index is "which .should() command are we @@ -52,10 +74,9 @@ export default function (Commands, Cypress, cy, state) { if (_.isFunction(chainers)) { cy.state('onBeforeLog', (log) => { - log.set('command', command) - log.set('commandLogId', `${assertionIndex}-${logIndex}`) - logIndex++ + + return onBeforeLog(log, command, `${assertionIndex}-${logIndex}`) }) try { @@ -98,8 +119,7 @@ export default function (Commands, Cypress, cy, state) { const applyChainer = function (memo, value) { logIndex++ cy.state('onBeforeLog', (log) => { - log.set('command', command) - log.set('commandLogId', `${assertionIndex}-${logIndex}`) + return onBeforeLog(log, command, `${assertionIndex}-${logIndex}`) }) try { diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index b4bda720ec16..f4cb2ef92338 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -280,7 +280,7 @@ export class Log { toJSON () { return _ .chain(this.attributes) - .omit(['error', 'command']) + .omit('error') .omitBy(_.isFunction) .extend({ err: $errUtils.wrapErr(this.get('error')), @@ -598,44 +598,15 @@ class LogManager { } } - let command = log.get('command') - - if (!command) { - command = state('current') - log.set('command', command) - } + let command = state('current') if (command) { - // See commands/asserting.ts for a detailed explanation of commandLogId. - // Short version: If onBeforeLog sets the same commandLogId as an existing log on the current command, - // then we want to merge the current instance into the existing one, rather than display a new log to - // the user. - let commandLogId = log.get('commandLogId') - - if (commandLogId != null) { - const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) - - if (previousLogInstance) { - // log.merge unsets any keys that aren't set on the new log instance. We - // copy over 'snapshots' beforehand so that existing snapshots aren't lost - // in the merge operation. - log.set('snapshots', previousLogInstance.get('snapshots')) - previousLogInstance.merge(log) - - if (previousLogInstance.get('end')) { - previousLogInstance.end() - } - - return - } - } - command.log(log) } // if snapshot was passed // in, go ahead and snapshot - if (log.get('snapshot') && !log.get('snapshots')) { + if (log.get('snapshot')) { log.snapshot() } From 971c272fdc3ec80602e676cc6fc695df6076866a Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Wed, 17 Aug 2022 08:50:27 -0700 Subject: [PATCH 7/9] Fix TS error --- packages/driver/src/cy/commands/asserting.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index a6e0da764a76..ef17c0acaa13 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -27,6 +27,8 @@ const onBeforeLog = (log, command, commandLogId) => { return false } } + + return true } export default function (Commands, Cypress, cy, state) { From d612806d6383f2dc4a0b12173607d4383df91016 Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Wed, 17 Aug 2022 11:58:45 -0700 Subject: [PATCH 8/9] Fix race condition with cross-origin tests --- packages/driver/src/cypress/cy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/driver/src/cypress/cy.ts b/packages/driver/src/cypress/cy.ts index 2c35c26c1f5d..002b7f5455c0 100644 --- a/packages/driver/src/cypress/cy.ts +++ b/packages/driver/src/cypress/cy.ts @@ -1185,7 +1185,7 @@ export class $Cy extends EventEmitter2 implements ITimeouts, IStability, IAssert // if this is a number, then we know we're about to insert this // into our commands and need to reset next + increment the index - if (_.isNumber(nestedIndex)) { + if (_.isNumber(nestedIndex) && nestedIndex < this.queue.length) { this.state('nestedIndex', (nestedIndex += 1)) } From 4292f808b870e273222b07da59c030cbddc580be Mon Sep 17 00:00:00 2001 From: BlueWinds Date: Mon, 22 Aug 2022 09:43:42 -0700 Subject: [PATCH 9/9] Review updates --- packages/driver/src/cy/commands/asserting.ts | 27 ++++++++++---------- packages/driver/src/cypress/log.ts | 4 +-- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/driver/src/cy/commands/asserting.ts b/packages/driver/src/cy/commands/asserting.ts index ef17c0acaa13..cdb37022ec31 100644 --- a/packages/driver/src/cy/commands/asserting.ts +++ b/packages/driver/src/cy/commands/asserting.ts @@ -10,22 +10,21 @@ const reHaveLength = /length/ const onBeforeLog = (log, command, commandLogId) => { log.set('commandLogId', commandLogId) - if (command) { - const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) - - if (previousLogInstance) { - // log.merge unsets any keys that aren't set on the new log instance. We - // copy over 'snapshots' beforehand so that existing snapshots aren't lost - // in the merge operation. - log.set('snapshots', previousLogInstance.get('snapshots')) - previousLogInstance.merge(log) - - if (previousLogInstance.get('end')) { - previousLogInstance.end() - } + const previousLogInstance = command.get('logs').find(_.matchesProperty('attributes.commandLogId', commandLogId)) + + if (previousLogInstance) { + // log.merge unsets any keys that aren't set on the new log instance. We + // copy over 'snapshots' beforehand so that existing snapshots aren't lost + // in the merge operation. + log.set('snapshots', previousLogInstance.get('snapshots')) + previousLogInstance.merge(log) - return false + if (previousLogInstance.get('end')) { + previousLogInstance.end() } + + // Returning false prevents this new log from being added to the command log + return false } return true diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts index c322e41ef6ec..b059cbbbc3c7 100644 --- a/packages/driver/src/cypress/log.ts +++ b/packages/driver/src/cypress/log.ts @@ -622,7 +622,7 @@ class LogManager { } } - let command = state('current') + const command = state('current') if (command) { command.log(log) @@ -653,7 +653,7 @@ class LogManager { this.triggerLog(log) // if the log isn't associated with a command, then we know it won't be retrying and we should just end it. - if (!state('current') || log.get('end')) { + if (!command || log.get('end')) { log.end() }