[No QA] Migrate GitHub Actions and scripts tests from Jest to bun:test - #98402
Merged
Conversation
These 19 files test .github/actions, .github/libs, .github/scripts and scripts/ — build and CI tooling rather than app code. Grouping them in one directory lets every config that needs to treat them differently (Jest, tsconfig, ESLint, the Bun test runner, CI path filters) use a single glob instead of enumerating each file. Renamed to the *.test.ts suffix Bun's test runner discovers, so the whole directory can be passed to `bun test` without listing files. Pure move: no file contents changed.
Adds the plumbing that lets tests/tooling/ run under `bun test` instead of Jest: - tests/tooling/tsconfig.json type-checks the directory with @types/bun (bun:test's ambient types conflict with the @types/jest the root config uses), and the root tsconfig excludes it so nothing is checked twice with the wrong types. - tests/tooling/setup.ts is preloaded to default GITHUB_REPOSITORY, mirroring jest/setup.ts for the files Jest still owns. - `test:tooling` runs the whole directory with --isolate, which gives each file a fresh module registry, so module-level state and mock.module() calls can't leak between files the way they would in Bun's default single-registry mode. - Jest ignores the directory, ESLint parses it with the new tsconfig, knip treats the files as entry points (nothing imports them), and the Bun CI job runs the new script.
These 14 files exercise .github/actions and .github/libs, whose @actions/@octokit dependencies go ESM-only in their next majors. Running them under bun:test means those imports resolve natively, so no CJS shims are needed when the upgrade lands. Mechanical changes throughout: import the test globals from bun:test, drop the `@jest-environment node` docblocks (Bun has no jsdom to opt out of), and swap Jest's two-parameter mock generics for Bun's single function-type form. Two differences needed real changes: - Bun resolves @actions/* as real ESM, whose namespace exports are read-only live bindings, so `core.getInput = mock` no longer works; these now use jest.spyOn. That also removes the tests' dependency on @src/types/utils/asMutable. - Bun has no `jest.mock(path)` automock and no advanceTimersByTimeAsync. Auto-mocked modules are replaced with explicit per-function spies, and DeployChecklistUtils' retry tests get a small local helper that yields to the microtask queue before firing the timer. postOrReplaceComment also drops jest-when, which depends on Jest's internal expect-matcher state. Its per-argument stubbing is replaced with a plain inputs record per test that throws on an undeclared input, preserving jest-when's strictness about unexpected calls.
This is the only action still ending in `module.exports = run` while
using ESM `import` statements at the top. Babel rewrote the whole file
to CommonJS for Jest, so the mix went unnoticed; Bun loads it as real
ESM and refuses it outright ("Expected CommonJS module to have a
function wrapper"), which blocks migrating its test to bun:test.
Every other action already uses `export default run`. The rebuilt bundle
changes only the three corresponding lines, and its
`require.main === require.cache[...]` entry guard - the only thing that
actually invokes the action - is unaffected.
Completes tests/tooling/ with the five files the earlier commit left on Jest, so the whole directory now runs under one runner: - bumpVersion and Git relied on `jest.mock(path)`'s automock, which Bun has no equivalent of. They now replace `fs`/`fs/promises` and `child_process` with mock.module() before importing the module under test. --isolate keeps those replacements from reaching other files. - markPullRequestsAsDeployed drops its ActionUtils mock and exercises the real getJSONInput, since the mock only reimplemented it. That means core.getInput's stub has to return strings, as the real one does, and MOBILE_EXPENSIFY_PR_LIST has to be a declared input rather than one that throws and gets swallowed. - DeployChecklistUtils' retry tests replace advanceTimersByTimeAsync, which Bun lacks, with a helper that alternates between yielding to the microtask queue and advancing the clock until the call settles. It throws rather than hanging if the call is waiting on something else. - versionUpdater, failureNotifier and detectReactComponent needed only their bun:test imports.
- @types/bun types the assertion helpers more tightly than @types/jest did, so a few mocks needed adjusting: DeployChecklistUtils' redundant listForRepo mock is folded into the spy that wrapped it, expect.stringContaining (typed `any` by bun-types) becomes toContain, and the two octokit stubs that can't carry the endpoint statics get an explicit disable with the reason. - getPullRequestIncrementalChanges stubs unset inputs as '' rather than null, matching what core.getInput actually returns. - oxfmt sorts bun:test alongside the other test-runner imports, which also reorders the imports in the existing server/ bun tests. - jest-when and @types/jest-when are dropped now that postOrReplaceComment, their only consumer, no longer uses them.
Jest's CI job passes --silent, so this output was never visible before; `bun test` has no equivalent flag, and the code under test logs enough to bury the results (2200 lines of output for a 19-file run). Stub the console in the preload instead, behind TEST_VERBOSE.
- runWithFakeTimers now advances by the exact backoff schedule the code under test declares, restoring what the Jest version pinned; advancing by an arbitrary amount had made the tests pass for any delay. It also installs the fake timers inside the try, so a synchronous throw can't leave the clock frozen for later tests. - GithubUtils re-installs its core.getInput spy per test: getCommitHistoryBetweenTags' afterEach calls jest.restoreAllMocks(), which under Jest could not reach the plain assignment this replaced. - createOrUpdateDeployChecklist pins the clock. Its assertions re-derive today's date and compare it against the title the action stamped, and Jest's global fake timers used to make that deterministic. test:tooling also pins TZ=utc, as npm test does. - isDeployChecklistLocked drops its module restore, dead under --isolate, along with the comment claiming files share a module registry. - bunTests.yml watches tests/utils/**, which five of these files import. - Git's two `not.toContain(expect.stringContaining(...))` assertions were vacuous: toContain compares with === and ignores asymmetric matchers. - Comment fixes: the @actions/github context justification described an environment that doesn't hold in CI, and CIGitLogic's preamble still described Jest, which no longer runs it. It now also records that a failure cascades, since Bun's --bail can't be scoped to one file. - Tightened the four seatbelt counts that dropped.
Moving these files out of tests/unit/ took them out of test.yml, which preDeploy.yml calls on every merge to main - so the tests covering preDeploy's own machinery (isDeployChecklistLocked, the deploy checklist, markPullRequestsAsDeployed) stopped gating the deploy they guard. Give bunTests.yml a workflow_call trigger and add it to preDeploy alongside typecheck/lint/test. That also closes the same pre-existing gap for the server tests. Split bunTests.yml into two jobs so a server-test failure no longer hides the tooling results, and so the git-heavy tooling suite runs alongside the victory-chart-renderer build rather than after it. The rest is documentation, because nothing told a contributor that this directory exists or how to run it: - tests/tooling/README.md covers how to run the suite and a single file (the leading ./ and the flags are both load-bearing), the rule for what belongs here rather than in tests/unit/, and the bun:test/Jest API gaps that shaped these files. - README.md, tests/README.md and CLAUDE.md all described Jest as the only unit-test runner. - bunfig.toml's comment claimed Jest owns all of tests/. Also: typecheck.yml's paths filter listed tsconfig.json literally, so editing tests/tooling/tsconfig.json (or server/tsconfig.json) skipped typecheck entirely.
Adding the workflow_call trigger meant this group is now evaluated for pushes to main, where github.ref is the same for every merge - so two merges in quick succession would cancel each other's run. That is the bug #41936 fixed for lint and test, and it would be worse here: confirmPassingBuild only treats 'failure' as failing, so a cancelled bunTests would pass the gate silently. Use the same SHA-qualified group the other preDeploy workflows use. Also drop GithubUtils' core.getInput stub, which had no implementation and no assertions behind it - the tests that need core stub it themselves - and move useFakeTimers inside the try that restores them.
These four transitively import @actions/core at runtime, so they would have broken under Jest the moment the ESM-only upgrade landed. Moving them now means no Jest test can reach @actions/* or @octokit/* any more, which is the whole point of this PR: nothing in tests/unit/ needs a CJS shim built for it. Per-file notes: - artifactsResolver replaces child_process, githubCLI and GithubUtils with mock.module() in place of three jest.mock() automocks. Typing the paginate mock properly also retires two no-unsafe-type-assertion disables. - generateTranslations swaps `en` per test via a fresh mock.module() rather than a getter over a mutable variable: the script imports `en` as a default binding, which Bun resolves once at link time, so a getter only fires for the first test. Its Git stubs are now spies on the three static methods the script actually calls, rather than an automock of the whole class, and the diff fixtures gained the `diffType` the real Git.diff always sets - inert at runtime, but it means the fixtures now type-check against FileDiff. - ChatGPTTranslator drops its OpenAIUtils automock. Only `promptResponses` was ever stubbed, and the constructor it was hiding just stores a key. - createRetestRequestForCP needed only its bun:test imports. test:tooling now also preloads scripts/stubReactNative.js: generateTranslations reads src/languages/en, which pulls in react-native, whose Flow syntax Bun can't parse. Jest got away with it because Babel transformed react-native. This is the same stub `bun scripts/generateTranslations.ts` already runs with - bunfig.toml's top-level preload doesn't apply to `bun test`. Counts are unchanged: the four ran 63 tests under Jest and run 63 here, so tests/tooling is 23 files and 316 tests.
…orts Migrating generateTranslationsTest ran fine under bun:test (40/40), but it can't be type-checked there. tests/tooling type-checks with @types/bun, whose ambient JSX declarations conflict with the app's React and react-native types, and the script under test imports src/languages/en - which drags ~3,000 app files into the project and reports thousands of errors that the root project, with `jest` types instead of `bun`, does not. Confirmed both directions: main's tsconfig finds no errors in src/, and swapping only the `types` array reproduces them. The test side can't narrow that graph, because the import comes from the script. Moving it needs the pure logic extracted or a separate type-check strategy, so it stays on Jest here and still has to move before @actions/* goes ESM-only. Recorded in tests/tooling/README.md alongside the constraint itself, so the next person doesn't rediscover it. ChatGPTTranslator hit the same wall for a reason that *was* fixable: it imported Locale from @src/types/onyx/Locale, which re-exports the whole @src/CONST barrel. It now uses TranslationTargetLocale from @src/CONST/LOCALES - self-contained, and the type the API it exercises actually takes. The stubReactNative preload goes away with generateTranslations, since nothing else here reaches react-native.
Codecov Report✅ All modified and coverable lines are covered by tests. |
Three unrelated failures on this PR: - `verify` rebuilt isDeployChecklistLocked's bundle and found a diff. Main recently made the html-entities patch idempotent by folding the "worklet" directive onto the "use strict" line, but the committed bundle still embeds the old two-line form, so every PR branched off current main fails this check. Rebuilt against the current patch. Not introduced here - it reproduces on plain main - but it blocks this PR, so it's fixed here. - `Compare knip issues against main` reported three new `unlisted` findings, which were the same three pre-existing ones at their new tests/tooling paths. @octokit/request-error is imported directly by six files (three of these tests plus .github/libs/GithubUtils, .github/libs/isTeamMember and getPullRequestIncrementalChanges) but only ever resolved transitively through @actions/github, so declaring it resolves the finding rather than relocating it. - `spellcheck` flagged "Backoffs" and "bunfig". The first is gone: runWithFakeTimers' parameter is now expectedDelaysMs, matching the LIST_RETRY_DELAYS_MS constant it is pinned to. The second is a real filename, so it goes in the dictionary. The Android HybridApp job also failed, at "Load files from 1Password". That is infrastructure - the iOS job on the same runner passed, and nothing here touches the build - so there is nothing to fix for it.
Three conflicts, all where main independently reworked files this branch had migrated: - createOrUpdateDeployChecklist: main replaced the hand-built fake octokit with a real one from initOctokitWithToken plus spies, added createMock in place of `as unknown as` casts, and added the mockDeployChecklistIssuesByLabel helper. Resolved by taking main's file wholesale and re-applying this branch's bun migration onto it, rather than stitching nine hunks: the memfs mock.module, the dynamic import, explicit @actions/core spies, the pinned clock, and Bun's Mock<T> in place of jest.SpiedFunction/MockedFunction/mocked. Octokit's defaults/endpoint statics are now copied onto the mocks with Object.assign, since a Bun mock doesn't carry them and paginate reads them off the method. - isDeployChecklistLocked: main's changes were jest.mocked and typed requireActual, both of which the bun version already supersedes, so the conflicting hunks take this branch's side. Main's createMock improvements merged cleanly and are kept. - eslint.seatbelt.tsv: kept this branch's renamed paths, minus the three entries main deleted outright - its createMock rework removed the last no-unsafe-type-assertion violations in failureNotifier, createOrUpdateDeployChecklist and isDeployChecklistLocked. Confirmed by running ESLint, which neither errors nor re-tightens the baseline. Verified after the merge: 276 tooling tests pass, all four tsconfig projects are clean, ESLint and knip are clean, all 24 action bundles still reproduce, generateTranslationsTest still passes under Jest, and no Jest test reaches @actions/* or @octokit/*.
This was the last Jest test whose import graph reached @actions/*, and the one blocking the ESM-only upgrade. It runs unchanged in substance: 40 tests, same assertions. The reason it was left behind earlier was type-checking, not runtime. tests/tooling had its own tsconfig declaring @types/bun, and this file reaches src/ through the script it covers, so the whole app got checked with bun-types - whose global JSX declarations are incompatible with the app's React types. The fix removes that separate project rather than working around it. bun-types/test.d.ts is the single file in the package that declares `bun:test`, and it references nothing else, so the root tsconfig now pulls in just that one file and type-checks tests/tooling alongside everything else with the app's real types. It goes in `files` rather than `include` because `exclude` covers node_modules and would drop it. That means one less tsconfig, one less project in `npm run typecheck`, and no ESLint override for the directory. The trade-off worth knowing: @types/jest's globals are now visible in tests/tooling, so a missing bun:test import can type-check and still fail at runtime. Recorded in tests/tooling/README.md. Migration specifics: - `en` is swapped per test with a fresh mock.module() rather than a getter over a mutable variable: the script imports it as a default binding, which Bun resolves once at link time, so a getter would only ever be read for the first test. - The Git automock becomes spies on the three static methods the script actually calls. `openai` needs no stub at all - under --dry-run the script builds a DummyTranslator and never constructs the client. - test:tooling preloads scripts/stubReactNative.js again, because src/languages/en pulls in react-native, whose Flow syntax Bun can't parse. It is the same stub `bun scripts/generateTranslations.ts` already runs with. tests/tooling is now 23 files and 316 tests, and no Jest test reaches @actions/* or @octokit/* - verified across all 1180 of them.
test:bun and test:tooling were two npm scripts, two CI jobs and two Bun invocations for what is one thing: the tests this repo runs under Bun. They are now a single invocation passing both roots, so `npm run test:bun` covers server/ and tests/tooling/ together - 348 tests across 27 files. Two flags had to be reconciled to make one invocation viable, and both resolve in the tooling suite's favour: - --isolate replaces --concurrent. CIGitLogic's tests build on each other's git state and its own comment says it must not run in parallel with anything else, so concurrency was never safe for it. The server suite loses nothing measurable: 32 tests, 3.4s either way. - The tooling preloads now apply to the server tests too. Checked that they are inert there: no server test reads GITHUB_REPOSITORY, and none spies on or asserts console, which is what setup.ts stubs. This also fixes the problem the split CI jobs were working around. The concern was that a server failure would short-circuit the tooling step so it never ran; with one invocation Bun runs every file and reports every failure regardless, which is what the split was trying to buy. So the second job goes away as well. Running just the tooling directory, or one file, means passing the flags directly - documented in tests/tooling/README.md along with the reason each one is load-bearing.
CIGitLogic built its dummy repo and git remote at fixed paths under $HOME (~/DumDumRepo and ~/dummyGitRemotes/DumDumRepo), and every checkoutRepo() rm -rf'd them. That made the suite hostile to anything else on the machine, which its own header comment admitted: "They should not be run in parallel with other tests on the same machine." It now mkdtemp's a sandbox per run and puts both the remote and the checkout inside it, so two runs cannot see each other. Verified: two concurrent copies of the file both pass, where before they scored 0/9 and 0/1. Two concurrent `npm run test:bun` runs also both pass 348/348. It no longer leaves anything in $HOME either. The suite still changes the process-wide cwd, because the git helpers it covers resolve `git` against process.cwd() exactly as they do in a real Actions checkout - narrowing that would mean adding a cwd parameter to production code purely for the test. Instead the file gets its own process, via --parallel. test:bun therefore moves from --isolate to --parallel, which runs each file in a worker process and implies --isolate, so the module-registry and process.env isolation the suite depends on is retained. Measured back to back on the full 348 tests: 70s -> 54s, then 77s -> 68s on a second pass. The win is capped because wall clock is now set by the longest single file, and CIGitLogic is ~52s of it. --concurrent stays off, and the earlier claim that --isolate forced that choice was wrong - they compose fine. It is off because it does not work here and does not pay: - It makes tests within a file run at once, which cannot beat the longest-file bound above. - 16 of the 23 files reset module-level spies in beforeEach, so a concurrent sibling clears the mocks a running test is about to assert on. 9 tests fail deterministically today; the rest are latent. - Measured slower anyway: --parallel 52.9s passing vs --parallel --concurrent --max-concurrency 7 at 57.2s with 9 failures. CIGitLogic is marked describe.serial regardless, since its tests each build on the repo state the previous one left behind - that ordering is a real invariant, not a flag-dependent one. Also drops a stale README paragraph claiming tests/unit/generateTranslationsTest.ts is still on Jest; that file was migrated and no longer exists.
Replaces execSync with Bun's `$`. On the real suite, measured back to
back three times: 56.3s -> 49.8s, 56.3s -> 49.3s, 51.5s -> 45.2s, so
about 11%. Bun parses and spawns the command itself instead of forking
/bin/sh -c for each of the ~1000 git and npm invocations; a microbench
of `git rev-parse HEAD` puts the per-call cost at 17.6ms vs 27.5ms.
Since test:bun's wall clock is set by this file, the whole suite drops
with it.
`exec` is now a tagged template forwarding to `$`, which keeps the
existing logging and TEST_VERBOSE behaviour while letting Bun escape
interpolated values. Manual quoting is gone from the call sites that
only needed it to survive the shell:
exec(`git commit -m "${content}"`) -> exec`git commit -m ${content}`
Quotes are kept where they wrap a message built from literal text and
values, since there they read as one message rather than as escaping.
The three `git rev-parse --verify` probes were try/catch blocks around a
command run only for its exit code, two of them with empty catches.
They are now `if (await refExists('staging'))`, using `.nothrow()`.
Everything becomes async, because Bun's shell has no sync API. Verified
no await was missed by running eslint with no-floating-promises forced
on: it reports nothing on the file, and does report a deliberately
removed await, so the check is real rather than silently inactive.
Type-checking needs one structural change. `$` is a runtime API, so it
needs the whole of @types/bun rather than the single bun:test module
declaration the root project pulls in. Those types redeclare globals the
app already owns - a `jest` namespace that shadows @types/jest's generic
signatures, and a `fetch` carrying `preconnect` - and adding them to the
root project produces 1088 errors across tests/unit. Scoping the whole
of tests/tooling into a bun-typed project does not work either, because
generateTranslations.test.ts reaches src/ and that reintroduces the
conflict from the other side (2564 errors).
So CIGitLogic alone is excluded from the root project and type-checked
by tests/tooling/tsconfig.json, mirroring server/tsconfig.json. Verified
that project actually checks the file, rather than checking nothing, by
introducing a deliberate type error and confirming it is reported. The
eslint config points type-aware rules at the same project, likewise
verified with a deliberate error.
Two concurrent `npm run test:bun` runs still pass 348/348 each, so the
sandbox isolation from the previous commit survives the change in how
subprocesses are spawned.
The exec() wrapper was an async function, so it awaited the ShellPromise
internally and handed back a plain ShellOutput. That destroyed Bun's
chaining, which is why six commands in this file bypassed exec() and
called $ directly to reach .text() and .nothrow() - and were therefore
never logged.
A Proxy over $ keeps the lazy, chainable ShellPromise intact, so one
logged $ now covers every command in the file, including the
git rev-parse probes. Verified the chaining survives: .text(),
.nothrow(), .quiet(), the $.ShellError constructor and instanceof all
pass through the Proxy.
The failure path is dropped rather than ported. exec() caught
ShellError to log stderr, on the assumption Bun would not surface it -
that was wrong. bun:test prints the whole error object, stderr included,
along with the source line of the failing command:
ShellError: Failed with exit code 128
exitCode: 128,
stdout: "",
stderr: "fatal: not a git repository ...",
So the catch block was redundant. It also could not have been kept here:
attaching a rejection handler inside the apply trap starts the command
eagerly, and the next .quiet() then throws "Shell is already running".
Both facts are recorded in the comment so nobody adds one back.
Checked that type-aware linting still works through the Proxy, since
Reflect.apply returns `any` and could have silently degraded the return
type to something no-floating-promises ignores. It does not: with a
deliberately removed await, the rule fires on both a bare $`...` and a
floating helper call.
No performance cost - marginally faster across two pairs (59s -> 54s,
53s -> 48s), consistent with dropping an async frame and a try/catch per
call.
It was this branch's rename of main's JEST_VERBOSE knob, kept through the migration without anything wiring it up - there is no test:bun:verbose script, so reaching it meant setting the env var by hand. Removed from all three places it had spread to: the shell wrapper's .quiet(), setup.ts's console stubbing, which is now unconditional, and the command list in the README. Failures are unaffected either way, since bun:test reports those itself rather than through console.
knip parses these template literals to find binaries the repo invokes. In
git merge ${branchName} --no-ff -m "Merge pull request #${num} from ..."
the # opens a comment as far as its shell parser is concerned, so the next
bare word reads as a command name and it reported a missing `from` binary -
the one new finding on this PR (main 134, PR 131, added 1, resolved 4).
Interpolating the message instead of inlining it fixes that, because the
prose is then a value Bun escapes rather than shell source anything can
parse. Applied to the three git merge calls that failed and the three
commit messages carrying the same latent shape, so the rule is uniform:
human-readable messages are interpolated, never inline. Messages without a
# are left quoted, which knip reads fine.
The merge messages are load-bearing - GitUtils parses PR numbers back out
of them - so the escaped form has to be byte-identical. assertPRsMergedBetween
covers exactly that, and the suite passes 9/9.
|
@neil-marcellini 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] |
NicolasBonet
requested changes
Aug 13, 2026
Main refactored the same octokit mocks this branch migrated, so the two conflicting files needed real resolution rather than a side-pick: - tests/tooling/GithubUtils.test.ts conflicted through the rename from tests/unit/GithubUtilsTest.ts. Main replaced the hand-built octokit stub with the real client plus createMock, which supersedes what the migration carried over, so main's version is taken wholesale and the bun conversions reapplied on top: bun:test imports, Mock<T> in place of jest.SpiedFunction, and no @jest-environment pragma. - config/eslint/eslint.seatbelt.tsv resolves to main. Every conflict was the same shape - this branch renamed entries to tests/tooling/*, main deleted them because its refactor removed the underlying violations. Lint passes against main's baseline with no entries re-added. Main's changes to three other tooling tests merged cleanly but not correctly, since they were written against Jest: - jest.SpiedFunction<T> is not a bun type. It resolves anyway through @types/jest so nothing errored, which is exactly why it needed checking by hand. Now Mock<T>. - jest.fn<TReturn, TArgs>() takes one function type in bun, not two. - spyOn(GithubUtils, 'octokit', 'get') fails at runtime with "does not support accessor properties yet" and took out all 9 isAuthorizedContributor tests. It was redundant regardless: octokit is a getter over internalOctokit.rest, which initOctokitWithToken has already populated. - mockImplementation() with no argument type-errors under bun even though the runtime allows it. Where a stub had to satisfy an octokit method type, the real defaults/endpoint statics are copied onto it rather than asserting the difference away.
Review feedback: `expect(result as unknown).toStrictEqual({html_url})`
appeared 12 times. The cast was there because run() resolves to
IssuesCreateResponse, which a bare {html_url} literal is not assignable
to, but it also switched off checking on both sides - a typo in the
field name would have compared clean.
createMock<IssuesCreateResponse>({html_url}) is the convention this file
already uses elsewhere: the partial is checked against the real response
type, and the single assertion stays isolated in createMock.
The three remaining casts in the file are gone too. Those stubs had to
satisfy an octokit method type, which carries defaults/endpoint statics
a plain function cannot have. Copying the real statics onto each stub
makes them genuinely satisfy the type, so the assertion and its
eslint-disable are no longer needed. The file now has no 'as unknown'.
A cold-cache lint loads the full TypeScript program into every worker (~12GB each, per the note above runs-on), so 2 workers already needed ~30GB of the 32GB runner. This branch maps CIGitLogic.test.ts to a second tsconfig project - it needs @types/bun, whose globals cannot go in the root project - and each worker then holds that program as well. That overshot the remaining headroom and the runner was killed mid-lint, twice, with no ESLint output at all: the annotation reports lost communication from memory starvation rather than any lint error. Only this branch hit it because the ESLint cache key hashes config/eslint/**, which this branch changes, so it always takes the cold path. Other branches restore a warm cache and finish in 1.5-4 minutes; these runs ground on for 13 and 17 minutes before dying. blacksmith-16vcpu-ubuntu-2404 is already used by two other workflows here. Concurrency stays at 2 workers, so this only adds headroom rather than changing how lint runs.
The paths filter did not list lint.yml, so the previous commit changing the runner did not start a lint run and the PR kept showing the older failure. Editing how lint runs should re-run lint. Also condenses the runs-on comment. The array formatting is oxfmt's.
One conflict, again reached through a rename: main changed
tests/unit/markPullRequestsAsDeployedTest.ts, which this branch migrated
to tests/tooling/markPullRequestsAsDeployed.test.ts.
Resolved in favour of this branch's version. Main's change is a retyping
of the mock scaffolding - createMock for the octokit responses, real
octokit plus spyOn instead of a hand-built stub - and touches no test
body: not one it/describe/expect line differs. So keeping our version
loses no coverage and no behaviour.
Taking main's version instead would have meant undoing the constraints
the migration exists to satisfy. The module under test memoizes
GithubUtils.octokit.git.getCommit in a module-load-time constant, so the
octokit mock has to be installed before it is imported, which is why the
mock is built at module scope here rather than in beforeAll. Main's
version also leans on three things bun does not have: the two-type-param
jest.fn<TReturn, TArgs>(), jest.mocked(x, {shallow: true}), and
jest.mock() rather than mock.module().
The seatbelt baseline grandfathered these under tests/unit/*Test.ts, which the migration renamed, so the violations came back as errors under the new paths. Local lint hid it: seatbelt is readOnly unless CI is set, so this only reproduces with CI=true. awaitStagingDeploys: all three are gone. It builds a stub octokit purely to swap listWorkflowRuns, so it now initialises a real client and spies on that method, copying the real defaults/endpoint statics onto the stub - the same fix already used in waitForPreviousRuns. The InternalOctokit import went with it. markPullRequestsAsDeployed keeps its assertion behind an inline disable. The action memoizes GithubUtils.octokit.git.getCommit at module load, so the mock has to exist before the import and a real client cannot be spied on in time; the stub also replaces paginate, which a real client would run for real against stubbed responses. createMock does not help either, as PartialDeep still demands octokit's method types. Net effect against main: 5 grandfathered violations across these two files become 1 documented exception, and the baseline is untouched.
NicolasBonet
approved these changes
Aug 14, 2026
Contributor
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This is PR 2 of 4 from the bottom-up ncc → esbuild migration plan.
It migrates the tests covering our build and deploy tooling —
.github/actions/,.github/libs/,.github/scripts/andscripts/— from Jest tobun:test. Scripts are all using the Bun runtime now.That is every test whose import graph reaches
@actions/*or@octokit/*— verified across all 1,180 Jest test files.Why
@actionsand@githubdependencies since their newer versions are pure-ESM and Jest can't handle itTest timing
awaitStagingDeploys— polls on a real timergenerateTranslationsCIGitLogic— drives realgit/npmsubprocessesOn that first group Jest burns 42s of CPU where Bun uses 2.4s, roughly 17× less machine time.
Fixed Issues
$
PROPOSAL:
Tests
npm installin a clean checkout.npm run test:bunand confirm it reports348 pass,0 failacross 27 files — theserver/suite plustests/tooling/, which now share one Bun invocation rather than a second npm script.npm test -- tests/unit tests/actionsand confirm the Jest suite still passes and no longer collects the 23 migrated files (npx jest --listTests | grep -c tests/toolingprints0).npm run typecheckand confirm all four projects pass. Most oftests/toolingis checked by the root project;CIGitLogic.test.tsneeds@types/bunfor Bun's$shell, whose globals conflict with the app's, so it is excluded from the root project and checked bytests/tooling/tsconfig.jsoninstead.npm run lintandnpm run knipand confirm both are clean — in particular thatjest-whenis no longer reported as an unused dependency andtests/tooling/setup.tsis not reported as an unused file../.github/scripts/verifyActions.shand confirm it reports "Github Actions are up to date!" — i.e. the rebuiltmarkPullRequestsAsDeployedbundle matches its source.node .github/actions/javascript/markPullRequestsAsDeployed/index.jswith no inputs set and confirm it fails withError: Input required and not supplied: PR_LIST(the entry guard still fires) rather than doing nothing.tests/tooling/README.mddocuments —TZ=utc bun test --parallel --preload ./scripts/stubReactNative.js --preload ./tests/tooling/setup.ts ./tests/tooling/GithubUtils.test.ts— and confirm it passes on its own.expectedDelaysMsvalues intests/tooling/DeployChecklistUtils.test.tsfrom2000to1999, re-run, and confirm the test fails with "did its retry schedule change?" rather than passing or hanging.This PR also exercises several of the bundled actions on itself before merge, at least
isAuthorizedContributorandverifySignedCommits, andValidate GitHub Actionsruns the bundle build in CI.Offline tests
N/A - this PR only changes test tooling and CI configuration, not app code.
QA Steps
N/A - this PR only changes internal test/CI tooling; there is no end-user-facing or staging/production behavior to QA.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
n/a - automated tests only