[No QA] Simplify test workflow using Jest --shard flag

[roryabraham]

Details

This PR started out as a simplification of the test workflow, but grew with some (valuable) scope-creep. Upgrading Jest was a huge pain, so I'm proud of this PR that gets it done!

The initial changes

• Get rid of the unnecessary config job from test.yml, instead define the matrix inline
• Separates out the shell tests into their own job (before they were running in all three jobs)

Upgrading Jest

• In order to use the --shard feature of jest to split up the tests between multiple runners, we needed to upgrade to at least Jest 28: https://jestjs.io/blog/2022/04/25/jest-28#sharding-of-test-run, so I went for the latest version (Jest 29)
• Upgrading Jest presented a number of challenges, each of which I managed to work through (some with lots of frustration) I'll try to explain the more significant learnings below.
• Had to add a few more babel plugins to get Jest 29 running (I think they're using some newer JS features)

Fake Timers - why?

One of the biggest changes with Jest 28 was that it changed the "fake timers" module.

I think the idea behind fake timers is to make tests faster and less flaky. Rather than waiting for actual CPU cycles, timers, JavaScript macrotasks, etc... (all things I don't really understand very well), everything is "frozen" by default with fake timers.

Then you get fine-grained control when you want to wait for promises to resolve, when you want to advance the timers and by how much. This speeds things up because you generally never wait for real timeouts, and when the clock needs to be advanced that happens as close to instantly as possible.

So in general using real timers in tests is bad (supposedly), so we enable them globally. The only timer we don't fake globally is nextTick, and I think it has to do with how Onyx intentionally updates subscribers only on the next tick (example). As stated in the code comment there we don't necessarily need to do this anymore and it in my opinion it would be very valuable to clean that up in the future. I think that would also mean we could remove many if instances of waitForPromisesToResolve throughout our tests.

Fixing flaky UnreadActionsTest

The UnreadActionsTest is slow and was pretty flaky (very consistent on my M2 mac, but very inconsistent on the GitHub Actions runners). So I did a few things to make it better:

• Moved jest.setTimeout to the correct location. Not sure if something changed in jest or if it just wasn't doing anything before, but according to the jest docs:

  Set the default timeout interval (in milliseconds) for all tests and before/after hooks in the test file. This only affects the test file from which this function is called.

  So I think that the file root is the correct place (and it resolved timeout errors I was seeing on this PR).

• Before we were using jest.advanceTimersByTime(100); to wait for animations to complete. This was flaky even on my computer, and to get it working consistently on the runner I had to increase the timeout to like 500. Maybe something to do with the new timers? Anyways, rather than relying on this sort of hacky/slow/flaky method of waiting for the animation to finish, I decided to use the approach recommended by @testing-library/react-native – using waitFor.

• However, waitFor had a gotcha – for now, we need to use the @testing-library/react-native jest preset in order for it to work properly (fixed upstream in React Native 0.71.2, issue created here, TODO left in the code)

• In order to use the @testing-library/react-native plugin, I had to upgrade @testing-library/react-native. (More on this below)

Upgrading @testing-library/react-native

• v11 of @testing-library/react-native included a breaking change that removed some of the queries we were using, so I had to update those
• At this point, since I was updating some queries already, I made the decision to migrate over all our test queries from using the result of a render call to using screen instead. This wasn't strictly necessary, but is recommended by the library maintainers. While it did make the diff of this PR a bit nastier, it's ultimately a simplification because it doesn't require you to maintain and pass around a reference to the render result. I went back and forth on whether this was worth it, but it was just a series of a few simple find-and-replace queries in three files, so I included it.

SessionTest

I'm not sure why this was passing before and not after the Jest upgrade, but it seems like it was a false positive before. Session.signOut alone is not enough to make PushNotification.deregister be called. You need to call signOutAndRedirectToSignIn instead, which in turn clears Onyx, which in turn causes PushNotification.deregister to be called.

Other

• I moved jest configs to their own file, mostly because I find the JS more readable than pure JSON and I like being able to jump straight to the jest config file. We could move it to a subdirectory but that causes a few issues if (like me) you run tests using the GUI of your IDE. It also wasn't clear to me if I should put it in config/jest.config.js or jest/config.js, so I just put in the project root because that's the standard place for it.
• One small QOL improvement we get from upgrading Jest is that when tests fail you get a nice summary of failing tests, which we didn't have before (instead I always had to search for FAIL tests/ in the GitHub Actions logs). Now we get a concise list of all the tests that failed.

Future Work

• The default test environment is jsdom, but GitHub Actions tests run in node, as so should be tested in a node environment rather than jsdom. This can be accomplished with a comment in the first lines of the file, but I think a better solution would be to move those to tests/githubActions and configure jest to use the node runner for tests in that directory, and jsdom for the others. This PR is big enough already so I'm not doing that today.
• There are probably a lot of other locations where we could use waitFor instead of advancing timers or using waitForPromisesToResolve, and it might make the tests more readable and intuitive

Fixed Issues

$ #14088

Tests (already done)

Purposely break a Jest test, then push code to this PR. The test workflow should fail. This happened plenty of times during development, as you can see below.

• Verify that no errors appear in the JS console

Offline tests

None.

QA Steps

None.

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Screenshots are not included, since only test code has been updated and those tests have run in the CI for this PR. For good measure I've included a screenshot of all tests passing locally (both in my terminal and my IDE).

[roryabraham]

I think I might've figured out why UnreadIndicatorsTest isn't working:

[roryabraham]

Okay, that test is consistently passing when run in isolation, but I'm able to reproduce the failure (and timeout issue) when running the full npm run test test suite locally. I suspect this means that another test is not cleaning up after itself properly.

[roryabraham]

This was redundant

[roryabraham]

This seems to be resolved now, maybe because of the new fake timers module or because we have doNotFake: ['nextTick]

[MelvinBot]

@thesahindia @aldo-expensify One of you needs to 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]

[roryabraham]

Removing C+ review here because there's nothing for them to test beyond what we can see in the CI.

[aldo-expensify]

NAB: The DateUtils.getDBTime() will get a frozen time that only moves forward if you do jest.advanceTimersByTime, did I get that correctly?

[roryabraham]

Yes

[roryabraham]

But that's kind of a good thing because the "fake" timestamps are consistent and predictable.

[aldo-expensify]

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: N/A

[OSBotify]

✋ 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]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	752.484 ms → 781.026 ms (+28.542 ms, +3.8%)
Open Search Page TTI	594.790 ms → 596.145 ms (+1.355 ms, ±0.0%)
App start nativeLaunch	19.724 ms → 20.500 ms (+0.776 ms, +3.9%)
App start regularAppStart	0.016 ms → 0.017 ms (+0.002 ms, +9.8%)
App start runJsBundle	213.000 ms → 209.645 ms (-3.355 ms, -1.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 752.484 ms Stdev: 30.928 ms (4.1%) Runs: 688.6598499999382 690.8003730000928 703.7739790000487 704.3804510000627 707.157229000004 712.8014960000291 731.4435699998867 735.5723210000433 738.1922009999398 739.2843939999584 747.7976389999967 752.4251979999244 755.8913819999434 756.3109599999152 756.3895670000929 759.8337040001061 762.9985940000042 765.4193969999906 769.9294720001053 770.2914120000787 771.5365969999693 771.8055120001081 771.9444180000573 772.1065340000205 779.625728999963 784.394541000016 786.0551710000727 787.5991589999758 789.1442150000948 810.9552470000926 Current Mean: 781.026 ms Stdev: 28.075 ms (3.6%) Runs: 733.285166000016 743.962373000104 745.532150000101 745.9629160000477 746.4489569999278 747.464433999965 749.324271999998 758.603192999959 758.7334950000513 761.3660210000817 770.7156990000512 770.9298719998915 771.7909619999118 773.510685000103 776.6592300001066 778.270557000069 780.8051970000379 781.11750199995 784.8138739999849 785.8895789999515 788.1724849999882 789.8782490000594 797.0175520000048 799.7510909999255 801.5880370000377 806.1520789999049 808.9805570000317 811.4116579999682 817.0310659999959 826.4899669999722 840.2441169999074 840.9216489999089
Open Search Page TTI	Baseline Mean: 594.790 ms Stdev: 21.478 ms (3.6%) Runs: 557.8547779999208 564.726521999808 569.9966639999766 570.5347490001004 572.510050999932 573.536376999924 574.2652999998536 575.1717940000817 575.7635910001118 578.4668370001018 579.7003989999648 580.0175370001234 585.2878420001362 589.8754879999906 590.9044599998742 593.1753340000287 594.0799969998188 598.7196450000629 599.4250080001075 600.5401620001066 602.573161000153 604.5430909998249 604.7118329999503 606.21822099993 610.7148440000601 614.6234539998695 616.4442550002132 616.4985770001076 618.9348140000366 627.879924000008 642.6419270001352 642.9547929998953 Current Mean: 596.145 ms Stdev: 26.400 ms (4.4%) Runs: 543.893798999954 557.4832359999418 558.0064699999057 560.6892499998212 561.3616140000522 571.4215090000071 571.6922199998517 572.4075930002145 576.4726559999399 580.5936280000024 584.4304209998809 585.5056149999145 586.9629320001695 587.2652590000071 591.6818859998602 594.151531000156 594.2807620000094 595.8307300000452 596.1278490000404 602.0274669998325 602.1556400000118 607.2303470000625 608.6734219999053 615.0590820000507 616.9798179999925 617.5708830000367 619.6223560001235 624.3665769998915 628.1652430000249 628.9522299999371 636.6710210000165 643.2943519998807 651.7659100000747
App start nativeLaunch	Baseline Mean: 19.724 ms Stdev: 1.412 ms (7.2%) Runs: 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 22 23 24 Current Mean: 20.500 ms Stdev: 1.310 ms (6.4%) Runs: 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 22 22 22 23 23 23
App start regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (6.7%) Runs: 0.013590000104159117 0.013712999876588583 0.013794000027701259 0.013835000107064843 0.014038000022992492 0.014362999936565757 0.014445000095292926 0.015015000011771917 0.015177000081166625 0.015217999927699566 0.015339999925345182 0.015379999997094274 0.015462000155821443 0.015462999930605292 0.015543999848887324 0.015625 0.015910000074654818 0.015910000074654818 0.01595099992118776 0.015992000000551343 0.016032000072300434 0.016032000072300434 0.016153999837115407 0.016276000067591667 0.01647999999113381 0.016561000142246485 0.016600999981164932 0.016600999981164932 0.016682999907061458 0.0166830001398921 0.017536999890580773 0.017537999898195267 Current Mean: 0.017 ms Stdev: 0.001 ms (5.8%) Runs: 0.01509599993005395 0.015705999918282032 0.015706000151112676 0.015829000156372786 0.016032000072300434 0.016235999995842576 0.01627700007520616 0.0163569999858737 0.016478999983519316 0.01647999999113381 0.016601999988779426 0.01664300006814301 0.016682000132277608 0.016844999976456165 0.01688600005581975 0.01700900006107986 0.01745599997229874 0.017496000044047832 0.017496999818831682 0.017536999890580773 0.017537000123411417 0.017577999969944358 0.017862999811768532 0.017863000044599175 0.017903000116348267 0.018026000121608377 0.018269999884068966 0.01831099996343255 0.018352000042796135 0.018635999877005816 0.0195720000192523
App start runJsBundle	Baseline Mean: 213.000 ms Stdev: 21.678 ms (10.2%) Runs: 180 182 183 188 189 189 190 192 197 199 201 201 207 207 209 211 217 218 219 220 220 223 227 229 231 231 236 246 247 254 260 Current Mean: 209.645 ms Stdev: 12.460 ms (5.9%) Runs: 181 192 196 197 197 199 200 202 203 204 205 205 205 205 206 206 208 208 210 212 215 216 218 219 220 220 222 224 230 237 237

[OSBotify]

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.2.68-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.68-0 🚀

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