Repository navigation
[No QA] chore(ci): cache Jest transforms in the Reassure perf jobs - #100297
roryabraham merged 9 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9632d94cc7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
collectioneur
left a comment
There was a problem hiding this comment.
What do you think about renaming seedStickyDisks to something like seedCache? It might help avoid confusion with seedJestCache (if we do, let's update the PR description as well).
I also noticed that almost all of the comments could likely be shortened or removed entirely, since the code is written clearly enough to explain itself
One last thought on the tests: maybe we can trim them down and just keep the first three for this CI job? 🤔 I feel they don't add significantly more safety, but they will definitely add maintenance overhead for us when the code changes later
e73ed48 to
f439127
Compare
| - 'package.json' | ||
| - 'package-lock.json' | ||
| - '.github/workflows/bunTests.yml' | ||
| - '.github/workflows/reassurePerformanceTests.yml' |
There was a problem hiding this comment.
Why would bun tests be dependent upon reassure or jest workflows?
There was a problem hiding this comment.
I added it to run tests when a PR edits only yml files related to the cache mechanism. Without it, those files could be modified and merged without the tests running on the PR, the failure would only surface post-merge on main.
There was a problem hiding this comment.
Added one line comment explaining why we need that
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.75-0 🚀
|
|
Hey @WojtekBoman. Do we need QA for this? |
|
no QA |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The Reassure perf jobs re-transformed ~7,000 files through Babel on every run, 45% of each measure job (217.0s cold vs 119.7s warm). They now restore a Jest transform cache written on
mainby a newseedJestPerfCache.yml, called fromseedStickyDisks.yml. Restore-only and norestore-keys, so a fork PR can never write the shared entry. Also addsconcurrencycancellation and abun:testguard on the shared key.Fixed Issues
$ #100428
PROPOSAL:
Tests
Open this PR and let the
Run performance testsworkflow run.Open the
baseline-perf-testsjob log and expandRestore Jest transform cache. Verify the step reports a cache hit for a key of the formLinux-X64-jest-perf-<hash>(on the very first run after merge tomaina miss is expected, since the entry has not been seeded yet).Expand
Report Jest cache sizein the same job. Verify the run annotation reads roughlyJest perf cache: ~120MB, ~7000 files, notabsentand not double that size.Repeat steps 2-3 for the
perf-tests(branch) job. Verify both measure jobs restore the exact same key.Verify that the perf comparison comment is still posted on the PR and that render counts are unchanged from before this PR (
COUNT_DEVIATION: 0must not start firing).Push a second commit to this PR while the first
Run performance testsrun is still in progress.Verify the in-flight run is cancelled by the new
concurrencygroup and only the newest run continues, and that no other PR's perf run is cancelled.On a checkout of this branch, run
npm run test:bun -- tests/tooling/jestPerfCacheKey.test.ts.Verify all tests pass.
Edit any one of the four
.jest-cachekey:values in.github/workflows/reassurePerformanceTests.ymlor.github/workflows/seedJestPerfCache.ymlso it differs by a single character, re-run the test file, and verifykeys every .jest-cache step identicallyfails. Revert the edit.Add a
restore-keys:entry to any of those four steps, re-run, and verifynever falls back to a prefix keyfails. Revert the edit.Remove the
seedJestPerfCachejob from.github/workflows/seedStickyDisks.yml, re-run, and verifyis reachable from a push to mainfails. Revert the edit.Offline tests
N/A - this PR only changes GitHub Actions workflows and a tooling test. It touches no app code, no API calls, and no Onyx data.
QA Steps
N/A - CI-only change with no user-facing surface. Same as tests: verification happens in the GitHub Actions logs on
mainafter merge, whereSeed Jest perf cacheshould run as part ofSeed sticky diskson each push, skip the ~220s populate when the entry is already present, and re-populate it after a toolchain change to the lockfile,patches/**,babel.config.js,config/babel/**,jest.config.js, or.nvmrc.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari