perf(test): eliminate SQLite fsync and real rate-limit backoff wait in tests#8553
Merged
Merged
Conversation
…n tests Profiling queue-lifecycle-guards.test.ts (33.7s) and backfill.test.ts (19.2s) found two more real per-test costs, confirmed via a CPU profile showing wall time going to idle (real I/O wait), not compute. SQLite fsyncs on every autocommit write by default; TestD1Database's clones are real temp files (needed for the template-clone trick from the earlier fix), so every app .prepare(...).run() paid a real fsync. journal_mode=MEMORY + synchronous=OFF are safe for a throwaway clone unlinked as soon as it's open -- crash-consistency durability is meaningless for data nothing outlives the test to read back. Benchmarked: 50 inserts, 13.9ms default vs 0.6ms tuned. fetchWithGitHubRetry's exponential backoff (500/1000/2000ms real setTimeout) is legitimate production behavior for a genuine 403/429, but several tests deliberately drive a fixture through the full retry loop to assert the resulting sync state, not the delay -- paying the full 3.5s every time. client.ts gains setGithubRateLimitRetrySleepCapMsForTest(), capping only what is actually awaited, not rateLimitRetryMs's return value -- its dedicated pure-function backoff-math test stays untouched and still exercises the real numbers. Both wired once, suite-wide, via a new test/helpers/vitest-setup.ts (vitest.config.ts setupFiles) -- same ...ForTest convention as clearInstallationTokenCacheForTest, applied automatically so every current AND future test with this shape benefits without opting in. queue-lifecycle-guards.test.ts: 33.7s -> 7.2s. backfill.test.ts: 19.2s -> 1.7s.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8553 +/- ##
==========================================
+ Coverage 89.58% 92.50% +2.92%
==========================================
Files 97 793 +696
Lines 22706 79552 +56846
Branches 3872 24036 +20164
==========================================
+ Hits 20341 73592 +53251
- Misses 2187 4800 +2613
- Partials 178 1160 +982
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This was referenced Jul 24, 2026
Merged
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
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.
Summary
Third follow-up in the CI-speed sequence. Profiling
queue-lifecycle-guards.test.tsandbackfill.test.ts(still 33s and 19s respectively after the earlier fixes) found two more real per-test costs, both confirmed via a CPU profile that showed the wall time going to idle (waiting on real I/O), not application compute:TestD1Database's clones are real temp files (needed for the template-clone trick), and SQLite's defaults (journal_mode=DELETE,synchronous=FULL) fsync on every autocommit statement — every.prepare(...).run()in app code is its own implicit transaction. Benchmarked: 50 inserts, 13.9ms default vs 0.6ms tuned (23×).journal_mode=MEMORY+synchronous=OFFare safe for a throwaway clone that's unlinked as soon as it's open — crash-consistency durability is meaningless for data nothing outlives the test to read back.fetchWithGitHubRetry's exponential backoff (500ms → 1000ms → 2000ms, realsetTimeout) is legitimate production behavior for a genuine 403/429 — but several tests deliberately drive a fixture through the full retry loop to assert the resulting sync state (rate-limited status, capped segments), not the delay itself, and paid the full 3.5s wall time for it every time.client.tsgainssetGithubRateLimitRetrySleepCapMsForTest()— it caps only what's actuallyawaited, notrateLimitRetryMs's return value, so the dedicated pure-function test asserting its exact backoff math (github-app.test.ts) is completely untouched and still exercises the real numbers.Both defaults are wired once, suite-wide, via a new
test/helpers/vitest-setup.ts(registered invitest.config.ts'ssetupFiles) — the same...ForTestoverride convention asclearInstallationTokenCacheForTest/setMaxChunksPerRepoForTest, applied automatically so every current and future test with this shape benefits without an author needing to opt in.Measured:
queue-lifecycle-guards.test.ts33.7s → 7.2s;backfill.test.ts19.2s → 1.7s.Validation
npm run typecheckclean.npm run test:coverage(full, unsharded): 21,482 passed / 0 failed.github-app.test.ts's dedicatedrateLimitRetryMsbackoff-math tests still pass unmodified, confirming the override only touches the awaited delay, not the function's own correctness.