Skip to content
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

Adds a command to run performance tests across branches #22418

Merged
merged 4 commits into from
May 19, 2020

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented May 18, 2020

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

./bin/plugin/cli.js perf master v8.1.0 v8.0.0

It is also very hand when working on PRs to ensure that we're not introducing performance regressions

Capture d’écran 2020-05-18 à 2 15 38 PM

@youknowriad youknowriad self-assigned this May 18, 2020
@youknowriad youknowriad added [Type] Performance Related to performance efforts [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels May 18, 2020
const config = require( '../config' );

/**
* @typedef WPRawPerformanceResults
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth I hope you'll appreciate all these efforts :)

@github-actions
Copy link

github-actions bot commented May 18, 2020

Size Change: +275 kB (24%) 🚨

Total Size: 1.11 MB

Filename Size Change
build/annotations/index.js 3.62 kB +2 B (0%)
build/block-editor/index.js 105 kB +17 B (0%)
build/block-library/editor-rtl.css 7.22 kB +31 B (0%)
build/block-library/editor.css 7.22 kB +31 B (0%)
build/block-library/index.js 119 kB +444 B (0%)
build/blocks/index.js 48.1 kB +1 B
build/components/index.js 182 kB +125 B (0%)
build/components/style-rtl.css 17.1 kB +46 B (0%)
build/components/style.css 17.1 kB +45 B (0%)
build/compose/index.js 6.68 kB +3 B (0%)
build/core-data/index.js 11.4 kB +3 B (0%)
build/data-controls/index.js 1.29 kB -2 B (0%)
build/data/index.js 8.42 kB -4 B (0%)
build/dom-ready/index.js 568 B -1 B
build/edit-navigation/index.js 6.6 kB +4 B (0%)
build/edit-post/index.js 302 kB +274 kB (90%) 🆘
build/edit-post/style-rtl.css 12.2 kB -31 B (0%)
build/edit-post/style.css 12.2 kB -31 B (0%)
build/edit-site/index.js 12.8 kB +757 B (5%) 🔍
build/edit-widgets/index.js 7.73 kB -138 B (1%)
build/edit-widgets/style-rtl.css 4.59 kB -102 B (2%)
build/edit-widgets/style.css 4.59 kB -102 B (2%)
build/editor/index.js 44.3 kB +6 B (0%)
build/element/index.js 4.65 kB -2 B (0%)
build/hooks/index.js 2.13 kB +1 B
build/keyboard-shortcuts/index.js 2.51 kB +1 B
build/keycodes/index.js 1.94 kB +2 B (0%)
build/list-reusable-blocks/index.js 3.13 kB -3 B (0%)
build/media-utils/index.js 5.29 kB +3 B (0%)
build/notices/index.js 1.79 kB +1 B
build/nux/index.js 3.4 kB +1 B
build/plugins/index.js 2.56 kB -2 B (0%)
build/primitives/index.js 1.5 kB +1 B
build/redux-routine/index.js 2.85 kB +1 B
build/rich-text/index.js 14.8 kB +5 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 789 B 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad force-pushed the add/command-to-run-perf-tests branch from 4b44f0b to 11059a3 Compare May 18, 2020 13:16
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be documented somewhere?

bin/plugin/commands/performance.js Outdated Show resolved Hide resolved
bin/plugin/commands/performance.js Outdated Show resolved Hide resolved
bin/plugin/commands/performance.js Outdated Show resolved Hide resolved
'>> Running the test on the ' + formats.success( branch ) + ' branch'
);
const results = [];
for ( let i = 0; i < 3; i++ ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
Copy link
Member

@aduth aduth May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
bin/plugin/commands/performance.js Outdated Show resolved Hide resolved
await runShellScript( 'npm run wp-env start' );

/**
* @type {Object.<string, WPFormattedPerformanceResults>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#generic-types

Don’t ever use the types Number, String, Boolean, Symbol, or Object. 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

Suggested change
* @type {Object.<string, WPFormattedPerformanceResults>}
* @type {Record<string, WPFormattedPerformanceResults>}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bin/plugin/commands/performance.js Outdated Show resolved Hide resolved
@youknowriad youknowriad merged commit 532844f into master May 19, 2020
@youknowriad youknowriad deleted the add/command-to-run-perf-tests branch May 19, 2020 11:01
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 19, 2020
@aduth
Copy link
Member

aduth commented May 19, 2020

Should it be documented somewhere?

👀

@youknowriad
Copy link
Contributor Author

oups forgot that comment :) I'll follow-up

* @return {string} Formatted time.
*/
function formatTime( number ) {
const factor = Math.pow( 10, 2 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants