dev: perf regression test configuration #13194

hannojg · 2022-11-30T17:33:22Z

Details

This PR is based on:

Run e2e performance regression tests on merge into main via AWS Device farm #12320.

The changes in #12320 made running the performance regression tests locally quite difficult. This PR fixes this.
In addition this PR has the aim, to reduce the runtime of the performance regression tests, so that developers can run those tests locally.
Its important that developers don't get blocked for a long time, or otherwise no-one will run the tests locally, although they provide value and can tell the developer early on if he/she messed up somewhere.

The time reduction is accomplished by two things:

You can now have a custom config that you specify when running the tests. The config for local has fewer test iterations set
When the changes are only in JS (or the native changes are irrelevant) the native app now only needs to get build once, instead of twice. When passing --buildMode js-only to the CLI it will just re-bundle the JS code into the existing native build.

This is in an effort to make it more easy for devs to compare their changes performance wise.

In addition to that you can now also choose to just run specific tests, by using the --includes option:

nom run test:e2e --includes "login|signup"

Note: The changes of this PR are a bit hard to get, as it's based on another un-merged PR. I will mark this PR as draft, and once the original PR got merged, I will mark this PR as ready.

Fixed Issues

$ #11711
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Try to run the e2e tests locally with:

npm run test:e2e -- --development

Offline tests

Not needed

QA Steps

No QA needed, internal dev tooling

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…/perf-regression-test-configuration

Now you can either rebuild the native app fully, skip it entirely, or just rebundle the JS bundle

AndrewGable · 2022-12-05T21:09:08Z

PR was merged that this was based off of - Happy to review when this is ready!

…/perf-regression-test-configuration

hannojg · 2022-12-06T18:26:24Z

@AndrewGable Cool, I think its good for a review now 💪

melvin-bot · 2022-12-06T18:26:41Z

@NikkiWines Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Szymon20000 · 2022-12-08T13:12:00Z

Sorry if I'm missing sth obvious but I'm wondering how we can only change js If we compare 2 different branches.
Looks like we use the same apk path and build directory is not a part of git (or do we somehow add it temporarily?) .

Also if we can have multiple tests right now is it a problem that main can have a different list of tests than currently testing branch?
Shouldn't we checkout back to previous branch right after building apk?

Do you think it would be easier to instead of checking-out to a different branch we could clone different branch to and place it somewhere within the repo? For instance repo/.compareBranch/cloned_repo

Szymon20000 · 2022-12-08T13:14:38Z

I would also add a flag --development mode or sth that only runs test once and only using the current branch. The purpose would be to speed up writing new tests.

hannojg · 2022-12-09T09:55:34Z

Also if we can have multiple tests right now is it a problem that main can have a different list of tests than currently testing branch?
Shouldn't we checkout back to previous branch right after building apk?

yeah that's definitely an interesting point. We can check back after building the app. The tests are bundled inside the app, so when building the app on main the tests will be missing either way. I think for that case we can't compare anything, and just shouldn't submit any test results.

Sorry if I'm missing sth obvious but I'm wondering how we can only change js If we compare 2 different branches.

This is an optional build option. I think for the --development mode it would be handy. Then you don't need to rebuild the whole app just because you made some changes to the test on the JS side.

Do you think it would be easier to instead of checking-out to a different branch we could clone different branch to and place it somewhere within the repo? For instance repo/.compareBranch/cloned_repo

Not really, I don't see how that would help? 🤔 if there is a difference in the tests, the tests would still be missing e.g. in the cloned repo.

Szymon20000 · 2022-12-09T11:01:10Z

No really, I don't see how that would help? 🤔 if there is a difference in the tests, the tests would still be missing e.g. in the cloned repo.

Then you would have access to new tests and can build the apk on the main branch using new tests. It could just copy tests to cloned repo/e2e/sth.
If a test is new it doesn't mean we can't run it against main. For Instance I can image someone could compare release candidate to previously released version.

Szymon20000 · 2022-12-09T11:02:34Z

At the moment adding test is a bit hard because:

you need to commit your changes otherwise it can't checkout to main or other base branch
new test is not part of the base branch

Szymon20000 · 2022-12-09T11:06:08Z

Actually cloning can be slow but I can image we could just copy e2e directory to a new dir that is not part of git so it will stay there even when we checkout.

hannojg · 2022-12-12T13:19:56Z

Meeting with Szymon:

Adding tests (or testing newly added tests) need to be simpler with a development flag
- Run tests on the same branch
- Run tests against main (copy new e2e tests)
- Run fewer iterations (like 1)
- Update documentation

…/perf-regression-test-configuration

AndrewGable · 2022-12-13T23:02:14Z

Looking great, let me know when this is ready for review. Can you link #11711 in the fixed issues? Thanks!

…/perf-regression-test-configuration

hannojg · 2022-12-15T17:22:34Z

Hey @AndrewGable, I think this should be good now for review 😊 👍

AndrewGable

Looking good! Just one question.

tests/e2e/testRunner.js

…/perf-regression-test-configuration

AndrewGable · 2022-12-28T23:38:34Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

N/A - Test changes, no source code changes.

OSBotify · 2022-12-28T23:44:53Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2022-12-29T00:04:30Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	821.586 ms → 827.491 ms (+5.905 ms, +0.7%)
runJsBundle	190.871 ms → 195.438 ms (+4.567 ms, +2.4%)
regularAppStart	0.015 ms → 0.015 ms (+0.000 ms, +2.6%)
nativeLaunch	9.938 ms → 9.871 ms (-0.067 ms, -0.7%)

Show details

Name	Duration
TTI	Baseline Mean: 821.586 ms Stdev: 32.031 ms (3.9%) Runs: 761.0569910001941 766.9312200001441 775.045167000033 782.79841999989 790.1830310001969 792.9966359999962 793.7100920001976 795.8917419998907 797.3591889999807 803.8159230002202 805.3751790001988 808.7190829999745 811.2859729998745 811.7989810002036 813.628289999906 821.573379999958 826.7494740001857 834.4231819999404 835.9500609999523 836.3768020002171 837.8203879999928 846.5568039999343 847.130189999938 855.0000089998357 856.9196739997715 860.7382370000705 862.0918580000289 862.2607559999451 866.8139980002306 886.5905059999786 Current Mean: 827.491 ms Stdev: 32.508 ms (3.9%) Runs: 769.4002359998412 770.5122130000964 775.0457560000941 782.7277049999684 788.8380430000834 793.4789339997806 795.3998619997874 804.7510330001824 806.1375730000436 808.832409999799 811.7107159998268 816.275588999968 819.6946350000799 822.3168850000948 833.3617320000194 833.8771810000762 836.6929049999453 841.067826999817 845.1441640001722 847.2845549997874 848.5182559997775 848.6278909998946 853.606190000195 854.7940719998442 855.5519770001993 859.5936320000328 864.1725369999185 864.4949320000596 882.9929680000059 889.8347499999218
runJsBundle	Baseline Mean: 190.871 ms Stdev: 17.229 ms (9.0%) Runs: 164 167 168 171 172 173 176 178 179 180 183 183 184 185 185 186 187 188 192 196 200 202 202 205 206 209 210 217 218 219 232 Current Mean: 195.438 ms Stdev: 23.123 ms (11.8%) Runs: 160 162 165 168 169 169 174 176 177 181 182 184 188 188 190 195 196 196 198 198 206 207 207 211 215 215 216 217 220 222 246 256
regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (5.9%) Runs: 0.01346899988129735 0.013549999799579382 0.013630999717861414 0.013753999955952168 0.013793999794870615 0.01399700017645955 0.014078999869525433 0.014200999867171049 0.014200999867171049 0.014282000251114368 0.014403999783098698 0.014405000023543835 0.014567000325769186 0.014567000325769186 0.014648000244051218 0.014812000095844269 0.014851999934762716 0.014973999932408333 0.01501499954611063 0.015054999850690365 0.01509599993005395 0.015176999848335981 0.015299000311642885 0.015381000004708767 0.015625 0.015829000156372786 0.015868999995291233 0.016234999988228083 0.016439000144600868 0.01696800021454692 Current Mean: 0.015 ms Stdev: 0.001 ms (4.8%) Runs: 0.013671999797224998 0.014078999869525433 0.014362999703735113 0.014486000407487154 0.01464799977838993 0.01464899955317378 0.014649000018835068 0.0147299999371171 0.014771000016480684 0.014811000321060419 0.014851000159978867 0.014851999934762716 0.014932999853044748 0.014932999853044748 0.015054999850690365 0.015055000316351652 0.015056000091135502 0.015135999768972397 0.015137000009417534 0.015217999927699566 0.015299000311642885 0.015380000229924917 0.015420999843627214 0.01578700030222535 0.015868999995291233 0.015910000074654818 0.016195000149309635 0.0163569999858737 0.016478999983519316 0.016479999758303165 0.016764999832957983
nativeLaunch	Baseline Mean: 9.938 ms Stdev: 1.767 ms (17.8%) Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 12 12 12 12 13 14 15 Current Mean: 9.871 ms Stdev: 1.338 ms (13.6%) Runs: 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 11 11 11 11 12 12 12 12 13

OSBotify · 2022-12-29T20:24:02Z

🚀 Deployed to staging by @AndrewGable in version: 1.2.46-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-01-02T17:42:22Z

🚀 Deployed to production by @roryabraham in version: 1.2.46-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

