Clean up globals patching, supports vitest#222
Merged
ianmacartney merged 9 commits intomainfrom Mar 30, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes introduce a comprehensive end-to-end testing infrastructure for Convex workflows. A new e2e testing module exposes endpoints to start multiple workflows and poll their status, paired with an executable Node.js script that orchestrates workflow execution, approval handling, and terminal state validation. Supporting changes update environment cleanup patterns, enable previously skipped tests, and modify test workflows to remove timing delays. Changes
Sequence DiagramsequenceDiagram
participant Client as E2E Client
participant API as Convex API
participant WF as Workflows
participant State as Workflow State
Client->>API: startAll()
activate API
API->>WF: start exampleWorkflow, catchErrorWorkflow, parentWorkflow, signalBasedWorkflow, confirmationWorkflow
API-->>Client: WorkflowIds
deactivate API
loop Status Polling
Client->>API: statusAll(workflowIds)
API->>State: query workflow status
State-->>API: WorkflowStatus[]
API-->>Client: Status updates
end
Client->>API: approveConfirmation(workflowId)
activate API
API->>WF: execute chooseProposal mutation
WF->>State: update confirmation workflow
State-->>WF: completed
deactivate API
par Parallel Monitoring
WF-->>State: transitions (executing → completed/failed/canceled)
and
Client->>Client: track terminal states
end
Client->>Client: validate all workflows reached terminal state
Client->>Client: report results and exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
ianmacartney
changed the title
ianmacartney
changed the title
commit: |
ianmacartney
force-pushed
the
ian/vitest-compat
branch
from
6abc0b0 to
bbee0d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
example/convex/inlineTest.ts (1)
63-64: Consider removing the “re-addsetTimeout” TODO.Keeping this note may encourage reintroducing nondeterministic timing in tests; prefer deleting it or moving rationale to a tracked issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/inlineTest.ts` around lines 63 - 64, Remove the in-code TODO and commented setTimeout lines in example/convex/inlineTest.ts (the "// TODO: use setTimeout..." and the commented await new Promise(...) line) to avoid encouraging nondeterministic timing in tests; if you need to preserve the rationale, create a tracked issue in the repo and replace the TODO with a one-line comment linking to that issue number or add the issue ID to the PR description instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@example/convex/inlineTest.ts`:
- Around line 63-64: Remove the in-code TODO and commented setTimeout lines in
example/convex/inlineTest.ts (the "// TODO: use setTimeout..." and the commented
await new Promise(...) line) to avoid encouraging nondeterministic timing in
tests; if you need to preserve the rationale, create a tracked issue in the repo
and replace the TODO with a one-line comment linking to that issue number or add
the issue ID to the PR description instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7135d92-ab5c-4596-893f-5e63d664eee8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
example/convex/example.test.tsexample/convex/inlineTest.test.tsexample/convex/inlineTest.tspackage.jsonsrc/client/environment.tssrc/client/workflowMutation.tssrc/test.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/workflowMutation.ts
- src/test.ts
- src/client/environment.ts
- example/convex/example.test.ts
- example/convex/inlineTest.test.ts
ianmacartney
force-pushed
the
ian/vitest-compat
branch
from
b317cb8 to
cf8af43
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
example/convex/setup.test.ts (1)
11-12: Use a typed cast instead ofas anyto document the type bridge requirement.Line 12 bypasses type-safety due to an alpha version incompatibility between
convex-testand@convex-dev/workflow/test. Replace the untyped cast with an explicit type parameter and update the TODO to reference the upstream dependency constraint.Proposed patch
export function initConvexTest() { const t = convexTest(schema, modules); - // TODO: figure out why this isn't working - workflow.register(t as any); + // TODO: remove this cast once convex-test stable release resolves type compatibility + workflow.register(t as Parameters<typeof workflow.register>[0]); return t; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/setup.test.ts` around lines 11 - 12, Replace the untyped cast in the call to workflow.register(t as any) with an explicit type bridge by supplying the correct generic/type parameter for workflow.register (e.g., workflow.register<YourTestType>(t)) so the cast documents the intended shape instead of using any; also update the TODO comment to mention the upstream dependency incompatibility between convex-test and `@convex-dev/workflow/test` (alpha mismatch) so future readers know why the explicit typed bridge is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@example/convex/setup.test.ts`:
- Around line 11-12: Replace the untyped cast in the call to workflow.register(t
as any) with an explicit type bridge by supplying the correct generic/type
parameter for workflow.register (e.g., workflow.register<YourTestType>(t)) so
the cast documents the intended shape instead of using any; also update the TODO
comment to mention the upstream dependency incompatibility between convex-test
and `@convex-dev/workflow/test` (alpha mismatch) so future readers know why the
explicit typed bridge is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca0bd263-f87f-48b3-af71-656582c61ddb
📒 Files selected for processing (1)
example/convex/setup.test.ts
ianmacartney
force-pushed
the
ian/vitest-compat
branch
from
cf8af43 to
2b79db6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
example/convex/example.test.ts (1)
61-71: Assert an observable effect fromparentWorkflow.This only proves the workflow reached a terminal state. If the child invocation or follow-up mutation regresses but the parent still completes, this test still passes. Please assert the returned value or a persisted side effect that proves both steps ran.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.test.ts` around lines 61 - 71, The test only asserts workflow.status is "completed" — change it to assert an observable effect from parentWorkflow by either retrieving the workflow result or verifying a persisted side-effect: capture the return value from workflow.start (or call workflow.result/getResult equivalent) for internal.nestedWorkflow.parentWorkflow, or query the mutation/state change the parent or its child should have produced after await t.finishAllScheduledFunctions(vi.runAllTimers) using t.run to read the database or read API; update the expect to assert that returned value or persisted record demonstrates both the child invocation and follow-up mutation ran.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/e2e.ts`:
- Around line 12-53: The public test helpers startAll (and similarly statusAll
and approveConfirmation) are currently exposed as top-level mutation/query
handlers; change their definitions to use internalMutation/internalQuery instead
(e.g., replace mutation({ ... }) with internalMutation({ ... }) and query({ ...
}) with internalQuery({ ... })), update any imports/exports that reference these
symbols, and ensure they are invoked only from a dedicated internal test
endpoint or directly from test scripts rather than being publicly reachable;
keep the handler signatures and returned payloads the same while limiting their
registration to the internal API surface.
In `@example/e2e.mjs`:
- Around line 57-77: The approval retry loop (tryApprove) currently only logs
after exhausting attempts and leaves the confirmation workflow stuck inProgress;
change it to fail the main run by throwing (or rejecting) when the final attempt
fails and ensure any approvalTimer is cleared so the outer async flow sees the
error; specifically modify tryApprove/approvalTimer handling so that on the last
failed attempt (attempt >= 5 and !approvalDone) you clearTimeout(approvalTimer)
and throw a descriptive Error (or reject the parent promise) instead of just
console.error, and consider replacing the fixed APPROVAL_DELAY_MS trigger with
polling the confirmation workflow state before attempting approval.
- Around line 23-29: The loop that loads .env.local assigns raw values
(process.env[match[1]] = match[2]) leaving trailing CR and surrounding quotes;
update the assignment in the for loop (the block that processes envLocal and
match[2]) to normalize the value by trimming whitespace (including \r) and
removing optional surrounding single or double quotes before setting
process.env[match[1]] so CONVEX_URL and other vars are usable by ConvexClient.
---
Nitpick comments:
In `@example/convex/example.test.ts`:
- Around line 61-71: The test only asserts workflow.status is "completed" —
change it to assert an observable effect from parentWorkflow by either
retrieving the workflow result or verifying a persisted side-effect: capture the
return value from workflow.start (or call workflow.result/getResult equivalent)
for internal.nestedWorkflow.parentWorkflow, or query the mutation/state change
the parent or its child should have produced after await
t.finishAllScheduledFunctions(vi.runAllTimers) using t.run to read the database
or read API; update the expect to assert that returned value or persisted record
demonstrates both the child invocation and follow-up mutation ran.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6813be1d-37e8-42d9-a598-ade17880cb9f
⛔ Files ignored due to path filters (2)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
eslint.config.jsexample/convex/e2e.tsexample/convex/example.test.tsexample/convex/inlineTest.test.tsexample/convex/inlineTest.tsexample/e2e.mjspackage.jsonsrc/client/environment.tssrc/client/workflowMutation.tssrc/test.ts
✅ Files skipped from review due to trivial changes (3)
- eslint.config.js
- example/convex/inlineTest.test.ts
- src/client/environment.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test.ts
- package.json
- example/convex/inlineTest.ts
- src/client/workflowMutation.ts
Comment on lines
+23
to
+29
| if (!process.env.CONVEX_URL) { | ||
| try { | ||
| const envLocal = readFileSync(".env.local", "utf-8"); | ||
| for (const line of envLocal.split("\n")) { | ||
| const match = line.match(/^(\w+)=(.+?)(\s*#.*)?$/); | ||
| if (match) process.env[match[1]] = match[2]; | ||
| } |
There was a problem hiding this comment.
Normalize .env.local values before assigning them.
This parser preserves trailing \r on CRLF files and keeps surrounding quotes, so a valid CONVEX_URL can become unusable. Trim and unquote the captured value before passing it to ConvexClient.
Minimal fix
- if (match) process.env[match[1]] = match[2];
+ if (match) {
+ const value = match[2].trim().replace(/^(['"])(.*)\1$/, "$2");
+ process.env[match[1]] = value;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!process.env.CONVEX_URL) { | |
| try { | |
| const envLocal = readFileSync(".env.local", "utf-8"); | |
| for (const line of envLocal.split("\n")) { | |
| const match = line.match(/^(\w+)=(.+?)(\s*#.*)?$/); | |
| if (match) process.env[match[1]] = match[2]; | |
| } | |
| if (!process.env.CONVEX_URL) { | |
| try { | |
| const envLocal = readFileSync(".env.local", "utf-8"); | |
| for (const line of envLocal.split("\n")) { | |
| const match = line.match(/^(\w+)=(.+?)(\s*#.*)?$/); | |
| if (match) { | |
| const value = match[2].trim().replace(/^(['"])(.*)\1$/, "$2"); | |
| process.env[match[1]] = value; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/e2e.mjs` around lines 23 - 29, The loop that loads .env.local assigns
raw values (process.env[match[1]] = match[2]) leaving trailing CR and
surrounding quotes; update the assignment in the for loop (the block that
processes envLocal and match[2]) to normalize the value by trimming whitespace
(including \r) and removing optional surrounding single or double quotes before
setting process.env[match[1]] so CONVEX_URL and other vars are usable by
ConvexClient.
Comment on lines
+57
to
+77
| const approvalTimer = setTimeout(async () => { | ||
| const tryApprove = async (attempt) => { | ||
| try { | ||
| console.log(`Approving confirmation workflow (attempt ${attempt})...`); | ||
| await client.mutation(api.e2e.approveConfirmation, { | ||
| workflowId: ids.confirmation, | ||
| }); | ||
| approvalDone = true; | ||
| console.log("Confirmation approved."); | ||
| } catch (e) { | ||
| const msg = e.message; | ||
| if (attempt < 5 && !approvalDone) { | ||
| console.log(` Approval not ready: ${msg.slice(0, 80)}. Retrying...`); | ||
| await new Promise((r) => setTimeout(r, 2000)); | ||
| return tryApprove(attempt + 1); | ||
| } | ||
| console.error(` Approval failed after ${attempt} attempts: ${msg}`); | ||
| } | ||
| }; | ||
| await tryApprove(1); | ||
| }, APPROVAL_DELAY_MS); |
There was a problem hiding this comment.
Fail fast when approval exhausts its retries.
After the last failed approveConfirmation attempt this only logs and returns, so the confirmation workflow stays inProgress and the script eventually fails with the unrelated global timeout. Reject the main run here, and ideally trigger approval from observed workflow state instead of a fixed 5s delay.
Also applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/e2e.mjs` around lines 57 - 77, The approval retry loop (tryApprove)
currently only logs after exhausting attempts and leaves the confirmation
workflow stuck inProgress; change it to fail the main run by throwing (or
rejecting) when the final attempt fails and ensure any approvalTimer is cleared
so the outer async flow sees the error; specifically modify
tryApprove/approvalTimer handling so that on the last failed attempt (attempt >=
5 and !approvalDone) you clearTimeout(approvalTimer) and throw a descriptive
Error (or reject the parent promise) instead of just console.error, and consider
replacing the fixed APPROVAL_DELAY_MS trigger with polling the confirmation
workflow state before attempting approval.
goffrie
approved these changes
Mar 27, 2026
This was referenced Apr 1, 2026
This was referenced Apr 11, 2026
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
2 participants
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.
Environment cleanup and test improvements
Requires fixes currently in get-convex/convex-test#76
Summary by CodeRabbit
Tests
Chores