fix(e2e): add test isolation for note-modal Slate editor tests#132
fix(e2e): add test isolation for note-modal Slate editor tests#132ryota-murakami merged 2 commits intomainfrom
Conversation
The note-modal tests were flaky on CI because the "save and persist" test writes content to the DB, and subsequent tests (slash command, heading autoformat) inherit that state. Ctrl+A+Backspace to clear Slate editors is inherently unreliable on slow CI runners. Fix: add resetProjectInfoNotes() helper that resets note fields to seed values before each test, ensuring a known starting state.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new e2e DB helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
=======================================
Coverage 71.03% 71.03%
=======================================
Files 145 145
Lines 4381 4381
Branches 1148 1180 +32
=======================================
Hits 3112 3112
Misses 1248 1248
Partials 21 21 ☔ 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.
🧹 Nitpick comments (1)
e2e/helpers/db-query.ts (1)
316-321: Optional: extract seed note values to avoid duplication withresetRepoCards().The four note strings defined here (
'Important project notes here','Another repo notes','','CLI tool documentation') are also inlined insideresetRepoCards()at itsseedProjectInfosarray. If a seed value changes, both places need updating.♻️ Suggested extraction
+// Canonical seed note values — shared by resetProjectInfoNotes() and resetRepoCards() +export const SEED_NOTES: Record<keyof typeof PROJECT_INFO_IDS, string> = { + projinfo1: 'Important project notes here', + projinfo2: 'Another repo notes', + projinfo3: '', + projinfo4: 'CLI tool documentation', +} as const const seedNotes = [ - { id: PROJECT_INFO_IDS.projinfo1, note: 'Important project notes here' }, - { id: PROJECT_INFO_IDS.projinfo2, note: 'Another repo notes' }, - { id: PROJECT_INFO_IDS.projinfo3, note: '' }, - { id: PROJECT_INFO_IDS.projinfo4, note: 'CLI tool documentation' }, + { id: PROJECT_INFO_IDS.projinfo1, note: SEED_NOTES.projinfo1 }, + { id: PROJECT_INFO_IDS.projinfo2, note: SEED_NOTES.projinfo2 }, + { id: PROJECT_INFO_IDS.projinfo3, note: SEED_NOTES.projinfo3 }, + { id: PROJECT_INFO_IDS.projinfo4, note: SEED_NOTES.projinfo4 }, ]Then reference
SEED_NOTESinresetRepoCards()as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/db-query.ts` around lines 316 - 321, The seed note strings are duplicated between the local constant seedNotes and the array inside resetRepoCards() (seedProjectInfos); extract the four note strings into a single exported constant (e.g., SEED_NOTES) and replace both occurrences so resetRepoCards() uses SEED_NOTES and the current seedNotes definition references the same constant; update any imports/usages accordingly and keep identifiers like seedNotes, resetRepoCards(), and seedProjectInfos in mind when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/helpers/db-query.ts`:
- Around line 316-321: The seed note strings are duplicated between the local
constant seedNotes and the array inside resetRepoCards() (seedProjectInfos);
extract the four note strings into a single exported constant (e.g., SEED_NOTES)
and replace both occurrences so resetRepoCards() uses SEED_NOTES and the current
seedNotes definition references the same constant; update any imports/usages
accordingly and keep identifiers like seedNotes, resetRepoCards(), and
seedProjectInfos in mind when making the change.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
When the Plate/Slate editor is empty, it renders a placeholder element with data-slate-placeholder="true". textContent() captures this placeholder text, causing the emptiness check to fail. Use placeholder element visibility instead — a visible placeholder means the editor is truly empty.
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/logged-in/note-modal.spec.ts (2)
294-298: Bold test still useswaitForTimeout(300)— consider the polling pattern applied to heading/slash tests.The heading autoformat (lines 542-551) and slash command (lines 244-253) tests use
toPass()polling to confirm the placeholder is visible after clear. For consistency and CI resilience, the same guard could be applied here (and in the italic test at lines 588-591).♻️ Proposed refactor
- // Clear existing content and wait for Slate to process - await page.keyboard.press(`${MOD}+a`) - await page.keyboard.press('Backspace') - // eslint-disable-next-line playwright/no-wait-for-timeout - await page.waitForTimeout(300) + // Clear existing content — retry inside polling loop for CI resilience + await expect(async () => { + await page.keyboard.press(`${MOD}+a`) + await page.keyboard.press('Backspace') + const placeholder = editorContent.locator('[data-slate-placeholder="true"]') + await expect(placeholder).toBeVisible() + }).toPass({ timeout: 10000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logged-in/note-modal.spec.ts` around lines 294 - 298, Replace the brittle fixed sleep after clearing Slate in the bold test (the block using page.keyboard.press(`${MOD}+a`) and page.keyboard.press('Backspace')) with the same toPass() polling pattern used by the heading autoformat and slash command tests: remove the waitForTimeout(300) and instead call the toPass() helper to poll until the editor placeholder (the same locator/assertion used in lines ~542-551 and ~244-253) becomes visible; apply the identical change to the italic test (lines ~588-591) so both tests wait deterministically for Slate processing rather than sleeping.
516-518: Navigation duplicated in test bodies — consider centralising inbeforeEach.Unlike the other two describe blocks, this
beforeEachskipspage.goto()/waitForLoadState('networkidle'), so both tests repeat that boilerplate individually (lines 523-525 and 570-572). As per coding guidelines,networkidleshould be the standard wait strategy for page loads — centralising it inbeforeEachis consistent with that and the rest of the file.♻️ Proposed refactor
- test.beforeEach(async () => { - await resetProjectInfoNotes() - }) + test.beforeEach(async ({ page }) => { + await resetProjectInfoNotes() + await page.goto(BOARD_URL) + await page.waitForLoadState('networkidle') + })Then remove the duplicated
goto/waitForLoadStatelines from each test body (lines 523-525 and 570-572).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logged-in/note-modal.spec.ts` around lines 516 - 518, The tests in this describe block call resetProjectInfoNotes in test.beforeEach but do not navigate to the app; two tests duplicate page.goto(...) and page.waitForLoadState('networkidle') in their bodies—move the navigation into the describe block's test.beforeEach so it runs after resetProjectInfoNotes (i.e., call page.goto(...) and await page.waitForLoadState('networkidle') in the same beforeEach), then remove the duplicated page.goto/page.waitForLoadState lines from the two tests to centralise navigation and use the standard networkidle wait strategy; look for test.beforeEach, resetProjectInfoNotes, and the page.goto/page.waitForLoadState('networkidle') calls to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/logged-in/note-modal.spec.ts`:
- Around line 294-298: Replace the brittle fixed sleep after clearing Slate in
the bold test (the block using page.keyboard.press(`${MOD}+a`) and
page.keyboard.press('Backspace')) with the same toPass() polling pattern used by
the heading autoformat and slash command tests: remove the waitForTimeout(300)
and instead call the toPass() helper to poll until the editor placeholder (the
same locator/assertion used in lines ~542-551 and ~244-253) becomes visible;
apply the identical change to the italic test (lines ~588-591) so both tests
wait deterministically for Slate processing rather than sleeping.
- Around line 516-518: The tests in this describe block call
resetProjectInfoNotes in test.beforeEach but do not navigate to the app; two
tests duplicate page.goto(...) and page.waitForLoadState('networkidle') in their
bodies—move the navigation into the describe block's test.beforeEach so it runs
after resetProjectInfoNotes (i.e., call page.goto(...) and await
page.waitForLoadState('networkidle') in the same beforeEach), then remove the
duplicated page.goto/page.waitForLoadState lines from the two tests to
centralise navigation and use the standard networkidle wait strategy; look for
test.beforeEach, resetProjectInfoNotes, and the
page.goto/page.waitForLoadState('networkidle') calls to update.
2 participants
Summary
resetProjectInfoNotes()DB helper to reset note content to seed valuesbeforeEachfor all 3 note-modal test describe blocksCtrl+A+Backspaceclear unreliable on slow CI runnersRoot Cause
The Slate/Plate.js editor
Ctrl+Aselection doesn't reliably select all content incontenteditableelements on GitHub Actions runners, especially when the content includes rich text with multiple block nodes. Rather than fighting browser timing, reset the DB to a known state before each test.Test plan
pnpm typecheckpassespnpm lintpassespnpm testpasses (1282 tests)Summary by CodeRabbit