Cleaned up e2e test isolation and file structure#27063
Conversation
These four files each contain only one test, so per-file isolation is identical to per-test isolation. The call was a no-op adding cognitive overhead without any behavioral benefit.
posts.test.ts (1 test) and post-settings.test.ts (1 test) tested the same feature surface as publishing.test.ts with the same setup pattern. Merging them eliminates 2 Ghost instance spinups in CI without losing any coverage.
Three separate tests for closing the preview modal (close button, ESC from header, ESC from iframe) are now one test that exercises all three dismiss methods in sequence. Same coverage, one setup instead of three.
WalkthroughThis pull request removes the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests/admin/posts/post-preview.test.ts (2)
5-5: Test suite naming should follow 'Ghost Admin - Feature' convention.The test suite is named
'Post Preview Modal'but guidelines specify the format'Ghost Admin - Feature'or'Ghost Public - Feature'.Suggested fix
-test.describe('Post Preview Modal', () => { +test.describe('Ghost Admin - Post Preview Modal', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/posts/post-preview.test.ts` at line 5, The suite name used in test.describe currently reads "Post Preview Modal" and must follow the "Ghost Admin - Feature" convention; update the test.describe invocation (the string passed to test.describe) to a name like "Ghost Admin - Post Preview Modal" (or equivalent "Ghost Admin - <Feature>") so it matches project naming guidelines and keeps the rest of the tests (inside the same test.describe block) unchanged.
12-51: Consolidating multiple scenarios into one test deviates from coding guidelines.The coding guidelines state "one test = one scenario (never mix multiple scenarios)". This test exercises three distinct dismiss methods (close button, ESC from header, ESC from iframe) in sequence.
I understand from the PR objectives that this consolidation is intentional to reduce CI overhead (fewer Ghost instances). If this trade-off is accepted, consider adding a brief comment at the top of the test explaining the consolidation rationale for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/posts/post-preview.test.ts` around lines 12 - 51, This test mixes three independent scenarios (close button, ESC from header, ESC from iframe) so either split them into three separate tests or, if consolidation is intentionally chosen to reduce CI overhead, add a brief comment at the top of this test explaining that rationale; locate the test function (the test named 'preview modal opens and can be closed via close button, ESC from header, and ESC from iframe') and either refactor its body into three tests that each exercise PostEditorPage.previewButton/previewModal.close(), previewModal.header + pressKey('Escape'), and previewModalDesktopFrame.clickPostLinkByTitle(...) + pressKey('Escape') respectively, or prepend a one-line explanatory comment above the existing test describing why multiple scenarios are consolidated for CI cost reasons.
🤖 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/tests/admin/posts/post-preview.test.ts`:
- Line 5: The suite name used in test.describe currently reads "Post Preview
Modal" and must follow the "Ghost Admin - Feature" convention; update the
test.describe invocation (the string passed to test.describe) to a name like
"Ghost Admin - Post Preview Modal" (or equivalent "Ghost Admin - <Feature>") so
it matches project naming guidelines and keeps the rest of the tests (inside the
same test.describe block) unchanged.
- Around line 12-51: This test mixes three independent scenarios (close button,
ESC from header, ESC from iframe) so either split them into three separate tests
or, if consolidation is intentionally chosen to reduce CI overhead, add a brief
comment at the top of this test explaining that rationale; locate the test
function (the test named 'preview modal opens and can be closed via close
button, ESC from header, and ESC from iframe') and either refactor its body into
three tests that each exercise
PostEditorPage.previewButton/previewModal.close(), previewModal.header +
pressKey('Escape'), and previewModalDesktopFrame.clickPostLinkByTitle(...) +
pressKey('Escape') respectively, or prepend a one-line explanatory comment above
the existing test describing why multiple scenarios are consolidated for CI cost
reasons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f0b2e23-6d52-44f6-9e02-c89d62adf11a
📒 Files selected for processing (8)
e2e/tests/admin/members/virtual-window.test.tse2e/tests/admin/posts/newsletter-send.test.tse2e/tests/admin/posts/post-preview.test.tse2e/tests/admin/posts/post-settings.test.tse2e/tests/admin/posts/posts.test.tse2e/tests/admin/posts/publishing.test.tse2e/tests/admin/settings/private-site.test.tse2e/tests/admin/settings/staff-invites.test.ts
💤 Files with no reviewable changes (6)
- e2e/tests/admin/settings/private-site.test.ts
- e2e/tests/admin/settings/staff-invites.test.ts
- e2e/tests/admin/posts/newsletter-send.test.ts
- e2e/tests/admin/posts/posts.test.ts
- e2e/tests/admin/members/virtual-window.test.ts
- e2e/tests/admin/posts/post-settings.test.ts
1 participant
Summary
Phase 1+2 of the e2e shard rebalancing plan.
ISO cleanup (Phase 1):
usePerTestIsolation()from 5 files where it was unnecessary:private-site,staff-invites,newsletter-send,virtual-window)filter-actions.test.ts— 2 tests that operate on independent label operations with no shared state conflictsFile consolidation (Phase 1):
posts.test.ts(1 test) andpost-settings.test.ts(1 test) intopublishing.test.ts— same feature surface, eliminates 2 Ghost instance spinupsforEach collapse (Phase 2):
force-upgrade.test.ts: Collapsed 2NAV_ITEMS.forEach()loops (14 tests, each with its own Ghost instance) into 2 flow tests. Eliminates ~11 Ghost instance spinups — likely the single most expensive file in the suite.navigation.test.ts: CollapsedNAV_ITEMS.forEach()(7 tests) into 1 flow test that iterates all items in sequence.No coverage removed. Every assertion from the original tests is preserved — just grouped into fewer tests with fewer Ghost instances.
Net impact:
test()callsTest plan