Adds a command to run performance tests across branches#22418
Adds a command to run performance tests across branches#22418youknowriad merged 4 commits intomasterfrom
Conversation
youknowriad
added
[Type] Performance
bin/plugin/commands/performance.js
Outdated
There was a problem hiding this comment.
@aduth I hope you'll appreciate all these efforts :)
|
Size Change: +275 kB (24%) 🚨 Total Size: 1.11 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
add/command-to-run-perf-tests
branch
from
4b44f0b to
11059a3
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Should it be documented somewhere?
| '>> Running the test on the ' + formats.success( branch ) + ' branch' | ||
| ); | ||
| const results = []; | ||
| for ( let i = 0; i < 3; i++ ) { |
There was a problem hiding this comment.
Any significance to three iterations? Or is it just an arbitrary number to try toward some confidence of trusting the output? I guess it depends for what reasons we would expect a "bad" output, but it also occurs to me that there's no randomness or mixing of runs between branches (i.e. A A A B B B C C C instead of A B C A B C A B C or A C B B A C C B A).
Might also be something to extract as a named / documented constant, to clarify all of the above points to the next maintainer.
There was a problem hiding this comment.
Or is it just an arbitrary number to try toward some confidence of trusting the output?
This is the reason, I think cache could create wrong numbers that's why I trust multiple runs better.
For the order, I agree that it's better to switch but the problem is that it will make the command a lot slower unless we have separate wp-env environments entirely. I might consider it separately.
| log( '>> Installing dependencies' ); | ||
| // The build packages is necessary for the performance folder | ||
| await runShellScript( | ||
| 'npm install && npm run build:packages', |
There was a problem hiding this comment.
Note: Depending if you're trying to be as efficient as possible, note that build:packages also runs TypeScript. Running ./bin/packages/build.js would be the most direct (example).
Related previous discussion about incorporating TypeScript in build at #18942 (comment).
bin/plugin/commands/performance.js
Outdated
| await runShellScript( 'npm run wp-env start' ); | ||
|
|
||
| /** | ||
| * @type {Object.<string, WPFormattedPerformanceResults>} |
There was a problem hiding this comment.
When the name of the key for an object provides hints for developers what to do like setting, use indexable interface. If not, use Record.
Don’t ever use the types
Number,String,Boolean,Symbol, orObject. These types refer to non-primitive boxed objects that are almost never used appropriately in JavaScript code.
https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html
| * @type {Object.<string, WPFormattedPerformanceResults>} | |
| * @type {Record<string, WPFormattedPerformanceResults>} |
There was a problem hiding this comment.
Found this here btw https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
There was a problem hiding this comment.
Found this here btw https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
Huh, that's interesting, and seemingly contradictory to their own documentation. Unless the difference is in specifying the generic's types. I have some recollection that using Object leads to a lot of scenarios where the type becomes treated as equivalent to a (largely useless) any type.
👀 |
|
oups forgot that comment :) I'll follow-up |
| * @return {string} Formatted time. | ||
| */ | ||
| function formatTime( number ) { | ||
| const factor = Math.pow( 10, 2 ); |
There was a problem hiding this comment.
I'm guessing there's a good reason we're calling Math.pow( 10, 2 ) instead of simply using 100 but I can't figure out what it might be. Any hints @youknowriad?
There was a problem hiding this comment.
also I see this is called formatTime but it appears to be applied to a number of different data types and it appears to be used as a way of converting to a fixed number of decimal places.
in trunk the ' ms' suffix is gone, moved to where we store this data in a JSON file. I think it's currently the equivalent of this, but I'm confused because of the disparity between what it's called and what it does.
mapValues( medians, n => Math.round( 100 * n ) / 100 )Though to be honest if this is all we're doing then would you find it wrong to leave the numbers as they are and then run .toFixed(2) at the place where we print them? I think that could make the intention more clear (if our goal is simply to limit to a fixed number of decimal places when we print these out) and leave the data more raw along the way.
There was a problem hiding this comment.
the intention more clear (if our goal is simply to limit to a fixed number of decimal places when we print these out) and leave the data more raw along the way.
I think that's probably the intent here yeah.
3 participants
When preparing the bi-weekly release posts, we need to run the performance tests across multiple versions. The process for that is not very well documented and can change from one person to another.
This PR adds a command to the Plugin CLI tool where you provide a list of branches/tags/commits and it will run the performance tests in each one of these branches and outputs the result as a table.
Example
It is also very hand when working on PRs to ensure that we're not introducing performance regressions