test(testcontainers): hoist container boot off the test timer - #4686
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe testcontainers package now uses 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Vitest bills fixture setup to the first test's testTimeout - the fixture chain runs inside the test timer - so the one-off worker container boot landed on whichever test resolved it first and consumed a budget sized for test work. On fork PRs, which get no secrets and so skip the CI image pre-pull, that added ~10s and pushed five webapp shards past their 60s cap. Internal runs cleared it by under 10s, so it was a latent flake there too. Registering the boot as a beforeAll with its own timeout moves it off the test clock. Registration is lazy, so only files that actually touch a fixture family pay for it, and happens once per file since isolate gives each file a fresh module registry.
000782b to
8aa5670
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/devin review |
|
Starting Devin Review. 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
What
The one-off worker container boot is billed to whichever test resolves the fixture first. This moves it into a
beforeAllwith its own timeout.Why
vitest runs the fixture chain inside the test timer:
There is no
fixtureTimeout. So booting Postgres (plusCREATE DATABASE, schema push, ClickHouse and Redis) lands on the first test and consumes a budget sized for test work.That is why losing the image pre-pull on fork PRs was fatal rather than merely slower: the extra ~10s crossed the 60s cap. Since fork time is roughly internal + 10s and forks exceed 60s, internal runs were already clearing that cap by under 10s — a latent flake regardless of forks.
How
withWarmupwraps each fixture family and lazily registers abeforeAllon first touch, with its own generous timeout. Registration is lazy so only files that actually use a family pay for it —@internal/testcontainersis imported by hundreds of test files, many of which only need Redis. It registers once per file, sinceisolategives each file a fresh module registry.Eight families are wrapped.
isolatedRedisTest,replicationContainerTestandpostgresAndRedisTestare deliberately untouched: they use per-test containers by design, so there is no one-off boot to hoist.No test file or CI changes, and it applies to every package using these fixtures.
Verification
Proven by mutation.
src/warmup.test.tsruns container tests under a deliberately tight cap:Test timed outIt is kept as a regression test — without it, unwrapping a fixture would break nothing visibly.
triggerFailedTask.call.test.ts, one of the five shard casualties, passes locally in 20.4s.Also here
@internal/testcontainershad notestscript, soturbo run test --filter "@internal/*"skipped the package and its existingheteroDedicated.test.tsnever ran in CI. Adding the script (matching the sibling packages') runs both files; verified green through turbo exactly as CI invokes it.