test(run-store,webapp): cover the run-ops router at three shards - #4805
Draft
d-cs wants to merge 2 commits into
Draft
test(run-store,webapp): cover the run-ops router at three shards#4805d-cs wants to merge 2 commits into
d-cs wants to merge 2 commits into
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
|
Contributor
WalkthroughWebapp tests now use the production Merge Risk: 🔵 Low · up to The PR only changes tests, but two routing helpers use a legacy schema setting instead of the production dedicated setting, so the tests may not fully represent production behavior. The PR is mergeable with explicit owner follow-up to align that configuration. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and on-topic. It explains the coverage goals, test topology changes, mutation validation, and absence of production changes. It does not include the template's Closes, Checklist, Changelog, or Screenshots sections, but the key summary and testing information are present.
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required crumbs marker for the changed routing-store setup helpers before merge. Apply it at this helper and the corresponding helpers in
apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts,internal-packages/run-store/src/runOpsStore.envScopedResidency.test.ts,internal-packages/run-store/src/runOpsStore.newMethods.test.ts, andinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.ts.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b55be6bb-3d75-45a9-b9af-dc655f520f5e
📒 Files selected for processing (6)
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (26)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (11)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
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:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
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:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
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:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsinternal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.newMethods.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.tsinternal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
🧠 Learnings (3)
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/test/performTaskRunAlertsStoreRouting.test.tsapps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
internal-packages/run-store/src/runOpsStore.envScopedResidency.test.tsinternal-packages/run-store/src/runOpsStore.runKeyedRouting.test.ts
📚 Learning: 2026-08-21T14:26:14.909Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4752
File: internal-packages/run-store/src/runOpsStore.shardMap.test.ts:5-11
Timestamp: 2026-08-21T14:26:14.909Z
Learning: For these RoutingRunStore unit tests, use an instrumented fakeStore() with a shared ordered call log when verifying routing algebra such as sequential probe order and merge precedence. Use testcontainer-backed tests separately for database behavior, including mixed residency and replica-lag scenarios.
Applied to files:
internal-packages/run-store/src/runOpsStore.runKeyedRouting.test.ts
🔇 Additional comments (1)
internal-packages/run-store/src/runOpsStore.forWaitpointCompletion.test.ts (1)
6-9: LGTM!
Comment on lines
36
to
+39
| function buildRoutingStore(prisma17: PrismaClient, prisma14: PrismaClient) { | ||
| const newStore = new PostgresRunStore({ prisma: prisma17, readOnlyPrisma: prisma17 }); | ||
| const legacyStore = new PostgresRunStore({ prisma: prisma14, readOnlyPrisma: prisma14 }); | ||
| return new RoutingRunStore(newStore, legacyStore); | ||
| return new RoutingRunStore({ new: newStore, legacy: legacyStore }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'schemaVariant|newStore|findRun|updateMetadata' \
apps/webapp/app/v3/runStore.server.ts \
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts \
apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts \
internal-packages/run-store/srcRepository: triggerdotdev/trigger.dev
Length of output: 50382
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019 -type f -name '*.md' -maxdepth 3 -print
printf '%s\n' '--- target test helpers and call sites ---'
sed -n '1,130p' apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts
sed -n '1,120p' apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
printf '%s\n' '--- production store construction ---'
rg -n -C 10 'function buildRunStore|buildRunStore|schemaVariant' apps/webapp/app/v3/runStore.server.ts
printf '%s\n' '--- bound constructor and affected method branches ---'
rg -n -C 8 'class PostgresRunStore|constructor\\(|schemaVariant|async findRun\\(|findRun\\(|async updateMetadata|updateMetadata\\(' internal-packages/run-store/src --glob '*.ts' --glob '!*.test.ts'Repository: triggerdotdev/trigger.dev
Length of output: 21946
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- relevant conventions ---'
cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/conventions/apps-webapp.md
cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/conventions/apps-webapp-app-v3.md
cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/conventions/repo-wide.md
printf '%s\n' '--- PostgresRunStore definition ---'
rg -n -C 8 'class PostgresRunStore|schemaVariant|findRun|updateMetadata' internal-packages/run-store/src/PostgresRunStore.ts internal-packages/run-store/src --glob 'PostgresRunStore*.ts' --glob '!*.test.ts'
printf '%s\n' '--- exact target helper references ---'
rg -n -C 5 'buildRoutingStore|findRun|updateMetadata|schemaVariant|`@crumbs`' \
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts \
apps/webapp/test/updateMetadataStoreRoutingHetero.test.tsRepository: triggerdotdev/trigger.dev
Length of output: 50382
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- updateMetadata implementation ---'
sed -n '1538,1618p' internal-packages/run-store/src/PostgresRunStore.ts
printf '%s\n' '--- findRun projection helper and dedicated relation sets ---'
rg -n -C 6 '`#findTaskRunWithSelect`|TASK_RUN_DEDICATED|stripDedicatedRelations|hydrate' internal-packages/run-store/src/PostgresRunStore.ts
sed -n '2350,2470p' internal-packages/run-store/src/PostgresRunStore.ts
printf '%s\n' '--- affected service calls and assertions ---'
rg -n -C 10 'runStore\\.(findRun|updateMetadata)|findRun\\(|updateMetadata\\(' \
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts \
apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts \
apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts \
apps/webapp/app/services/metadata/updateMetadata.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 26738
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- target test call sites ---'
rg -n -C 12 'findRun|updateMetadata|buildRoutingStore' \
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts \
apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts
printf '%s\n' '--- service definitions and direct store calls ---'
rg -n -C 12 'findRun|updateMetadata' apps/webapp/app --glob '*.ts' | head -n 240Repository: triggerdotdev/trigger.dev
Length of output: 41522
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- alert service source file ---'
fd -i 'performTaskRunAlerts.server.ts' apps/webapp
fd -i 'updateMetadata.server.ts' apps/webapp
printf '%s\n' '--- exact routed reads ---'
alert_file="$(fd -i -t f 'performTaskRunAlerts.server.ts' apps/webapp | head -n 1)"
metadata_file="$(fd -i -t f 'updateMetadata.server.ts' apps/webapp | head -n 1)"
rg -n -C 14 'findRun|include|select' "$alert_file" "$metadata_file"
printf '%s\n' '--- exact findRun branch condition ---'
sed -n '2814,2835p' internal-packages/run-store/src/PostgresRunStore.tsRepository: triggerdotdev/trigger.dev
Length of output: 17064
Match the NEW-store configuration to production.
Production buildRunStore sets schemaVariant: "dedicated" for the NEW store, but both helpers omit it and default to "legacy". Both tested services call PostgresRunStore.findRun, which uses different code paths for these variants. Set schemaVariant: "dedicated" on both NEW stores.
📍 Affects 2 files
apps/webapp/test/performTaskRunAlertsStoreRouting.test.ts#L36-L39(this comment)apps/webapp/test/updateMetadataStoreRoutingHetero.test.ts#L25-L28
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
1 participant
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
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.