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

Performance: add e2e tests for load and typing time #14506

Merged
merged 7 commits into from May 28, 2019

Conversation

@ellatrix
Copy link
Member

commented Mar 18, 2019

Description

Adds some simple tests to report the average load and typing times.

Example:

Average time to load: 4889.1ms
Average time to DOM content load: 4034.9ms
Average time to type character: 67.165ms
Slowest time to type character: 140ms
Fastest time to type character: 50ms

Why? It is more accurate than trying to record key presses at random places in a big post. It's also possible to test with different content. Note that speed can still vary depending on a number of environmental factors, but at least some things are now constant.

The tests are done with a 1000 paragraph post, where each paragraph contains 200 characters. Maybe additional tests should be made with more diverse content, but I think this is a good start.

For load times, 10 samples are taken. For typing speed 200 samples are taken (typing one full paragraph).

The typing time includes everything Puppeteer is executing during page.keyboard.type, so, in reality, it is probably less. We can try to look at keyup and keydown events instead.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Is there a way we could automate this across versions. Like have a shell script that checkout old tags, runs this test and produce a table at the end?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Can we test in a post with images, quotes... like this one for instance https://waitbutwhy.com/2017/04/neuralink.html

} from '@wordpress/e2e-test-utils';

describe( 'Performance', async () => {
it( '1000 paragraphs', async () => {

This comment has been minimized.

Copy link
@gziolo

gziolo Mar 19, 2019

Member

Do we want it to be executed every time we run e2e tests?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 19, 2019

Author Member

No, let's exclude.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@youknowriad I tried checking out other tags and running the tests, the only problem is that the tests don't exist in the working directory then. Not sure how to solve it. I can try copying the file, then writing it for each tag.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@ellatrix I was thinking we use the exact same test in several checkouts:

  • Keep the current set of tests (master)
  • Checkout the tags in a different temporary folder
  • Prepare the environment (reinstall packages, rebuild...)
  • Run the current set of tests on this environment.
@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

I'm slowly giving up on this :(

Always keep running into vague errors

Validation Error:

Options: setupTestFrameworkScriptFile and setupFilesAfterEnv cannot be used together.
Please change your configuration to only use setupFilesAfterEnv.

Configuration Documentation:
https://jestjs.io/docs/configuration.html

What I was trying:

#!/bin/bash

# Create temporary directory to write results.
rm -rf temp
mkdir temp

# master
# npm run test-e2e packages/e2e-tests/specs/performance.test.js
#

versions="5.2.0 5.1.0 5.0.0 4.9.0"
branch=$(git branch | grep \* | cut -d ' ' -f2)
cp packages/e2e-tests/specs/performance.test.js temp/performance.test.js

git stash

# Iterate the string variable using for loop
for version in $versions; do
	git checkout "v$version" -q
	cp temp/performance.test.js packages/e2e-tests/specs/performance.test.js
	echo "Running tests against $version"
	npm run test-e2e packages/e2e-tests/specs/performance.test.js
	git reset --hard -q
	git clean -fq
done

git checkout $branch -q
git stash pop -q
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

It's a good start. Other persons could chime-in and try to improve things.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

master:

Average time to load: 4770.4ms
Average time to DOM content load: 3960.8ms
Average time to type character: 60.96ms
Slowest time to type character: 130ms
Fastest time to type character: 47ms

5.2.0:

Average time to load: 4742.7ms
Average time to DOM content load: 3928.4ms
Average time to type character: 63.07ms
Slowest time to type character: 132ms
Fastest time to type character: 48ms

5.1.0:

Average time to load: 5851.9ms
Average time to DOM content load: 4299.9ms
Average time to type character: 42.21ms
Slowest time to type character: 127ms
Fastest time to type character: 30ms

5.0.0:

Average time to load: 5647.9ms
Average time to DOM content load: 4110.5ms
Average time to type character: 46.09ms
Slowest time to type character: 87ms
Fastest time to type character: 38ms

4.9.0:

Average time to load: 7290.7ms
Average time to DOM content load: 5250.8ms
Average time to type character: 49.08ms
Slowest time to type character: 88ms
Fastest time to type character: 37ms
@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

Had to tweak some files to test the other versions:

4.8.0:

Average time to load: 7445.2ms
Average time to DOM content load: 5752.4ms
Average time to type character: 129.305ms
Slowest time to type character: 264ms
Fastest time to type character: 110ms

4.7.0:

Average time to load: 8772.5ms
Average time to DOM content load: 6898.1ms
Average time to type character: 136.305ms
Slowest time to type character: 287ms
Fastest time to type character: 120ms
@aduth

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Re: #14506 (comment)

I'd imagine the warning may be a result of the recent Jest version upgrade, since your script does not account for the need to run npm install after checking out the tag.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@aduth Thanks :) I figured it out while manually checking out the tags and running the tests. Still, the whole idea is kind of flawed, as it only works back to 4.9.0. Earlier there wasn't a separate e2e package. It would be better to do something like @youknowriad suggested, but it doesn't seem like a small task. The current PR is still useable though, we can run it for every version we release. I ran it back until 4.7.0 as you can see above. the results look good: 4.7.0 was very slow, we made some small improvements for 4.8.0, then 4.9.0 is significantly faster thanks to @youknowriad's PR, performance decreased again with 5.2.0, but master is still similar (no big increase in the coming 5.3.0 release).

@nerrad

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

I wonder if there'd be some value in having a job dedicated to performance regression in travis? So for instance if the variance is +100ms or something then the job would fail. That might help catch pulls that significantly impact performance? I'm not sure offhand how this would work, but presumably there would need to be some service that marks/saves the current performance metrics on the master branch as the baseline for pulls (then maybe those metrics could be a simple json object saved to a file in the repo? - or amazon s3)?

(note, I'm not suggesting my idea block this pull getting merged, more of an iteration that might be something that could be explored).

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@nerrad Yeah, that would be cool. It would require the performance test to run all the time as well, probably best as a separate job. We can keep a file with the times to test against, hopefully lowering them as performance improvements are made.

@gziolo

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I can help with the follow-up PR if we decide to integrate it with Travis. We would have to find a way to put this benchmark into its own Jest run (maybe with projects feature?).

@ellatrix ellatrix force-pushed the try/test-perf branch from ffbe6e2 to dacf4b1 Apr 1, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

These branches can be used for 4.7 and 4.8:

v4.7.0...test/performance-4-7
v4.8.0...test/performance-4-8

They require the files to be moved. You can't just stash and checkout a different release.

Would be good to create a solution that can just install any Gutenberg version and run the tests, but at the moment I like to spend my time on other things (I don't think it's a trivial thing to do). Anyone is free to try of course.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

I pushed a commit to be able to run the performance test using a simple CLI script

npm run test-performance

I think we should merge this branch.

My last question is should we keep the "neuralink" post or replace it with a similar lipsum post in terms of content/blocks (thinking about copyright)

@gziolo
Copy link
Member

left a comment

It looks good, there might be one tweak necessary to the existing e2e test config

testPathIgnorePatterns: [
'e2e-tests/specs/performance.test.js',
],
reporters: [

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 16, 2019

Member

Should this reporter to be included in here?

@aduth

This comment has been minimized.

Copy link
Member

commented May 1, 2019

They require the files to be moved. You can't just stash and checkout a different release.

If I understand the issue here to be the need to stash new files, you can pass the -u flag.

git stash -u

https://stackoverflow.com/questions/835501/how-do-you-stash-an-untracked-file

ellatrix and others added some commits Apr 3, 2019

@youknowriad youknowriad force-pushed the try/test-perf branch from e1c05b2 to a50569a May 24, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

I think we should merge this branch. I keep looking for it to run performance tests.

@gziolo

gziolo approved these changes May 24, 2019

Copy link
Member

left a comment

Looks good, let's get it in 👍

@@ -9,4 +9,7 @@ module.exports = {
'@wordpress/jest-puppeteer-axe',
'expect-puppeteer',
],
testPathIgnorePatterns: [
'e2e-tests/specs/performance.test.js',

This comment has been minimized.

Copy link
@gziolo

gziolo May 24, 2019

Member

Can we put it into its own folder to avoid this ignore rule?

e2e-tests/benchmark/index.js - should do the trick

@youknowriad youknowriad referenced this pull request May 24, 2019

Merged

@wordpress/data: Introduce useSelect custom hook. #15737

11 of 12 tasks complete
@aduth

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I've pushed an alternate test fixture document, ideally roughly equivalent in stress testing a variety of different contents (headers, lists, quotes, paragraphs, images, inline formats), randomly generated with a combination of loripsum.net and manual insertion of images from placeholder.com.

  Words Headings Paragraphs Blocks
Before 36396 0 840 1065
After 38819 202 547 952
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

I noticed that with the recent changes, running the performance tests is a lot slower even if the results are similar. I wonder if it's because of the time needed to load the images. I don't think this should stop us from landing the PR but it would be good to mock these or load local images.

@youknowriad youknowriad merged commit b14d70c into master May 28, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the try/test-perf branch May 28, 2019

@aduth

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.