perf: pull log attributes from test instead of storing log attrs multiple times #23738
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ FlakinessThis 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 |
| @@ -181,27 +181,27 @@ export class EventManager { | |||
| }) | |||
| }) | |||
|
|
|||
| const logCommand = (logId) => { | |||
| const consoleProps = Cypress.runner.getConsolePropsForLogById(logId) | |||
| const logCommand = (logIds) => { | |||
There was a problem hiding this comment.
logIds / commandIds are pretty unclear to me - I kept reading this as "an array of log IDs" and an "array of command IDs". I think, since it has just two props, it might be better to split them out as (testId, logId) - or if not, at least rethink the variable name.
| const existing = _logsById[attrs.id] | ||
| const { instrument } = attrs | ||
|
|
||
| // test = getTestById(testId) |
There was a problem hiding this comment.
Couple of commented out lines here?
| // test = getTestById(testId) | ||
|
|
||
| // if (test) { | ||
| // pluralize the instrument as a property on the runnable |
There was a problem hiding this comment.
What's an example of 'instrument' that we're pluralizing?
There was a problem hiding this comment.
This could be command, route, hook or agent. Sessions is the odd ball instrument to date and is tied to command.
We are storing these on the test as commands, routes, agents, and hooks: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress/runner.ts#L24
# Conflicts: # packages/driver/src/cypress/runner.ts
| @@ -20,9 +20,9 @@ class Sessions extends React.Component<SessionsProps> { | |||
| } | |||
|
|
|||
| printToConsole = (name) => { | |||
| const logId = this.props.model[name].id | |||
| const { id, testId } = this.props.model[name] | |||
There was a problem hiding this comment.
does this need to be type safe?
| const { id, testId } = this.props.model[name] | |
| const { id, testId } = this.props.model[name] || {} |
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Apart of #23391
Apart of #21135
While profile Cypress to determine the memory impact we have, I noticed we are collecting all of the logs by id to easily pull the log attributes when a reporter command is hovered/clicked to show the console props in the devtools console and to show the associated snapshot.
We keep a reference of all log attributes on each test as well, which we need to store on the server-side when a test navigate to a new origin (thus we update top) and have to re-normalized the test runnables, determine where we left off in the spec run and repopulate the reporter with all of the logs for the spec.
This change updates to pull these log attributes from the test itself instead of storing these attributes again.
Some examples:
User facing changelog
Lump under overall perf updates to reduce memory footprint.
Steps to test
Since this change is related to interactions with the reporter, it's best to pull this down and verify this behavior has been maintained. I've verified in several places and everything seems to check out.
PR Tasks
cypress-documentation?type definitions?