test(run-store,webapp): cover the run-ops router at three shards - #4805
Conversation
The fake-slot router suites only ever built two stores, so several rules that only apply above two were untestable by construction. clearIdempotencyKey is the sole caller of the "every other shard" helper, and with two stores that helper returns a single entry, which hides a take-the-first bug. The absent-id partition has the same blind spot: a gen-2 id and a cuid select the same store when only one other store exists. Three suites now run at two shards and at three. Each new assertion was checked by mutating the router to the failure it guards, confirming the three-shard arm fails while the two-shard arm still passes, then reverting. Two webapp tests defined their own local RoutingRunStore. They compiled against a two-store model whatever the real class did, and one described a routing rule the code never implemented. Both now build the real router over the two Postgres stores they already create. No production code changes.
…parameterization-tri-13433
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (32)
🧰 Additional context used📓 Path-based instructions (11)We use vitest exclusively. **Never mock anything** - use testcontainers instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
Test files must not import `app/env.server.ts`; pass configuration as options instead.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging. Mark lines with📄 CodeRabbit inference engine (AGENTS.md) Files:
Use zod for validation in packages/core and apps/webapp📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🔇 Additional comments (2)
WalkthroughWebapp tests now use the production Merge Risk: ⚪ Minimal · up to This PR expands coverage for existing two- and three-shard routing behavior and uses the production router in webapp tests without changing runtime behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the test-only changes, affected suites, routing behaviors, validation steps, and the absence of production code changes. It does not use every template section, but the missing checklist, changelog, and screenshots are non-critical for this test-focused change.
✨ Finishing Touches 💡 1📝 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 |
![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}
Production builds the NEW run-ops store with schemaVariant "dedicated", but both hetero routing tests omitted it, so the store defaulted to "legacy". The two variants take different read paths: findRun routes through the dedicated relation-stripping adapter only when the variant is set, so the tests were exercising the legacy path for a store that is dedicated in production. Verified the branch is now reached by making the dedicated arm throw, which failed 3 of the 5 tests.
Resolves the two conflicts in the store-routing tests. Upstream (#4805) replaced each file's hand-rolled test-local RoutingRunStore shim with the real RoutingRunStore from @internal/run-store, so the shim's removeTags forwarder this branch had added is obsolete; the real router already carries removeTags. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Summary
Several of the run-ops router's rules only apply above two stores, and the fake-slot suites only ever built two, so those rules were untestable by construction.
clearIdempotencyKeyis the sole caller of the "every other shard" helper, and with two stores that helper returns a single entry, which hides a take-the-first bug. The absent-id partition has the same blind spot: a gen-2 id and a cuid select the same store when only one other store exists.Three suites now run at two shards and at three, with the expected value indexed by topology wherever the rule genuinely changes. The fourth stays at two and says why in the file, because its N-shard behaviour is already pinned in
runOpsStore.shardMap.test.ts.Two webapp tests defined their own local
RoutingRunStore. They compiled against a two-store model whatever the real class did, and one described a routing rule the code never implemented. Both now build the real router over the two Postgres stores they already create.Validating a test-only change
Every new assertion passed the first time it ran, which proves nothing. Each was checked by breaking the router in the way the test claims to guard, then confirming the failure lands in the three-shard arm while the two-shard arm still passes:
expected +0 to be 1, the shape a silently under-counted waitpoint takesEach mutation was reverted. No production code changes.
One note for anyone extending these: the webapp resolves
@internal/run-storetodist/, not to source, so a source edit without a rebuild makes those two tests assert against the previous router and pass.