fix(e2e): migrate to real DB parallel testing with all 12 shards green#51
Conversation
Migrate E2E tests from MSW mocks to real Supabase databases running in 12 parallel shards. Each shard gets its own PostgREST instance and Next.js server, enabling true database isolation and mutation testing. Key changes: - Add e2e-parallel.sh and e2e-setup-databases.sh orchestration scripts - Add db-query.ts helpers with reset functions for test state management - Make DnD tests resilient to RSC cache (read actual DOM state, not absolute positions) - Fix note-modal slash command with Portal-aware locator and editor priming - Fix project-info-links with .last() for URL inputs and combobox triggers - Update playwright.config.ts for parallel shard support - Split MSW handlers (GitHub-only for E2E, +Supabase for Storybook) - Add auth proxy in server.ts for APP_ENV=test mock user injection
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughSwitches tests from hard-coded IDs to DB-backed constants and adds DB-reset helpers; introduces parallel/sharded E2E orchestration (scripts, Playwright flags, per-shard DB/proxies); splits MSW handler sets and updates Storybook preview; converts MSW reset route to no-op and tweaks E2E mock user data. Changes
Sequence Diagram(s)sequenceDiagram
actor Orchestrator as Orchestrator (scripts/e2e-parallel.sh)
participant DBMgr as Shard DBs (Postgres)
participant PostgREST as PostgREST containers
participant Proxy as nginx proxies
participant Next as Next.js servers
participant Playwright as Playwright shards
participant Reporter as Report Merger
Orchestrator->>DBMgr: create N shard DBs & load seeds
Orchestrator->>PostgREST: start PostgREST per shard
Orchestrator->>Proxy: generate & start nginx proxy per shard
Orchestrator->>Next: start Next.js server per shard (shardPort)
Orchestrator->>Playwright: launch shards with E2E_PARALLEL_MODE
Playwright->>Next: run tests against shard-specific baseURLs
Playwright->>Reporter: emit per-shard blob reports
Orchestrator->>Reporter: merge blobs -> final report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
🤖 Morph Preview TestPreview URL: https://gitbox-ev5pctyx8-laststance.vercel.app AI SummaryIssues:
Verified:
Notes:
RecordingAutomated testing by Morph |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
=======================================
Coverage 74.66% 74.66%
=======================================
Files 118 118
Lines 3765 3765
Branches 981 981
=======================================
Hits 2811 2811
Misses 938 938
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
e2e/logged-in/add-repository-combobox.spec.ts (1)
303-319:⚠️ Potential issue | 🟡 MinorConsole error listener is registered too late to catch the errors it intends to detect.
The listener is set up at line 304, after the combobox has already been opened (step 5, line 290) and verified (step 6, line 295). Any
TypeErrorthrown during combobox rendering will have already fired and been missed. Move the listener registration before step 5 (or ideally before the page reload at step 4) to actually capture async errors from the combobox lifecycle.Proposed fix
+ // Step 3.5: Set up console error listener BEFORE triggering the combobox + const consoleErrors: string[] = [] + page.on('console', (msg) => { + if (msg.type() === 'error') { + consoleErrors.push(msg.text()) + } + }) + // Step 4: Reload page to apply localStorage state await page.reload() await page.waitForLoadState('networkidle') // Step 5: Open AddRepositoryCombobox ... - // Step 8: Check console for no TypeErrors - const consoleErrors: string[] = [] - page.on('console', (msg) => { - if (msg.type() === 'error') { - consoleErrors.push(msg.text()) - } - }) - // Wait a moment for any async errors to surfacee2e/logged-in/maintenance-project-info.spec.ts (1)
178-181:⚠️ Potential issue | 🟠 MajorMissing
resetProjectInfoComments()in beforeEach — tests silently skip assertions after DB mutation.The "save comment on Enter key" test (line 271) persists a comment to the real DB. Since there's no DB reset, subsequent tests like "cancel edit on Escape key" (line 305) find
emptyCount === 0and skip their entire assertion block via theifguard. The tests pass but verify nothing.Other comment-related specs (e.g.,
comment-inline-edit.spec.ts,comment-display.spec.ts) callresetProjectInfoComments()inbeforeEachfor exactly this reason.Proposed fix
+import { resetProjectInfoComments } from '../helpers/db-query' + test.describe('Maintenance Comment Inline Edit (Authenticated)', () => { test.use({ storageState: 'e2e/.auth/user.json' }) const MAINTENANCE_URL = '/maintenance' test.beforeEach(async ({ page }) => { + await resetProjectInfoComments() await page.goto(MAINTENANCE_URL) await page.waitForLoadState('networkidle') })e2e/logged-in/kanban-dnd/card-dnd.spec.ts (1)
94-97:⚠️ Potential issue | 🟡 Minor
expect(cards.length).toBeGreaterThanOrEqual(0)is a tautology — always passes.The comment says "should have at least some cards" but the assertion allows zero. Either bump to
1or remove the assertion.- expect(cards.length).toBeGreaterThanOrEqual(0) + expect(cards.length).toBeGreaterThanOrEqual(1)e2e/logged-in/kanban-dnd/column-dnd.spec.ts (1)
560-599:⚠️ Potential issue | 🟡 MinorRetry after failed drop doesn't reset DB state.
The retry loop calls
page.reload()on failure, but the first drag attempt may have already mutated the DB (partial server action). TheresetStatusListPositions()only runs inbeforeEach, not between retry attempts. The second attempt could start from an inconsistent DB state.Proposed fix
} else if (attempt < MAX_ATTEMPTS) { - // Reset by refreshing page for next attempt + // Reset DB state and refresh page for next attempt + await resetStatusListPositions() await page.reload() await page.waitForLoadState('networkidle') await page.waitForTimeout(800)
🤖 Fix all issues with AI agents
In `@scripts/e2e-parallel.sh`:
- Around line 152-175: Under set -u the expansions of empty arrays will error on
older bash; change the Playwright invocation and the place EXTRA_PW_ARGS is
added so you use the safe expansion idiom for both arrays (replace plain
"${EXTRA_PW_ARGS[@]}" and "${PW_COMMON_ARGS[@]}" with the arr[@]+"${arr[@]}"
form) so the command and any other array expansions (EXTRA_PW_ARGS and
PW_COMMON_ARGS) do not trigger unbound variable errors on macOS/bash 3.2 while
preserving the same arguments when non-empty.
🧹 Nitpick comments (14)
scripts/e2e-setup-databases.sh (2)
158-170:elapsedcounts iterations, not seconds — fragile timeout math.Each iteration sleeps 0.5s, so
elapsed >= 60means ~30s. The error message correctly says "30s", but if someone tweaks the sleep interval this silently breaks. Consider making the relationship explicit.♻️ Suggested improvement
- elapsed=0 + attempts=0 + max_attempts=60 # 60 × 0.5s = 30s timeout while ! curl -sf "http://127.0.0.1:${PORT}/rest/v1/" > /dev/null 2>&1; do sleep 0.5 - elapsed=$((elapsed + 1)) - if [ "$elapsed" -ge 60 ]; then - echo "❌ Shard ${i} proxy on port ${PORT} failed to start within 30s" + attempts=$((attempts + 1)) + if [ "$attempts" -ge "$max_attempts" ]; then + echo "❌ Shard ${i} proxy on port ${PORT} failed to start within ${max_attempts} attempts (~30s)"
146-150: Pin thenginx:alpineimage to a specific version.
postgrest/postgrest:v12.2.3is pinned (line 33) butnginx:alpinefloats. A breaking change in a future nginx release could cause mysterious CI failures. Pin it for reproducibility and consistency.📌 Proposed fix
- nginx:alpine > /dev/null + nginx:1.28.2-alpine > /dev/nullscripts/e2e-parallel.sh (1)
27-29: Local Supabase demo JWTs — Gitleaks false positive, but consider documenting.These are the well-known default tokens from
supabase init(issuer:supabase-demo, exp: 2032). Not real secrets. However, adding a brief comment like# Default local Supabase demo tokens (public, not secrets)would silence future Gitleaks runs and reduce reviewer friction.playwright.config.ts (1)
81-204: Duplicated monocart-reporter config between CI and local branches.The coverage configuration objects on lines 93–147 and 151–203 are identical. Extract to a shared constant to avoid drift.
♻️ Proposed refactor
Add before
defineConfig:const monocartCoverageReporter = [ 'monocart-reporter', { name: 'GitBox E2E Coverage Report', outputFile: './coverage-e2e/index.html', coverage: { reports: [ ['v8'], ['html', { subdir: 'istanbul' }], ['lcovonly', { file: 'lcov.info' }], ['text-summary'], ['json-summary', { file: 'coverage-summary.json' }], ], entryFilter: { '**/node_modules/**': false, '**/_next/static/chunks/webpack*': false, '**/_next/static/chunks/polyfills*': false, '**/_next/static/**': true, }, sourceFilter: { '**/lib/actions/**': false, '**/lib/supabase/**': false, '**/app/auth/callback/**': false, '**/instrumentation*.ts': false, '**/sentry.*.config.ts': false, '**/tests/**': false, '**/mocks/**': false, '**/*.spec.ts': false, '**/*.test.ts': false, '**/*.test.tsx': false, '**/*.stories.tsx': false, '**/.storybook/**': false, '**/*.config.ts': false, '**/*.config.js': false, '**/*.config.mjs': false, '**/node_modules/**': false, '**/.next/**': false, '**/app/**': true, '**/components/**': true, '**/lib/**': true, '**/packages/**': true, }, sourcePath: (filePath: string) => filePath.replace(/^webpack:\/\/gitbox\//, '').replace(/^\.\//g, ''), }, }, ] as constThen use it in both branches:
- ? [['blob'], [/* full monocart config */]] - : [['list'], [/* same monocart config */]] + ? [['blob'], monocartCoverageReporter] + : [['list'], monocartCoverageReporter]e2e/logged-in/project-info-links.spec.ts (2)
17-17:querySingleandPROJECT_INFO_IDSare only used in the skipped test.These imports are only referenced inside the
test.skipblock (lines 425–471). If that test stays skipped long-term, the dead imports add noise. Not blocking — just worth cleaning up if the skipped test isn't re-enabled soon.
564-576: Timeout bump to 10s on line 576 is disproportionate for a URL input appearing.Other similar
.last()waits use 5000ms (e.g., line 596). The 10000ms here seems inconsistent — was there a specific flaky scenario that warranted this?e2e/logged-in/board-settings.spec.ts (1)
19-24: Clean migration to dynamic board ID.Same note as
project-info-links.spec.ts:querySingleis only used inside thetest.skipblock (line 335). Consider removing the import until the test is re-enabled.e2e/logged-in/add-repository-pagination.spec.ts (1)
57-59: Pre-existing: this assertion is always true.
expect(optionCount).toBeGreaterThanOrEqual(0)can never fail sincecount()returns a non-negative integer. Not introduced by this PR, but worth noting — if the intent is to verify repos load, considertoBeGreaterThan(0)or removing the assertion.e2e/logged-in/add-repository-combobox.spec.ts (1)
845-850: Stale comment referencing MSW.Line 847 says "Note: With MSW mocking…" but this PR migrates E2E tests to real DB. Consider updating the comment to reflect the new setup.
e2e/logged-in/remove-from-board.spec.ts (1)
125-181: Destructive test without DB reset — potential test isolation concern.This test removes a card from the board (line 162) but there's no
beforeEachreset to restore cards. In the real DB parallel setup, if another test in this file (or shard) depends on the full card count, it could flake depending on execution order.Since the PR reports all 12 shards green, this may be fine today, but adding a card-reset helper in
beforeEachwould make this more resilient to future test additions.e2e/logged-in/maintenance-project-info.spec.ts (1)
210-216: Fragile card scoping via Tailwind.groupclass.Filtering by
.groupties the locator to a Tailwind utility class that could change during styling refactors. Adata-testidon the maintenance card (e.g.,data-testid="maintenance-card-{id}") would be more resilient — especially since[data-testid^="maintenance-card-"]is already used in the first test block (line 33).e2e/logged-in/comment-inline-edit.spec.ts (1)
98-128: Non-null assertion ontextContent()is acceptable here but worth noting.Lines 106 and 126:
originalTextcould technically benullfromtextContent(), but the preceding visibility assertion makes this safe in practice. Same pattern at lines 213–234.e2e/logged-in/kanban-dnd/card-dnd.spec.ts (1)
41-50:gotoFreshBoardis duplicated with a different implementation incolumn-dnd.spec.ts.This file uses query-param busting +
window.location.reload(), whilecolumn-dnd.spec.tsnavigates to/boardsfirst then back. Two divergent cache-busting strategies for the same purpose is a maintenance risk — if one proves more reliable, the other won't benefit.Extract to a shared helper in
e2e/helpers/(e.g., alongsidedb-query.ts).e2e/logged-in/kanban-dnd/column-dnd.spec.ts (1)
917-918: Stale variable namestatus3Movedno longer corresponds to status-3.After migrating to
STATUS_IDS.focusDevelopment, the variable namestatus3Movedis misleading. Consider renaming tofocusDevMovedfor clarity.
Use ${arr[@]+"${arr[@]}"} idiom to prevent unbound variable errors
on bash < 4.4 (macOS ships 3.2) when arrays are empty under set -u.
🤖 Morph Preview TestPreview URL: https://gitbox-6jrlcw07g-laststance.vercel.app AI SummaryIssues:
Verified:
Notes:
RecordingAutomated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/e2e-parallel.sh`:
- Around line 57-71: The cleanup() function iterates over SERVER_PIDS and
SHARD_PIDS using "${SERVER_PIDS[@]}" and "${SHARD_PIDS[@]}" which can cause
unbound-variable errors on older bash when arrays are empty; update those
expansions to the safe-empty-array idiom used elsewhere (e.g., use
"${SERVER_PIDS[@]:-}" and "${SHARD_PIDS[@]:-}") so the for-loops in cleanup()
handle empty arrays safely under set -u.
🧹 Nitpick comments (2)
scripts/e2e-parallel.sh (2)
26-29: Gitleaks flags these JWTs — they're safe but worth annotating.These are the well-known default local Supabase dev credentials (issuer:
supabase-demo), not real secrets. A short inline comment like# Default local supabase dev keys — not secretswould prevent future contributors (and secret-scanning tools) from raising alarms.
34-44:--shards/--grepwithout a following argument will blow up cryptically.Under
set -u, if--shardsis the last arg,$2triggers an unbound variable error with no helpful message. A guard is cheap:Proposed guard
--shards) + [[ $# -ge 2 ]] || { echo "Error: --shards requires a value"; exit 1; } SHARD_COUNT="$2" shift 2 ;; --grep) + [[ $# -ge 2 ]] || { echo "Error: --grep requires a value"; exit 1; } GREP_PATTERN="$2" shift 2 ;;
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
Apply same ${arr[@]+"${arr[@]}"} idiom to SERVER_PIDS and SHARD_PIDS
in cleanup() to prevent unbound variable errors on macOS bash 3.2.
🤖 Morph Preview TestPreview URL: https://gitbox-kxb0izej1-laststance.vercel.app AI SummaryIssues:
Verified:
Notes:
RecordingAutomated testing by Morph |
- Wait for repo cards to render in gotoFreshBoard (CI can be slow) - Dynamically find column with 2+ cards instead of hardcoding Planning - Use generic card references instead of specific card IDs for reorder
🤖 Morph Preview TestPreview URL: https://gitbox-nmnqon822-laststance.vercel.app AI SummaryIssues:
Verified:
Notes:
RecordingAutomated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/logged-in/kanban-dnd/card-dnd.spec.ts (1)
98-101:⚠️ Potential issue | 🟡 MinorAssertion
>= 0is always true — this test verifies nothing.
cards.lengthis a non-negative integer, sotoBeGreaterThanOrEqual(0)can never fail. This should be>= 1(or a more meaningful count like the expected 5 seed cards) to actually validate that cards render.Proposed fix
- expect(cards.length).toBeGreaterThanOrEqual(0) + expect(cards.length).toBeGreaterThanOrEqual(1)
🤖 Fix all issues with AI agents
In `@e2e/logged-in/kanban-dnd/card-dnd.spec.ts`:
- Around line 192-207: The test currently only verifies presence and length of
newOrder but not that the order changed; update the assertion after computing
newOrder (from getCardOrderInColumn and variables dragCard, initialOrder,
columnIds) to assert the order actually mutated — e.g., compare arrays to ensure
newOrder is not equal to initialOrder or assert the index of dragCard changed
(calculate initialIndex = initialOrder.indexOf(dragCard) and newIndex =
newOrder.indexOf(dragCard) and expect(newIndex).not.toBe(initialIndex)); use
those checks in the same block that verifies newOrder to fail when the drag was
a no-op.
In `@scripts/e2e-parallel.sh`:
- Around line 156-158: Remove the conditional length check that uses
`${`#EXTRA_PW_ARGS`[@]}` and instead always append EXTRA_PW_ARGS into
PW_COMMON_ARGS using the safe expansion idiom; delete the if/fi block and
replace it with a single append that uses the `${arr[@]+"${arr[@]}"}` pattern
for EXTRA_PW_ARGS so PW_COMMON_ARGS is updated safely on bash 3.2 with set -u
(referencing EXTRA_PW_ARGS and PW_COMMON_ARGS to locate the change).
- Around line 235-243: The summary block uses ${`#PASSED_SHARDS`[@]} and
${`#FAILED_SHARDS`[@]} which triggers unbound-variable errors under macOS bash 3.2
with set -u when those arrays are empty; change the code to track integer
counters (e.g., PASSED_COUNT, FAILED_COUNT) that are initialized to 0 and
incremented whenever you append to PASSED_SHARDS or FAILED_SHARDS, then replace
uses of ${`#PASSED_SHARDS`[@]} and ${`#FAILED_SHARDS`[@]} in the echo and the exit
check with those counters (and ensure the counters are defined at top-level so
they never trigger set -u), or alternatively use the safe test pattern
${FAILED_SHARDS[@]+x} when testing emptiness if you prefer not to add counters.
🧹 Nitpick comments (3)
e2e/logged-in/kanban-dnd/card-dnd.spec.ts (2)
316-323: Hard-coded column title strings could drift from DB seed.
'Production Release'and'MVP Release'are string literals that must match the seed data exactly. If the seed titles change, this test breaks silently (wrong assertion target). Consider deriving these from a shared constant or querying them.
349-351: Skipped test with TODO — tracking this?The
test.skipwith a TODO about real Supabase auth is reasonable given current constraints, but ensure there's an issue tracking re-enablement so it doesn't rot.Would you like me to open a GitHub issue to track re-enabling this test once real Supabase auth is implemented?
scripts/e2e-parallel.sh (1)
32-51: No validation on--shardsvalue.If someone passes
--shards fooor--shards 0, the script will proceed with nonsensical behavior (infinite loops fromseq, arithmetic errors, etc.). A quick guard after parsing would help.Proposed validation
done + +# Validate shard count +if ! [[ "$SHARD_COUNT" =~ ^[1-9][0-9]*$ ]]; then + echo "❌ Invalid --shards value: ${SHARD_COUNT} (must be a positive integer)" + exit 1 +fi
…fety
- Add actual reorder assertion in card-dnd intra-column test
- Replace ${#arr[@]} length checks with counters for bash 3.2 compat
- Use safe for-loop idiom for EXTRA_PW_ARGS expansion
🤖 Morph Preview TestPreview URL: https://gitbox-eritgdhmx-laststance.vercel.app AI SummaryIssues:
Verified:
Notes:
RecordingAutomated testing by Morph |
2 participants
Summary
scripts/e2e-parallel.sh,scripts/e2e-setup-databases.sh) for shard DB lifecycle management.bg-popoverlocator and editor priming technique.last()for URL inputs and combobox triggers to handle prior test state persistenceKey Architecture
pg_dump/pg_restorenext startinstances (ports 3008-3019), each with ownNEXT_PUBLIC_SUPABASE_URLnext build, runtime env override per shardAPP_ENV=testinserver.tsbeforeEachDB resets via PostgREST REST API (db-query.ts)Test Results
Test plan
pnpm typecheck— passpnpm lint— passpnpm test— 1202 tests passpnpm build— passpnpm e2e:parallel --skip-build— 12/12 shards greenSummary by CodeRabbit
Tests
Chores