hannojg added 13 commits November 30, 2022 11:53

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

049d346

…/perf-regression-test-configuration

feat: load custom config

aa53108

fix gitignore of e2e results

0a67506

fix path of test results

1f6f516

make error handling more resilient

d98e7b7

fix: running perf regression tests locally

f6fd71c

fix: results not calculated correctly due to runs not devidable by 4

95fa715

feat: add option to filter tests to run by --includes flag

759b144

doc: CLI options

f33026e

doc: env var options

55bdf10

add script for repackaging e2e release app

5d5d1d5

feat: add buildMode option

17e4485

Now you can either rebuild the native app fully, skip it entirely, or just rebundle the JS bundle

add tip notice on how to run tests faster

706967e

AndrewGable self-requested a review December 5, 2022 21:08

hannojg added 2 commits December 6, 2022 19:23

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

521214d

…/perf-regression-test-configuration

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

f3aa173

…/perf-regression-test-configuration

hannojg marked this pull request as ready for review December 6, 2022 18:26

hannojg requested a review from a team as a code owner December 6, 2022 18:26

melvin-bot bot requested review from NikkiWines and removed request for a team December 6, 2022 18:26

hannojg added 3 commits December 12, 2022 19:30

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

443aeff

…/perf-regression-test-configuration

add --development flag

6282828

add docs

818013e

hannojg added 3 commits December 15, 2022 18:08

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

04b9279

…/perf-regression-test-configuration

fix script linting, use bash

9c6b0bf

fix script linting

b22716d

AndrewGable reviewed Dec 17, 2022

View reviewed changes

tests/e2e/testRunner.js Outdated Show resolved Hide resolved

hannojg added 2 commits December 27, 2022 10:22

Merge branch 'main' of github.com:margelo/expensify-app-fork into dev…

3a65306

…/perf-regression-test-configuration

ignored error

8c1540e

hannojg requested review from AndrewGable and removed request for NikkiWines December 27, 2022 09:28

AndrewGable approved these changes Dec 28, 2022

View reviewed changes

AndrewGable merged commit 6c5b798 into Expensify:main Dec 28, 2022

OSBotify mentioned this pull request Dec 29, 2022

Deploy Checklist: New Expensify 2022-12-29 #13891

Closed

7 tasks

melvin-bot bot mentioned this pull request Jan 2, 2023

[HOLD for payment 2023-01-09] [Tracking] Add performance CI tests #11711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev: perf regression test configuration #13194

dev: perf regression test configuration #13194

hannojg commented Nov 30, 2022 •

edited

Loading

AndrewGable commented Dec 5, 2022

hannojg commented Dec 6, 2022

melvin-bot bot commented Dec 6, 2022

Szymon20000 commented Dec 8, 2022

Szymon20000 commented Dec 8, 2022

hannojg commented Dec 9, 2022 •

edited

Loading

Szymon20000 commented Dec 9, 2022 •

edited

Loading

Szymon20000 commented Dec 9, 2022

Szymon20000 commented Dec 9, 2022

hannojg commented Dec 12, 2022 •

edited

Loading

AndrewGable commented Dec 13, 2022

hannojg commented Dec 15, 2022

AndrewGable left a comment

AndrewGable commented Dec 28, 2022 •

edited

Loading

OSBotify commented Dec 28, 2022

github-actions bot commented Dec 29, 2022

OSBotify commented Dec 29, 2022

OSBotify commented Jan 2, 2023

dev: perf regression test configuration #13194

dev: perf regression test configuration #13194

Conversation

hannojg commented Nov 30, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

AndrewGable commented Dec 5, 2022

hannojg commented Dec 6, 2022

melvin-bot bot commented Dec 6, 2022

Szymon20000 commented Dec 8, 2022

Szymon20000 commented Dec 8, 2022

hannojg commented Dec 9, 2022 • edited Loading

Szymon20000 commented Dec 9, 2022 • edited Loading

Szymon20000 commented Dec 9, 2022

Szymon20000 commented Dec 9, 2022

hannojg commented Dec 12, 2022 • edited Loading

AndrewGable commented Dec 13, 2022

hannojg commented Dec 15, 2022

AndrewGable left a comment

Choose a reason for hiding this comment

AndrewGable commented Dec 28, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

OSBotify commented Dec 28, 2022

github-actions bot commented Dec 29, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 29, 2022

OSBotify commented Jan 2, 2023

hannojg commented Nov 30, 2022 •

edited

Loading

hannojg commented Dec 9, 2022 •

edited

Loading

Szymon20000 commented Dec 9, 2022 •

edited

Loading

hannojg commented Dec 12, 2022 •

edited

Loading

AndrewGable commented Dec 28, 2022 •

edited

Loading