fix(tests): fix flaky getSnapshotsSince test#3103
Conversation
|
WalkthroughThe test file Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.scratch/plan-graceful-oversized-batch-items.md (1)
1-5: Consider removing this scratch file before merging.Planning documents under
.scratch/are useful during development but generally shouldn't land on the main branch — they'll accumulate as permanent noise and can be confusing to future contributors. The design intent is already captured in the PR description and in the code itself. Consider deleting the file before merging, or moving it to an ADR/docs location if the design rationale should be preserved long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.scratch/plan-graceful-oversized-batch-items.md around lines 1 - 5, Remove or relocate the scratch planning document .scratch/plan-graceful-oversized-batch-items.md before merging: either delete the file to avoid committing transient notes to main, or move its contents into a permanent location (e.g., docs/ or an ADR) if the design rationale must be preserved; ensure the PR description or the new docs reference PR `#2980` so the design remains discoverable.internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts (1)
194-205: Polling loop is a clear fix — consider adding a post-loop assertion for better diagnostics.The replacement of a blind fixed wait with a deterministic poll is the right approach for a timer-based waitpoint whose completion depends on the background worker. The fix addresses the root flakiness.
One mild gap: if the background worker is delayed beyond 5 s (slow CI), the loop silently exhausts all iterations and the test fails at
expect(allSnapshots.length).toBeGreaterThanOrEqual(3)with a confusing count mismatch rather than a clear "waitpoint never completed" message. Adding a post-loop assertion on the status (or on the latest DB row) would give an immediately actionable failure message.The trailing
await setTimeout(200)is consistent with the rest of the file (setTimeout(500)at lines 80, 297, 333) so it doesn't introduce new flakiness.🔍 Optional: assert waitpoint completion after loop
for (let i = 0; i < 50; i++) { await setTimeout(100); const wp = await prisma.waitpoint.findFirst({ where: { id: waitpoint.id }, select: { status: true }, }); if (wp?.status === "COMPLETED") break; } + // Fail fast with a meaningful message if the background worker never fired + const completedWp = await prisma.waitpoint.findFirst({ + where: { id: waitpoint.id }, + select: { status: true }, + }); + expect(completedWp?.status, "waitpoint should be COMPLETED before checking snapshots").toBe("COMPLETED"); + // Allow time for the snapshot to be created after waitpoint completion await setTimeout(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts` around lines 194 - 205, The polling loop that checks prisma.waitpoint.findFirst for the waitpoint status may exhaust without explicit failure, so after the loop add a clear assertion that the waitpoint reached "COMPLETED" (e.g., re-query prisma.waitpoint.findFirst({ where: { id: waitpoint.id }, select: { status: true } }) and expect(wp?.status).toBe("COMPLETED")), and if not include a descriptive failure message mentioning the waitpoint.id to aid debugging; this replaces silent exhaustion and makes test failures explicit before proceeding to snapshot assertions.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.scratch/plan-graceful-oversized-batch-items.mdinternal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `tasks.batchTrigger()` to trigger multiple runs of a single task with different payloads
Applied to files:
.scratch/plan-graceful-oversized-batch-items.md
🪛 markdownlint-cli2 (0.21.0)
.scratch/plan-graceful-oversized-batch-items.md
[warning] 216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.scratch/plan-graceful-oversized-batch-items.md:
- Around line 216-239: The fenced code block containing the NDJSON data-flow
diagram lacks a language specifier (causing markdownlint MD040); update the
opening fence for that diagram (the block starting with "NDJSON bytes arrive")
to include a language such as text or mermaid (e.g., change ``` to ```text) so
the linter warning is resolved while preserving the diagram content.
---
Nitpick comments:
In @.scratch/plan-graceful-oversized-batch-items.md:
- Around line 1-5: Remove or relocate the scratch planning document
.scratch/plan-graceful-oversized-batch-items.md before merging: either delete
the file to avoid committing transient notes to main, or move its contents into
a permanent location (e.g., docs/ or an ADR) if the design rationale must be
preserved; ensure the PR description or the new docs reference PR `#2980` so the
design remains discoverable.
In `@internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts`:
- Around line 194-205: The polling loop that checks prisma.waitpoint.findFirst
for the waitpoint status may exhaust without explicit failure, so after the loop
add a clear assertion that the waitpoint reached "COMPLETED" (e.g., re-query
prisma.waitpoint.findFirst({ where: { id: waitpoint.id }, select: { status: true
} }) and expect(wp?.status).toBe("COMPLETED")), and if not include a descriptive
failure message mentioning the waitpoint.id to aid debugging; this replaces
silent exhaustion and makes test failures explicit before proceeding to snapshot
assertions.
ericallam
force-pushed
the
ea-branch-120
branch
from
f7fb35a to
3f2133e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts (1)
194-202: Add an assertion after the polling loop to surface timeout failures explicitly.If the waitpoint never reaches
"COMPLETED"within 50 iterations (5 s), the loop exits silently and the test continues, eventually failing at downstream snapshot assertions with misleading error messages rather than a clear "waitpoint did not complete in time" diagnostic. This partially re-introduces the same flakiness the PR is meant to fix.🛡️ Proposed fix
// Poll until the waitpoint is completed by the background worker + let waitpointCompleted = false; for (let i = 0; i < 50; i++) { await setTimeout(100); const wp = await prisma.waitpoint.findFirst({ where: { id: waitpoint.id }, select: { status: true }, }); - if (wp?.status === "COMPLETED") break; + if (wp?.status === "COMPLETED") { + waitpointCompleted = true; + break; + } } + expect(waitpointCompleted, "Waitpoint did not reach COMPLETED status within 5 s").toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts` around lines 194 - 202, The polling loop in getSnapshotsSince.test.ts that checks prisma.waitpoint.findFirst for waitpoint.id may exit silently if the waitpoint never reaches "COMPLETED", causing downstream misleading failures; after the for-loop, assert that the fetched waitpoint status is "COMPLETED" (or throw a clear error like "waitpoint did not complete in time") by re-reading the waitpoint via prisma.waitpoint.findFirst (using the same waitpoint.id) or by storing the last wp and asserting wp?.status === "COMPLETED" to surface a clear timeout failure for the waitpoint.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
🔇 Additional comments (1)
internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts (1)
197-201: The string"COMPLETED"matches the actual Prisma schema value. TheWaitpointStatusenum ininternal-packages/database/prisma/schema.prismadefinesCOMPLETEDas a valid value, so the comparisonwp?.status === "COMPLETED"at lines 197-201 functions correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal-packages/run-engine/src/engine/tests/getSnapshotsSince.test.ts`:
- Around line 194-202: The polling loop in getSnapshotsSince.test.ts that checks
prisma.waitpoint.findFirst for waitpoint.id may exit silently if the waitpoint
never reaches "COMPLETED", causing downstream misleading failures; after the
for-loop, assert that the fetched waitpoint status is "COMPLETED" (or throw a
clear error like "waitpoint did not complete in time") by re-reading the
waitpoint via prisma.waitpoint.findFirst (using the same waitpoint.id) or by
storing the last wp and asserting wp?.status === "COMPLETED" to surface a clear
timeout failure for the waitpoint.
2 participants
No description provided.