Add E2E test to test checkout performance #4344

harriswong · 2022-06-10T23:10:22Z

Changes proposed in this Pull Request

We would like to create an E2E test to measure metrics such as first paint, first contentful paint, etc. The test will run 3 times to take an average. It measures 3 things on the /checkout page:

Stripe element checkout
UPE checkout
WooPay checkout

The PR uses a similar technique as woocommerce-blocks https://github.com/woocommerce/woocommerce-blocks/blob/trunk/tests/utils/performance.ts.

Testing instructions

Follow E2E instructions here(https://github.com/Automattic/woocommerce-payments/tree/develop/tests/e2e) to setup.
Make sure your tests/e2e/deps/wcp-dev-tools folder is up-to-date. You can do git pull to make sure it is the latest.
Run npm run test:e2e-performance
You should see the test passed and a new file is created under tests/e2e/reports/checkout-performance.txt. Each line of the file is a JSON record of a performance metric. It should look something like this:

{"description":"Stripe element: Average","serverResponse":3693.6966666641333,"firstPaint":76.56499991814296,"domContentLoaded":89.84833319361012,"loaded":372.0683332843085,"firstContentfulPaint":76.56499991814296,"firstBlock":2955.526666715741}
{"description":"Stripe UPE: Average","serverResponse":4223.398333260168,"firstPaint":90.24666668847203,"domContentLoaded":123.69833327829838,"loaded":465.96499998122454,"firstContentfulPaint":90.24666668847203,"firstBlock":3817.1183334973953}
{"description":"WooPay: Average","serverResponse":4126.750000131627,"firstPaint":77.12333323433995,"domContentLoaded":95.77999993537863,"loaded":338.5383333079517,"firstContentfulPaint":77.12333323433995,"firstBlock":3170.6916666589677}

Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
Covered with tests (or have a good reason not to test in description ☝️)
Tested on mobile (or does not apply)

Post merge

'QA Testing Not Applicable'
Add or update critical flows and testing instructions for critical flows, if applicable.
Add what's changed (description, screenshot, demo videos etc.) to the release announcement post, if applicable.

harriswong · 2022-06-10T23:27:13Z

tests/e2e/utils/performance.js

@@ -0,0 +1,37 @@
+export async function getLoadingDurations() {


Similar to the function from woocommerce-blocks https://github.com/woocommerce/woocommerce-blocks/blob/trunk/tests/utils/performance.ts. The differences are that I can't destruct the performance.getEntriesByType(..) assignment or use array prototype.find. I get the following errors:
ReferenceError: _slicedToArray2
ReferenceError: _find

It could be a puppeteer problem since these are used within page.evaulate(). Our puppeteer version is 2 and it relies on puppeteer-utils. Blocks uses jest-environment-puppeteer where its puppeteer is at 14. It doesn't seem to have these issues. I think it would be good to upgrade it to the newer puppeteer version.

In any case, I worked around the ReferenceError and this function works in this branch.

Instead of using test:e2e, use test:e2e-performance. Performance test isn't needed for everyone. Splitting it up can speed up development.

harriswong · 2022-06-22T21:59:44Z

👋 @achyuthajoy @htdat I am adding an E2E test to measure /checkout performance. I want to check in with you both since you have reviewed some of the E2E PRs. Please let me know if I should ping someone else. Thanks for your time!

achyuthajoy · 2022-06-23T03:52:10Z

Hey @harriswong I'll take a look at the PR today :).

Just had a couple of questions though.

Is there a specific reason why the results are stored as a file instead of outputting to the terminal?
Is there plans to run these tests periodically via Github actions? If yes, then storing results in file might not be ideal.

It could be a puppeteer problem since these are used within page.evaulate(). Our puppeteer version is 2 and it relies on puppeteer-utils. Blocks uses jest-environment-puppeteer where its puppeteer is at 14. It doesn't seem to have these issues. I think it would be good to upgrade it to the newer puppeteer version.

We can look into upgrading puppeteer to higher version. We're also looking into upgrading the node version so wp-scripts and e2e dependencies should be a part of it.

harriswong · 2022-06-23T15:40:03Z

Is there a specific reason why the results are stored as a file instead of outputting to the terminal?

No. There isn't any specific reason to store it in a file. As long as we can get the metrics in our local dev/CI, then that should be enough for our needs. I saw woocommerce-blocks' PR (woocommerce/woocommerce-blocks#6119) which they stored in a file for output. I thought that was a pretty good idea and so I took that direction as well.

Is there plans to run these tests periodically via Github actions? If yes, then storing results in file might not be ideal.

I think it will be good to run this in Github Actions in the future, but maybe not for every PR. I was thinking it could be run manually from a trigger phrase in the comment, or per release candidate. In this case, would rendering output to the terminal be a better approach?

harriswong · 2022-06-29T21:47:55Z

I saw woocommerce-blocks' PR (woocommerce/woocommerce-blocks#6119) which they stored in a file for output. I thought that was a pretty good idea and so I took that direction as well.

GitHub actions deletes artifacts uploaded after a specific period. So if you wish to have historic data, then it's best to log it to the console, which will be preserver. Or you can send the data to another service to get it recorded.

Ok thanks! For now, I don't think we are running it in CI and we don't need the historic data. If this changes, I will pursue a different approach so that we can preserve the results. Let me know what you think.

achyuthajoy · 2022-06-30T17:51:03Z

I reverted the commit that introduced the "test:e2e-performance": "NODE_CONFIG_DIR='tests/e2e/config' wp-scripts test-e2e --config tests/e2e/config/jest.performance.config.js", command. Running npm run test:e2e will run the tests/e2e/specs/performance/payment-methods.spec.js though. Is it beneficial to use a technique similar to

woocommerce-payments/tests/e2e/utils/flows.js

Lines 45 to 52 in a661754

export const RUN_SUBSCRIPTIONS_TESTS =

'1' !== process.env.SKIP_WC_SUBSCRIPTIONS_TESTS;

export const RUN_ACTION_SCHEDULER_TESTS =

'1' !== process.env.SKIP_WC_ACTION_SCHEDULER_TESTS;

export const RUN_WC_BLOCKS_TESTS = '1' !== process.env.SKIP_WC_BLOCKS_TESTS;

so that local dev can skip the performance tests?

updates I added a commit(b4ca77c) to introduce the SKIP_WC_PERFORMANCE_TESTS env variable. Let me know if this is undesired, I can revert the commit.

@harriswong It would be much cleaner if we keep a separate jest config for performance tests. That way, you don't need to exclude the test using environment variables and keep the NPM script to run just the performance test.

What are your thoughts?

achyuthajoy · 2022-06-30T17:54:31Z

I saw woocommerce-blocks' PR (woocommerce/woocommerce-blocks#6119) which they stored in a file for output. I thought that was a pretty good idea and so I took that direction as well.

GitHub actions deletes artifacts uploaded after a specific period. So if you wish to have historic data, then it's best to log it to the console, which will be preserver. Or you can send the data to another service to get it recorded.

Ok thanks! For now, I don't think we are running it in CI and we don't need the historic data. If this changes, I will pursue a different approach so that we can preserve the results. Let me know what you think.

Sure. If historic data is not needed, writing to file or outputting to terminal works. I also noticed that terminal logs are also removed after a while.

@harriswong Do you plan to set up a Github workflow for running performance tests periodically/with manual trigger? Or just keep this for local testing.

For more accurate results I would suggest setting up a GitHub actions workflow. It's because running locally on mac with docker can be very slow and might not be the original performance.

harriswong · 2022-06-30T18:17:18Z

I reverted the commit that introduced the "test:e2e-performance": "NODE_CONFIG_DIR='tests/e2e/config' wp-scripts test-e2e --config tests/e2e/config/jest.performance.config.js", command. Running npm run test:e2e will run the tests/e2e/specs/performance/payment-methods.spec.js though. Is it beneficial to use a technique similar to

woocommerce-payments/tests/e2e/utils/flows.js

Lines 45 to 52 in a661754

export const RUN_SUBSCRIPTIONS_TESTS =

'1' !== process.env.SKIP_WC_SUBSCRIPTIONS_TESTS;

export const RUN_ACTION_SCHEDULER_TESTS =

'1' !== process.env.SKIP_WC_ACTION_SCHEDULER_TESTS;

export const RUN_WC_BLOCKS_TESTS = '1' !== process.env.SKIP_WC_BLOCKS_TESTS;

so that local dev can skip the performance tests?
updates I added a commit(b4ca77c) to introduce the SKIP_WC_PERFORMANCE_TESTS env variable. Let me know if this is undesired, I can revert the commit.

@harriswong It would be much cleaner if we keep a separate jest config for performance tests. That way, you don't need to exclude the test using environment variables and keep the NPM script to run just the performance test.

What are your thoughts?

@achyuthajoy To clarify, do you mean it is better to keep this commit a326f98?

achyuthajoy · 2022-06-30T18:21:40Z

To clarify, do you mean it is better to keep this commit a326f98

@harriswong Yes. That would be best to keep the performance test separate from current E2E.

Was this commit removed based on our previous discussion? If yes, I'm so sorry, I might've missed something while checking the implementation.

harriswong · 2022-06-30T18:24:52Z

I saw woocommerce-blocks' PR (woocommerce/woocommerce-blocks#6119) which they stored in a file for output. I thought that was a pretty good idea and so I took that direction as well.

GitHub actions deletes artifacts uploaded after a specific period. So if you wish to have historic data, then it's best to log it to the console, which will be preserver. Or you can send the data to another service to get it recorded.

Ok thanks! For now, I don't think we are running it in CI and we don't need the historic data. If this changes, I will pursue a different approach so that we can preserve the results. Let me know what you think.

Sure. If historic data is not needed, writing to file or outputting to terminal works. I also noticed that terminal logs are also removed after a while.

@harriswong Do you plan to set up a Github workflow for running performance tests periodically/with manual trigger? Or just keep this for local testing.

For more accurate results I would suggest setting up a GitHub actions workflow. It's because running locally on mac with docker can be very slow and might not be the original performance.

@achyuthajoy Thanks for the suggestion! I wasn't planning on setting up a GitHub action workflow as of yet. I was planning to run this locally so that we can measure a before-and-after effect on this other PR(#3968). We also don't know if we will be going forward with #3968. But you are right that the results may vary from machine to machine. Maybe creating a manual trigger is better. Even though #3968 is in draft, I can see some benefits in triggering it manually in the PR's comment. This way, the metric will be more consistent compared to each of our individual machines. I will look into adding a github action to this.

Can this be a follow-up or would you prefer to have these all in the same PR?

achyuthajoy · 2022-06-30T18:25:55Z

Can this be a follow-up or would you prefer to have these all in the same PR?

The GH workflow can be another PR.

harriswong · 2022-06-30T18:27:56Z

To clarify, do you mean it is better to keep this commit a326f98

@harriswong Yes. That would be best to keep the performance test separate from current E2E.

Was this commit removed based on our previous discussion? If yes, I'm so sorry, I might've missed something while checking the implementation.

No problem at all! git makes these super easy to do. Thanks for taking the time to review! 🙇 I do appreciate the feedback on this. Ok, I will bring back the a326f98 and revert the commit that introduces SKIP_WC_PERFORMANCE_TESTS. 👍

This reverts commit b4ca77c.

This reverts commit 4e5e2d0.

harriswong · 2022-07-01T00:09:11Z

Can this be a follow-up or would you prefer to have these all in the same PR?

The GH workflow can be another PR.

Added follow up here: #4389

harriswong · 2022-07-01T00:18:40Z

No problem at all! git makes these super easy to do. Thanks for taking the time to review! 🙇 I do appreciate the feedback on this. Ok, I will bring back the a326f98 and revert the commit that introduces SKIP_WC_PERFORMANCE_TESTS. 👍

Done. Running npm run test:e2e-performance should print something like this:

@achyuthajoy please take another look.

achyuthajoy · 2022-07-05T14:28:16Z

tests/e2e/utils/flows.js

@@ -490,4 +490,62 @@ export const merchantWCP = {
 			await checkbox.click();
 		}
 	},
+
+	activateWooPay: async () => {


Ideally we shouldn't write flows for toggling Dev Tools flag and use WP-CLI instead. This can be ignored for now since I've opened an issue to address these - #4402

cli would be great! It will make it a lot faster! Thanks for the note.

achyuthajoy · 2022-07-05T14:33:25Z

tests/e2e/specs/performance/payment-methods.spec.js

+		recreatePerformanceFile();
+	} );
+
+	describe( 'Stripe element', () => {


@harriswong It would be best if the test spec contains only 1 describe group and multiple it groups. Or you could split the tests to multiple files which I don't think is required in this case.

Let me know your thoughts.

The reason I did this was because I wanted to have a clean file per performance run. At the same time, I wanted each scenario (Stripe element, UPE, WooPay without UPE, etc) to have its own setup. As a result, I did

describe // first level to always wipe the file before we start a run describe // Stripe element setup describe // UPE setup describe // WooPay without UPE setup

Looking at this again, I wonder if I can mix beforeAll and beforeEach with 1 layer of describe, such that wiping the files execute only once. 👀

@harriswong I don't have a strong opinion here. I'm good to go with your approach.

The other thing I could do is to get rid of all the beforeEach and afterEach. That's because there is only 1 test case for now, all the setup and teardown can be part of the it(). Then, I can have just 1 describe, is this preferred or should I keep the nested describe code here?

achyuthajoy

@harriswong I've tested the changes locally and can confirm that it's working a expected. Awesome work on this one.

I've left a few comments. Once it's cleared, it's good to merge 👍

achyuthajoy

@harriswong I noticed that the report file created is not added to gitignore. Can you add it?

Also, tested the changes on a linux machine against a local server instance. The results are much faster that running on a Mac.

harriswong · 2022-07-11T22:41:59Z

Hi @achyuthajoy , I merged the latest develop and added a line to the .gitignore. Please take a look again!

Also, tested the changes on a linux machine against a local server instance. The results are much faster that running on a Mac.

Thanks for giving this a test! The power of linux 💪

achyuthajoy

LGTM 🚢

Thanks for your awesome work on this 👍

harriswong · 2022-07-12T21:46:56Z

@achyuthajoy The "PHP testing (7.1)" job is stuck but this recent PR https://github.com/Automattic/woocommerce-payments/pull/4425/files removed 7.1

Should we ignore this stuck test and merge?

achyuthajoy · 2022-07-13T05:45:23Z

@harriswong Yes, please ignore the 7.1 test. It was removed when WP & WC versions were bumped as part of the next release

harriswong added 2 commits June 9, 2022 14:57

Add a new performance test suite inside e2e folder

dac8544

Use page.evaulate() via puppeteer to output performance metrics

51936c0

harriswong commented Jun 10, 2022

View reviewed changes

harriswong added 5 commits June 13, 2022 12:58

Run the test 3 times and then calculate average.

60767d1

Merge branch 'develop' into add/1021-performance-measure

d474d08

Add performance test when UPE is enabled

9a85b29

Merge branch 'develop' into add/1021-performance-measure

3198b96

Add performance test for WooPay without UPE

d9b113c

harriswong changed the title ~~POC: Add performance metrics~~ WIP: Add performance metrics Jun 16, 2022

harriswong added 7 commits June 16, 2022 17:32

Save performance metrics to a local file

338ce18

Move logic out from the test spec

7bc32db

Merge branch 'develop' into add/1021-performance-measure

746274d

Update woopay selector name from the dev tool

56cda30

Add changelog

6d2c2ae

Merge branch 'develop' into add/1021-performance-measure

e8ae1ee

Make performance its own npm command

a326f98

Instead of using test:e2e, use test:e2e-performance. Performance test isn't needed for everyone. Splitting it up can speed up development.

harriswong changed the title ~~WIP: Add performance metrics~~ Add E2E test to test checkout performance Jun 22, 2022

harriswong added category: e2e Issues and PRs related to e2e tests. category: performance The issue/PR is related to performance. labels Jun 22, 2022

harriswong marked this pull request as ready for review June 22, 2022 21:51

harriswong assigned brettshumaker Jun 22, 2022

harriswong requested review from htdat and achyuthajoy June 22, 2022 21:55

harriswong unassigned brettshumaker Jun 22, 2022

harriswong requested a review from brettshumaker June 22, 2022 21:55

harriswong self-assigned this Jun 22, 2022

Merge branch 'develop' into add/1021-performance-measure

a79bc45

harriswong added 3 commits June 30, 2022 17:57

Merge branch 'develop' into add/1021-performance-measure

f71615a

Revert "Add a local env variable allowing us to skip performance tests"

5ef16e4

This reverts commit b4ca77c.

Revert "Revert "Make performance its own npm command""

6240c57

This reverts commit 4e5e2d0.

harriswong mentioned this pull request Jul 1, 2022

Create a github workflow for performance test #4389

Open

achyuthajoy reviewed Jul 5, 2022

View reviewed changes

achyuthajoy requested changes Jul 5, 2022

View reviewed changes

Merge branch 'develop' into add/1021-performance-measure

504274b

achyuthajoy self-requested a review July 8, 2022 07:18

achyuthajoy requested changes Jul 8, 2022

View reviewed changes

harriswong added 2 commits July 11, 2022 16:38

Merge branch 'develop' into add/1021-performance-measure

e93354d

Ignore performance tests via .gitignore

4dbd8ed

harriswong requested a review from achyuthajoy July 11, 2022 22:42

achyuthajoy approved these changes Jul 12, 2022

View reviewed changes

Merge branch 'develop' into add/1021-performance-measure

88e0e9a

achyuthajoy merged commit 8074238 into develop Jul 13, 2022

achyuthajoy deleted the add/1021-performance-measure branch July 13, 2022 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add E2E test to test checkout performance #4344

Add E2E test to test checkout performance #4344

harriswong commented Jun 10, 2022 •

edited

Loading

harriswong Jun 10, 2022

harriswong commented Jun 22, 2022

achyuthajoy commented Jun 23, 2022

harriswong commented Jun 23, 2022

harriswong commented Jun 29, 2022

achyuthajoy commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

harriswong commented Jul 1, 2022

harriswong commented Jul 1, 2022

achyuthajoy Jul 5, 2022

harriswong Jul 5, 2022

achyuthajoy Jul 5, 2022

harriswong Jul 5, 2022

achyuthajoy Jul 5, 2022

harriswong Jul 5, 2022

achyuthajoy left a comment

achyuthajoy left a comment

harriswong commented Jul 11, 2022

achyuthajoy left a comment

harriswong commented Jul 12, 2022

achyuthajoy commented Jul 13, 2022

		@@ -0,0 +1,37 @@
		export async function getLoadingDurations() {

Add E2E test to test checkout performance #4344

Add E2E test to test checkout performance #4344

Conversation

harriswong commented Jun 10, 2022 • edited Loading

Changes proposed in this Pull Request

Testing instructions

harriswong Jun 10, 2022

Choose a reason for hiding this comment

harriswong commented Jun 22, 2022

achyuthajoy commented Jun 23, 2022

harriswong commented Jun 23, 2022

harriswong commented Jun 29, 2022

achyuthajoy commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

achyuthajoy commented Jun 30, 2022

harriswong commented Jun 30, 2022

harriswong commented Jul 1, 2022

harriswong commented Jul 1, 2022

achyuthajoy Jul 5, 2022

Choose a reason for hiding this comment

harriswong Jul 5, 2022

Choose a reason for hiding this comment

achyuthajoy Jul 5, 2022

Choose a reason for hiding this comment

harriswong Jul 5, 2022

Choose a reason for hiding this comment

achyuthajoy Jul 5, 2022

Choose a reason for hiding this comment

harriswong Jul 5, 2022

Choose a reason for hiding this comment

achyuthajoy left a comment

Choose a reason for hiding this comment

achyuthajoy left a comment

Choose a reason for hiding this comment

harriswong commented Jul 11, 2022

achyuthajoy left a comment

Choose a reason for hiding this comment

harriswong commented Jul 12, 2022

achyuthajoy commented Jul 13, 2022

harriswong commented Jun 10, 2022 •

edited

Loading