Playwright test failures #581
Conversation
Add retries to email reading and wait for redirect in call test. Co-authored-by: me <me@kentcdodds.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughIntroduces retry/wait logic: e2e tests now wait for redirects and element visibility and use a retryable email reader; utility adds configurable retry/sleep; server-side gravatar check switched from AbortController to a Promise.race-based fetchWithTimeout. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
@cursor, playwright is failing in CI. Please fix that. |
|
I've pushed the fix. The CI failure was caused by importing The fix removes that import since the session utilities work correctly without it - the environment variables are already set by the web server. Summary of all changes in this PR:
Learn more about Cursor Agents |
The import of entry.server.tsx in the test utils was causing a Node.js stream assertion failure and esbuild deadlock in CI, preventing the Playwright web server from starting. This import was previously added to initialize global.ENV, but it's not needed since the session utilities work correctly with the environment variables already set.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/utils.ts (1)
30-64: LGTM! Robust retry mechanism for email reading.The retry logic effectively handles both transient file read errors and delayed email availability. The configurable parameters with sensible defaults (5 retries, 200ms delay) provide flexibility for different test scenarios. Error logging aids in debugging when issues occur.
The TODO comment on line 40 mentions adding validation for the email structure. Would you like me to generate an implementation that validates the email object against the
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/calls.spec.tse2e/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/calls.spec.ts (1)
e2e/utils.ts (1)
readEmail(30-64)
e2e/utils.ts (1)
content/blog/avoid-the-test-user/user-profile-example.jsx (1)
sleep(14-17)
🪛 ast-grep (0.40.4)
e2e/calls.spec.ts
[warning] 53-53: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(title, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: 🎭 Playwright
🔇 Additional comments (4)
e2e/calls.spec.ts (3)
48-49: LGTM! Explicit redirect wait improves test stability.The explicit wait for URL redirect after form submission properly handles the asynchronous navigation, preventing the test from proceeding before the call record is created. The 10-second timeout is reasonable for this operation.
54-56: LGTM! Visibility wait prevents race condition.The explicit wait for the call link to be visible before clicking addresses the race condition where the test might attempt to interact with elements before they're fully rendered. The 10-second timeout is appropriate.
Note: The static analysis warning about RegExp construction from a variable (line 54) is a false positive in this case, since
titleis generated byfaker.lorem.words(2)(line 9) and is fully controlled, not user input.
73-77: LGTM! Retry mechanism handles asynchronous email delivery.The retry configuration (10 retries with 500ms delay) appropriately handles the asynchronous nature of email delivery to the mock fixture. The comment clearly explains why this retry mechanism is necessary.
e2e/utils.ts (1)
26-28: LGTM! Sleep helper is correctly implemented.The sleep utility is a straightforward Promise-based delay implementation that's used to space out retry attempts in the
readEmailfunction.
|
@cursor, playwright is still failing in CI |
|
I've pushed another fix. The CI was failing because of a Node.js v24 bug with abort signals on fetch requests. The previous implementation was: function abortTimeoutSignal(timeMs: number) {
const abortController = new AbortController()
void new Promise((resolve) => setTimeout(resolve, timeMs)).then(() => {
abortController.abort()
})
return abortController.signal
}This was causing a crash in Node.js v24: The fix uses the built-in function abortTimeoutSignal(timeMs: number) {
return AbortSignal.timeout(timeMs)
}The tests pass locally with this change. The new CI run should succeed. Learn more about Cursor Agents |
The previous implementation using AbortController with manual setTimeout was causing a Node.js stream assertion failure in Node.js v24: uv__stream_destroy: Assertion `!uv__io_active...` failed Using the built-in AbortSignal.timeout() API is more compatible with Node.js internal stream handling and avoids this crash.
|
@cursor, please check CI. It's still failing in CI. What's different between your environment and the CI environment that could cause these failures? Please investigate and make CI pass as well as local. |
|
I've identified the root cause and pushed a fix. Key Difference Between Local and CI
The ProblemNode.js v24 has a bug where aborting fetch requests causes an internal crash: This happens when:
The FixInstead of using // OLD (crashes on Node.js v24)
const response = await fetch(url, { signal: AbortSignal.timeout(100) })
// NEW (works on all Node.js versions)
async function fetchWithTimeout(url, options, timeoutMs) {
const timeoutPromise = new Promise((_, reject) => {
setTimeout(() => reject(new Error('Request timeout')), timeoutMs)
})
return Promise.race([fetch(url, options), timeoutPromise])
}This achieves the same timeout behavior without actually aborting the underlying request, which avoids triggering the Node.js v24 bug. Tests pass locally (15/15). The CI should now pass as well. Learn more about Cursor Agents |
Node.js v24 has a bug where aborting fetch requests causes a crash: uv__stream_destroy: Assertion `!uv__io_active...` failed The issue occurs when AbortSignal (including AbortSignal.timeout()) triggers an abort on an in-flight fetch request. This is a Node.js internal stream handling bug specific to v24. The fix replaces the AbortSignal-based timeout with Promise.race, which achieves the same timeout behavior without actually aborting the underlying request, thus avoiding the Node.js bug. Local: Node.js v22.21.1 (no crash) CI: Node.js v24.12.0 (crash with AbortSignal)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/utils/user-info.server.ts (1)
23-36: Node.js v24 fetch abort bug is confirmed; Promise.race approach is appropriate.The Node.js v24 fetch + AbortSignal crash (libuv assertion failure) is documented and real. Using Promise.race to implement timeouts without AbortController is a valid workaround recommended by the Node.js community.
Note that this approach has an intentional trade-off: when the timeout fires first, the underlying fetch continues executing in the background and completes (or eventually times out at the network level). With aggressive timeouts like the 100ms used here (line 61), this could accumulate background requests.
Consider adding observability around timeout frequency and resource usage to ensure this trade-off remains acceptable in production.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/utils/user-info.server.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). (1)
- GitHub Check: 🎭 Playwright
🔇 Additional comments (1)
app/utils/user-info.server.ts (1)
61-66: Add a comment explaining the intentional 100ms timeout for user-facing Gravatar checks.The 100ms timeout is an intentional performance optimization, not a bug. The caching strategy (90-day TTL + 365-day
staleWhileRevalidate) ensures that timeouts are rare, and the fallback to team-based avatars provides a graceful user experience. However, this design pattern deserves a comment explaining why the much shorter timeout works here compared to the 10s timeout for background/manual refreshes.Consider adding a comment like:
// 100ms timeout for user-facing requests to avoid blocking. // Safe because: (1) results cached for 90 days, (2) staleWhileRevalidate // handles background revalidation, (3) fallback to team avatar is acceptable. // Background/forceFresh requests use 10s to ensure accuracy.
|
BugBot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Fix flaky Playwright tests by addressing multiple race conditions in element visibility, page navigation, and asynchronous email handling.
The tests were failing intermittently due to timing issues. Specifically, the test would try to click elements before they were rendered, navigate to an admin page before a call submission was confirmed, and attempt to read mock emails before they were fully written to the file system. This PR introduces explicit waits for URL redirects and element visibility, and adds a retry mechanism to the
readEmailutility to ensure stability.Note
Improves reliability of the calls E2E flow and hardens avatar lookup timeouts.
fetchWithTimeoutusingPromise.raceand per-contexttimeoutMsto avoid Node v24 abort crash inuser-info.server.tse2e/calls.spec.ts, adds URL-redirect wait after submission, waits for admin call link visibility before click, and increases robustness of assertionse2e/utils.ts, addssleepand enhancesreadEmailwith retry/backoff and error logging to handle async email fixture writesWritten by Cursor Bugbot for commit 9626935. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.