[Fix] Flaky Neon, Email Delivery, and Other Tests#1235
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughIncreased polling and retry windows for e2e email/OTP flows, replaced several fixed waits with polling, added outboxEmails to OTP failure payloads, extracted integration provisioning helpers into new modules, and updated tests to import/use those helpers and pass Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Contributor
Greptile SummaryThis PR targets test flakiness in the e2e suite by bumping polling retry counts, increasing inter-poll delays, replacing fixed sleeps with adaptive polling loops, and refactoring Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test: email.test.ts\nshould provide delivery statistics] --> B[sendEmail to user]
B --> C{Poll loop\ni < 40 × 500 ms}
C -->|stats.hour.sent >= 1| D[Assert capacity.penalty_factor\nAssert rate_per_second ≈ 2.7778\nAssert stats snapshot]
C -->|exhausted — no explicit error| D
E[Test: outbox-api.test.ts\npause/cancel email] --> F[Send email with\nslowTemplate 2000ms busy-wait]
F --> G{Poll loop\ni < 50 × 100 ms}
G -->|email found & PATCH paused| H[Assert paused state]
G -->|exhausted| I[Assertions fail\nwith pauseSucceeded check]
H --> J[Unpause email]
J --> K[await wait 10 000ms fixed]
K --> L[Assert status === sent]
M[Import refactor] --> N[provision.test.ts\nno longer exports helper]
N --> O[provision-helpers.ts\nneon + custom]
O --> P[transfer.test.ts\nwebhooks.test.ts\nprovision.test.ts]
Last reviewed commit: 95a600a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce flakiness in the e2e suite by replacing fixed sleeps with polling, increasing retry budgets, and extracting shared “provision project” helpers for Neon/custom integration tests.
Changes:
- Replaced a fixed wait in the email delivery stats test with polling, and loosened assertions around capacity rate precision.
- Increased mailbox/email polling retries and timeouts in several backend e2e tests.
- Extracted duplicated
provisionProjecthelpers into dedicated*-helpers.tsmodules and updated imports.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/e2e/tests/js/email.test.ts | Switches from fixed sleep to polling for delivery stats; adjusts assertions/snapshot scope. |
| apps/e2e/tests/helpers.ts | Increases mailbox polling retry count. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/webhooks.test.ts | Updates provisionProject import to shared helper module. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/transfer.test.ts | Updates provisionProject import to shared helper module. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision.test.ts | Uses shared helper; skips verification email wait during signup for reduced flakiness. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision-helpers.ts | New shared helper for Neon project provisioning test setup. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/transfer.test.ts | Updates provisionProject import to shared helper module. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.ts | Uses shared helper for custom project provisioning. |
| apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision-helpers.ts | New shared helper for custom project provisioning test setup. |
| apps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.ts | Increases polling budgets and extends slow-render template delay to reduce timing flakiness. |
| apps/e2e/tests/backend/endpoints/api/v1/auth/passkey/register.test.ts | Skips verification email wait during signup to avoid mailbox timing dependency. |
| apps/e2e/tests/backend/backend-helpers.ts | Increases OTP sign-in email polling attempts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.ts (1)
120-120:⚠️ Potential issue | 🟡 MinorTest description refers to "neon" but this tests the custom integration endpoint.
The test description says "should fail if the neon client details are missing" but this file tests
/api/v1/integrations/custom/projects/provision. This appears to be a copy-paste error from the Neon integration tests.📝 Suggested fix
-it("should fail if the neon client details are missing", async ({ expect }) => { +it("should fail if the client details are missing", async ({ expect }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.ts` at line 120, The test title mistakenly references "neon" while testing the custom integration endpoint; update the test's description string in the it(...) call (currently `it("should fail if the neon client details are missing", ...)`) to accurately describe the behavior for the custom integration (e.g., `should fail if the custom integration client details are missing`), and scan nearby tests in the same file for similar copy-paste titles to rename consistently.
🧹 Nitpick comments (1)
apps/e2e/tests/js/email.test.ts (1)
190-195: Consider adding a timeout error for explicit failure.The polling loop exits silently if the email is never delivered within 40 iterations. While subsequent assertions will fail, an explicit timeout error would provide clearer diagnostics.
Also, consider adding a brief comment for the
anytype usage per coding guidelines.💡 Suggested improvement
- let info: any; - for (let i = 0; i < 40; i++) { - info = await serverApp.getEmailDeliveryStats(); - if (info.stats.hour.sent >= 1) break; - await wait(500); - } + // Using `any` because getEmailDeliveryStats return type is complex and we're testing its structure + let info: any; + let delivered = false; + for (let i = 0; i < 40; i++) { + info = await serverApp.getEmailDeliveryStats(); + if (info.stats.hour.sent >= 1) { + delivered = true; + break; + } + await wait(500); + } + expect(delivered).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/js/email.test.ts` around lines 190 - 195, The polling loop using info and serverApp.getEmailDeliveryStats() should throw a clear timeout error if no emails are observed after the 40 iterations: after the loop, check whether info?.stats?.hour?.sent >= 1 and if not, throw a descriptive error (e.g., "Timed out waiting for email delivery after X ms/iterations") so test failures are explicit; also add a brief inline comment explaining the use of the any type for info (e.g., "info typed as any because response shape varies in E2E test") or replace any with an appropriate type/interface to satisfy coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.ts`:
- Line 120: The test title mistakenly references "neon" while testing the custom
integration endpoint; update the test's description string in the it(...) call
(currently `it("should fail if the neon client details are missing", ...)`) to
accurately describe the behavior for the custom integration (e.g., `should fail
if the custom integration client details are missing`), and scan nearby tests in
the same file for similar copy-paste titles to rename consistently.
---
Nitpick comments:
In `@apps/e2e/tests/js/email.test.ts`:
- Around line 190-195: The polling loop using info and
serverApp.getEmailDeliveryStats() should throw a clear timeout error if no
emails are observed after the 40 iterations: after the loop, check whether
info?.stats?.hour?.sent >= 1 and if not, throw a descriptive error (e.g., "Timed
out waiting for email delivery after X ms/iterations") so test failures are
explicit; also add a brief inline comment explaining the use of the any type for
info (e.g., "info typed as any because response shape varies in E2E test") or
replace any with an appropriate type/interface to satisfy coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ab4e177-fa1c-46bc-bdfa-55261ee9cfdb
📒 Files selected for processing (12)
apps/e2e/tests/backend/backend-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/auth/passkey/register.test.tsapps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/transfer.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/transfer.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/webhooks.test.tsapps/e2e/tests/helpers.tsapps/e2e/tests/js/email.test.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
N2D4
approved these changes
Mar 10, 2026
nams1570
force-pushed
the
fix-neon-register-flaky-tests
branch
from
95a600a to
b6bdd8d
Compare
Because provisionProject used to be exported from provision.test.ts, when the test failed in provision.test.ts, CI registered it as coming from webhooks.test.ts. This is obviously bad so we extract it to a shared helper. The neon flaky test in provision.test.ts doesn't actually need to verify the email. Plus, there's a flag in the signUpWithEmail function that exists for precisely the purpose of skipping the check. This e2e test isn't related to sign up emails at all and they aren't part of the critical path.
Same issue with it checking if email has arrived when other tests in our test suite already do that. We also bump up number of retries to make any other tests that depend on this more robust
Bumping up number of attempts. Since it's polling with early exit, if it finds it early it exits. So the average case shouldn't change.
for email test switching to a polling approach is the right fix, like the todo says. for outbox api it's dicier due to a race condition. We
nams1570
force-pushed
the
fix-neon-register-flaky-tests
branch
from
b6bdd8d to
9230513
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.ts (2)
940-953: Fail fast on terminal non-sentstates after unpausing.This loop waits the full 25 seconds even if the email has already settled into a terminal state like
skipped, or if the refetch itself stops succeeding. That makes CI slower and hides the actual regression behind a timeout.As per coding guidelines, "Fail early, fail loud. Fail fast with an error instead of silently continuing."♻️ Suggested shape
// Poll until the email is sent (since we unpaused it) for (let i = 0; ; i++) { const finalGetResponse = await niceBackendFetch(`/api/v1/emails/outbox/${emailId}`, { method: "GET", accessType: "server", }); - if (finalGetResponse.body.status === "sent") break; + if (finalGetResponse.status !== 200) { + throw new StackAssertionError("Failed to refetch email after unpause", { + response: finalGetResponse, + }); + } + if (finalGetResponse.body.status === "sent") break; + if (finalGetResponse.body.status === "skipped") { + throw new StackAssertionError("Email reached a terminal state after unpause", { + response: finalGetResponse.body, + }); + } if (i >= 50) { throw new StackAssertionError(`Timed out waiting for email to be sent after unpause`, { - status: finalGetResponse.body.status, + response: finalGetResponse.body, }); } await wait(500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.ts` around lines 940 - 953, The polling loop that checks finalGetResponse.body.status after unpausing should fail fast on terminal non-"sent" states or on a failed refetch instead of waiting the full retry window; update the loop around the niceBackendFetch call (the for loop using emailId, finalGetResponse, wait) to immediately throw a StackAssertionError when the fetch fails or when finalGetResponse.body.status is a terminal state (e.g., "skipped", "failed", "cancelled" — include the status in the error), and keep the existing timeout fallback only for non-terminal transient states.
887-916: Throw an explicit timeout error from these pause loops.If the email never becomes pausable, both tests fall through to a generic
expect(pauseSucceeded).toBe(true)failure and lose the last outbox state. Please use the sameStackAssertionError+outboxEmailspattern you already added around Line 673 so these flakes stay debuggable.As per coding guidelines, "Fail early, fail loud. Fail fast with an error instead of silently continuing."♻️ Suggested shape
for (let i = 0; i < 50; i++) { const listResponse = await niceBackendFetch("/api/v1/emails/outbox", { method: "GET", accessType: "server", }); const emails = listResponse.body.items.filter((e: any) => e.to?.user_id === userId); if (emails.length > 0) { emailId = emails[0].id; // Try to pause it const pauseResponse = await niceBackendFetch(`/api/v1/emails/outbox/${emailId}`, { method: "PATCH", accessType: "server", body: { is_paused: true, }, }); if (pauseResponse.status === 200 && pauseResponse.body.status === "paused") { pauseSucceeded = true; break; } } await wait(100); } - expect(emailId).not.toBeNull(); - expect(pauseSucceeded).toBe(true); + if (!pauseSucceeded || emailId == null) { + throw new StackAssertionError("Timeout waiting to pause email", { + outboxEmails: await getOutboxEmails(), + userId, + }); + }Also applies to: 1008-1037
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.ts` around lines 887 - 916, The pause loop that polls /api/v1/emails/outbox (using niceBackendFetch) must throw an explicit timeout StackAssertionError with the current outboxEmails snapshot if it never finds a pausable email; replace the final generic expect(pauseSucceeded).toBe(true) failure with the same pattern used around Line 673 (create and throw new StackAssertionError with a descriptive message and include outboxEmails), and do the same for the second loop (lines 1008-1037) so tests fail fast and include the last observed outbox state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/tests/js/email.test.ts`:
- Around line 190-198: The polling loop currently only waits for
info.stats.hour.sent to become 1 but later asserts day/week/month counters too,
causing flakiness; update the loop that calls serverApp.getEmailDeliveryStats()
to break only when all asserted counters match (e.g., info.stats.hour.sent === 1
&& info.stats.day.sent === 1 && info.stats.week.sent === 1 &&
info.stats.month.sent === 1), keep the existing timeout/throw behavior, and
return the final info value from the loop so the subsequent assertions use the
polled result (removing the implicit any).
---
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.ts`:
- Around line 940-953: The polling loop that checks finalGetResponse.body.status
after unpausing should fail fast on terminal non-"sent" states or on a failed
refetch instead of waiting the full retry window; update the loop around the
niceBackendFetch call (the for loop using emailId, finalGetResponse, wait) to
immediately throw a StackAssertionError when the fetch fails or when
finalGetResponse.body.status is a terminal state (e.g., "skipped", "failed",
"cancelled" — include the status in the error), and keep the existing timeout
fallback only for non-terminal transient states.
- Around line 887-916: The pause loop that polls /api/v1/emails/outbox (using
niceBackendFetch) must throw an explicit timeout StackAssertionError with the
current outboxEmails snapshot if it never finds a pausable email; replace the
final generic expect(pauseSucceeded).toBe(true) failure with the same pattern
used around Line 673 (create and throw new StackAssertionError with a
descriptive message and include outboxEmails), and do the same for the second
loop (lines 1008-1037) so tests fail fast and include the last observed outbox
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d94d47a5-75ad-4484-900d-24ca344ef31d
📒 Files selected for processing (12)
apps/e2e/tests/backend/backend-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/auth/passkey/register.test.tsapps/e2e/tests/backend/endpoints/api/v1/emails/outbox-api.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/transfer.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision-helpers.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/transfer.test.tsapps/e2e/tests/backend/endpoints/api/v1/integrations/neon/webhooks.test.tsapps/e2e/tests/helpers.tsapps/e2e/tests/js/email.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/transfer.test.ts
- apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision.test.ts
- apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/provision-helpers.ts
- apps/e2e/tests/helpers.ts
- apps/e2e/tests/backend/backend-helpers.ts
- apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/transfer.test.ts
- apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/projects/provision-helpers.ts
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
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 of Changes
Just bumped up polling, removed unnecessary wait checks in tests that don't need them. Minor changes, not an exhaustive list of flaky test fixes
Note that importing a function into a file B that was exported from a test file A causes vitest to see all the tests in test file A as being under file B. This messes up CI and makes it harder to track failing tests.
Summary by CodeRabbit