-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Log frontend perf results in codehealth #47442
Conversation
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
Flaky tests detected in 4c0a3ae. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4014107005
|
'utf8' | ||
const resultsFiles = [ | ||
{ | ||
file: 'post-editor-performance-results.json', |
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 was trying to run this locally. I commented the parts that were about making the request to codehealth. Then, what I did was running the performance test and move the generated files to where this command expects them to be. For example, for post editor:
npm run test:performance -- --puppeteer-interactive packages/e2e-tests/specs/performance/post-editor.test.js
cp packages/e2e-tests/specs/performance/post-editor.test.results.json post-editor-performance-results.json
And similarly for the other two. Note that I had to rename the file, so I wonder if I missed another command? I guess my question is: are these paths expected?
Other than that, the command reported numbers, so it seems fine.
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, you need to run the comparison command ./bin/plugin/cli.js perf branch1 branch2
to get the right files.
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.
Left a comment about the paths https://github.com/WordPress/gutenberg/pull/47442/files#r1087679545 but other than that it seems fine.
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.
@youknowriad considering that we already removed all instances of mapKeys()
in the repository, WDYT about replacing _.mapKeys()
with Object.entries().map()
?
@tyxla sure, this is a tool though so there's no bundle size issue or anything. It would be good to start adding some lint errors whenever we get rid of some import. |
Sure, I realize that but ideally our end goal is to get rid of it entirely from the repo, otherwise, it can quickly be reintroduced if it sticks as a dependency.
We're already doing that, but for CommonJS ( |
Alternatively, for CJS we could start importing from specific modules (e.g. |
What?
In #47037 some frontend performance metrics have been added to our performance job. This PR logs the results in code health so we can track them over time.
I've did some manual tests locally, but we won't be able to test this 100% until we merge it :)