Ported publishing browser tests to e2e framework#26996
Open
Ported publishing browser tests to e2e framework#26996
Conversation
- Moved 18/20 tests from ghost/core/test/e2e-browser/admin/publishing.spec.js to new Playwright e2e tests - Created e2e/tests/admin/posts/publishing.test.ts (13 tests: publish, schedule, update, delete) - Created e2e/tests/admin/posts/post-access.test.ts (6 tests: visibility, tiers, timezone, recipients) - Extended page objects: PostEditorPage (PublishFlow, UpdateFlow, SettingsMenu), PostPage, MemberDetailsPage, SettingsPage, PostsPage - Added PageEditorPage for page-specific editor tests - Trimmed old publishing.spec.js to keep only 2 Lexical rendering tests
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded methods 🚥 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 |
- Extracted helper functions in publishing.test.ts to reduce duplication - Replaced regex with simple string match in post-access.test.ts
- Extracted scheduleAsap, verifyPostNotAccessible, waitForScheduledPost helpers - Schedule tests now share common setup/verification patterns
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
ghost/core/test/e2e-browser/admin/publishing.spec.js (1)
14-16:⚠️ Potential issue | 🟡 MinorIncorrect
awaitplacement in assertion.The
awaitis applied to the locator instead of theexpectassertion. This meanstoBeVisible()receives a locator synchronously but the assertion itself isn't awaited, which could cause the test to pass incorrectly or produce flaky behavior.🐛 Proposed fix
- // Check if the lexical editor is present - expect(await adminPage.locator('[data-kg="editor"]').first()).toBeVisible(); + // Check if the lexical editor is present + await expect(adminPage.locator('[data-kg="editor"]').first()).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-browser/admin/publishing.spec.js` around lines 14 - 16, The assertion incorrectly awaits the locator instead of the expect call; change the line using adminPage.locator('[data-kg="editor"]').first() so that the expect call is awaited: use await expect(adminPage.locator('[data-kg="editor"]').first()).toBeVisible(); ensuring the asynchronous assertion (expect(...).toBeVisible()) is awaited rather than the locator.
🧹 Nitpick comments (4)
e2e/helpers/pages/admin/members/member-details-page.ts (1)
131-136: Potential race condition when reading clipboard.The clipboard read happens immediately after
copyLinkButton.click(). If the clipboard write is asynchronous,navigator.clipboard.readText()might return stale data. Consider adding a small wait or verifying the clipboard content is non-empty.♻️ Suggested improvement
async impersonate(): Promise<string> { await this.settingsSection.memberActionsButton.click(); await this.settingsSection.impersonateButton.click(); await this.copyLinkButton.click(); + // Wait briefly for clipboard write to complete + await this.page.waitForTimeout(100); return await this.page.evaluate(() => navigator.clipboard.readText()); }Alternatively, if there's a UI confirmation after copying (like a toast), wait for that instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/member-details-page.ts` around lines 131 - 136, In impersonate(), avoid reading the clipboard immediately after copyLinkButton.click() — add a short wait or verification to prevent stale reads: after this.settingsSection.impersonateButton.click() and await this.copyLinkButton.click(), poll or await a condition (e.g., use this.page.waitForFunction(() => navigator.clipboard.readText().then(t => t && t.length > 0), {timeout: ...}) ) or wait for a UI confirmation/toast selector that appears on successful copy, then call navigator.clipboard.readText(); reference the impersonate method, copyLinkButton, and this.page for locating where to add the wait.e2e/helpers/pages/admin/settings/settings-page.ts (1)
57-59: Consider usinggetByTestIdfor consistency.The method uses
page.locator('[data-testid="select-option"]')while the rest of the file usespage.getByTestId(...). UsinggetByTestIdwould be more consistent with the existing pattern.♻️ Suggested change
getSelectOption(text: string | RegExp): Locator { - return this.page.locator('[data-testid="select-option"]').filter({hasText: text}); + return this.page.getByTestId('select-option').filter({hasText: text}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/settings/settings-page.ts` around lines 57 - 59, The getSelectOption method uses this.page.locator('[data-testid="select-option"]') which is inconsistent with the rest of the file; update the implementation of getSelectOption to use this.page.getByTestId('select-option') and preserve the existing .filter({ hasText: text }) call so it returns the same Locator while matching the project's getByTestId pattern.e2e/helpers/pages/admin/posts/post/post-editor-page.ts (2)
235-237: Wait for the settings menu before returning.This helper resolves immediately after the toggle click, so callers can race the menu render/animation. Waiting on a stable settings locator here will make the downstream interactions less timing-sensitive.
⏱️ Suggested guard
async openSettingsMenu(): Promise<void> { await this.settingsToggleButton.click(); + await this.settingsMenu.postUrlInput.waitFor({state: 'visible'}); }As per coding guidelines, "Use
waitFor()for guards in page objects, neverexpect()in page objects".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/posts/post/post-editor-page.ts` around lines 235 - 237, The openSettingsMenu() helper currently clicks settingsToggleButton and returns immediately; modify it to click settingsToggleButton and then wait for the settings menu locator to be stable using waitFor() (e.g., waitFor a visible/attached state on the settings menu locator) before returning so callers cannot race rendering/animation; reference the openSettingsMenu function, settingsToggleButton, and the settings menu locator, and use waitFor() (not expect()) as the guard.
123-125: Prefer a semantic helper here.
clickPublishAndSendLabel()duplicatesselectPublishType('publish+send')and exposes a label-specific DOM detail through the page-object API. A semantic wrapper keeps callers insulated from markup changes.♻️ Suggested simplification
- async clickPublishAndSendLabel(): Promise<void> { - await this.page.locator('label[for="publish-type-publish+send"]').click(); - } + async selectPublishAndSend(): Promise<void> { + await this.selectPublishType('publish+send'); + }As per coding guidelines, "Page object methods should use semantic names (e.g., 'login()' not 'clickLoginButton()') and keep all assertions in test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/posts/post/post-editor-page.ts` around lines 123 - 125, The method clickPublishAndSendLabel() exposes a DOM detail and duplicates existing behavior; replace it with a semantic wrapper that calls the existing selectPublishType('publish+send') (e.g., add selectPublishAndSend() which delegates to selectPublishType('publish+send')) and update callers to use the new semantic method; remove or deprecate clickPublishAndSendLabel() so tests no longer rely on the label-specific locator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/admin/posts/publishing.test.ts`:
- Around line 328-339: The test can race with autosave and may not create an
unsaved-change state; after calling editor.createDraft({title:..., body:...})
make a second edit (e.g., call a method like editor.appendBody or
editor.updateBody / editor.typeInBody to change the content) or explicitly
assert an unsaved indicator (e.g., check editor.hasUnsavedChanges or
editor.saveIndicator shows unsaved) before calling editor.openSettingsMenu and
settingsMenu.deletePost, so the test reliably exercises the unsaved-delete
branch and then assert editor.screenTitle contains 'Posts'.
- Around line 30-53: The test titled 'publish and email - post is published and
sent' only verifies public availability; update it to assert the "send" outcome
(or rename it to only claim publishing). Specifically, after calling
publishFlow.selectPublishType('publish+send') and publishFlow.confirm() in the
test, add an assertion that the email was actually sent by querying the test
mail backend: e.g., call the app's test email API or mock SMTP outbox and assert
a message was queued/sent to 'test+recipient1@example.com' with the expected
subject/body (or assert a mail-sent flag in the response), using the same test
helpers around PostEditorPage.publishFlow and PostPage.goto to locate the right
post; alternatively, if you don't add an email assertion, rename the test to
'publish - post is published' to reflect current behavior and make the same
change for the other similar test block noted in the review.
- Around line 105-120: The test uses a hard-coded wait (page.waitForTimeout)
after calling pageEditor.publishFlow.schedule/confirm/close and before asserting
post availability; replace that sleep with Playwright's deterministic polling
via expect.poll that repeatedly calls PostPage.goto('/scheduled-page-test/' or
the relevant path) and checks response?.status() to become 200, and then assert
pageEditor.postStatus contains 'Scheduled' as needed; update all similar blocks
referencing pageEditor.publishFlow.schedule, pageEditor.publishFlow.confirm,
pageEditor.publishFlow.close, pageEditor.postStatus and PostPage.goto (also at
the other indicated ranges) to use expect.poll instead of waitForTimeout.
---
Outside diff comments:
In `@ghost/core/test/e2e-browser/admin/publishing.spec.js`:
- Around line 14-16: The assertion incorrectly awaits the locator instead of the
expect call; change the line using
adminPage.locator('[data-kg="editor"]').first() so that the expect call is
awaited: use await
expect(adminPage.locator('[data-kg="editor"]').first()).toBeVisible(); ensuring
the asynchronous assertion (expect(...).toBeVisible()) is awaited rather than
the locator.
---
Nitpick comments:
In `@e2e/helpers/pages/admin/members/member-details-page.ts`:
- Around line 131-136: In impersonate(), avoid reading the clipboard immediately
after copyLinkButton.click() — add a short wait or verification to prevent stale
reads: after this.settingsSection.impersonateButton.click() and await
this.copyLinkButton.click(), poll or await a condition (e.g., use
this.page.waitForFunction(() => navigator.clipboard.readText().then(t => t &&
t.length > 0), {timeout: ...}) ) or wait for a UI confirmation/toast selector
that appears on successful copy, then call navigator.clipboard.readText();
reference the impersonate method, copyLinkButton, and this.page for locating
where to add the wait.
In `@e2e/helpers/pages/admin/posts/post/post-editor-page.ts`:
- Around line 235-237: The openSettingsMenu() helper currently clicks
settingsToggleButton and returns immediately; modify it to click
settingsToggleButton and then wait for the settings menu locator to be stable
using waitFor() (e.g., waitFor a visible/attached state on the settings menu
locator) before returning so callers cannot race rendering/animation; reference
the openSettingsMenu function, settingsToggleButton, and the settings menu
locator, and use waitFor() (not expect()) as the guard.
- Around line 123-125: The method clickPublishAndSendLabel() exposes a DOM
detail and duplicates existing behavior; replace it with a semantic wrapper that
calls the existing selectPublishType('publish+send') (e.g., add
selectPublishAndSend() which delegates to selectPublishType('publish+send')) and
update callers to use the new semantic method; remove or deprecate
clickPublishAndSendLabel() so tests no longer rely on the label-specific
locator.
In `@e2e/helpers/pages/admin/settings/settings-page.ts`:
- Around line 57-59: The getSelectOption method uses
this.page.locator('[data-testid="select-option"]') which is inconsistent with
the rest of the file; update the implementation of getSelectOption to use
this.page.getByTestId('select-option') and preserve the existing .filter({
hasText: text }) call so it returns the same Locator while matching the
project's getByTestId pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e1e84e0-3ab7-45d6-a910-fa8cf6eecfda
📒 Files selected for processing (10)
e2e/helpers/pages/admin/members/member-details-page.tse2e/helpers/pages/admin/posts/post/index.tse2e/helpers/pages/admin/posts/post/page-editor-page.tse2e/helpers/pages/admin/posts/post/post-editor-page.tse2e/helpers/pages/admin/posts/posts-page.tse2e/helpers/pages/admin/settings/settings-page.tse2e/helpers/pages/public/post-page.tse2e/tests/admin/posts/post-access.test.tse2e/tests/admin/posts/publishing.test.tsghost/core/test/e2e-browser/admin/publishing.spec.js
- Added publishPostWithVisibility helper in post-access tests - Consolidated create-draft-and-navigate patterns across both files
…ublishes helpers Addresses remaining SonarCloud duplication blocks in publishing tests
Tests only verify web availability, not email delivery. Email delivery is covered by newsletter-send.test.ts.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
e2e/tests/admin/posts/publishing.test.ts (2)
243-254:⚠️ Potential issue | 🟠 MajorMake the “unsaved changes” delete path deterministic.
Line 249 creates a saved draft, then Line 251 deletes immediately. If autosave has already settled, this can re-test the saved-delete path instead of unsaved-delete. Add a second edit and assert an unsaved indicator before deleting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/posts/publishing.test.ts` around lines 243 - 254, The delete test currently may hit the saved-delete path because autosave can finish before deletion; modify the test 'delete a post with unsaved changes' (using PostsPage and PostEditorPage) to make the unsaved state deterministic by performing a second edit on the draft (e.g., change body or title via PostEditorPage methods) and then assert the unsaved indicator is present (use whatever unsaved indicator accessor exists on PostEditorPage) before calling editor.openSettingsMenu() and editor.settingsMenu.deletePost(); this guarantees the unsaved-delete flow is exercised.
60-63:⚠️ Potential issue | 🟠 MajorReplace fixed sleep with deterministic polling for scheduled publish checks.
Line 62 uses
waitForTimeout(6000), which is flaky and can still miss scheduler jitter. Poll the public URL/status instead of sleeping.Suggested refactor
-async function waitForScheduledPost(page: Page) { - // Ghost auto-corrects past dates to ~5 seconds from now, so we wait for the scheduler to fire - await page.waitForTimeout(6000); -} +async function waitForScheduledPost(page: Page, slug: string, expectedStatus: number) { + const postPage = new PostPage(page); + await expect.poll(async () => { + const response = await postPage.goto(slug, {waitUntil: 'commit'}); + return response?.status(); + }, {timeout: 15000}).toBe(expectedStatus); +}- await waitForScheduledPost(page); + await waitForScheduledPost(page, slug, 200);- await waitForScheduledPost(page); + await waitForScheduledPost(page, '/scheduled-page-test/', 200);- await waitForScheduledPost(page); + await waitForScheduledPost(page, '/scheduled-email-only-test/', 404);As per coding guidelines, "Do not use hard-coded waits (
waitForTimeout)" and "Use Playwright's auto-waiting feature instead of hard-coded waits".Also applies to: 67-67, 138-138, 202-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/posts/publishing.test.ts` around lines 60 - 63, The current waitForScheduledPost function (and other occurrences using waitForTimeout at lines referenced) uses a fixed 6s sleep which is flaky; replace it with deterministic polling: repeatedly check the public post URL or the post status endpoint (using page.goto/page.waitForResponse or fetch from the test context) until the post returns the expected published state or HTTP 200, with a capped timeout and short interval between attempts. Update waitForScheduledPost to accept the post URL or ID, implement a retry loop that queries the public URL/status and breaks when published (or throws after timeout), and remove other hard-coded waitForTimeout uses in the same test file (search for waitForTimeout calls at the referenced locations) in favor of this polling helper so Playwright auto-waiting and deterministic checks are used instead of fixed sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/tests/admin/posts/publishing.test.ts`:
- Around line 243-254: The delete test currently may hit the saved-delete path
because autosave can finish before deletion; modify the test 'delete a post with
unsaved changes' (using PostsPage and PostEditorPage) to make the unsaved state
deterministic by performing a second edit on the draft (e.g., change body or
title via PostEditorPage methods) and then assert the unsaved indicator is
present (use whatever unsaved indicator accessor exists on PostEditorPage)
before calling editor.openSettingsMenu() and editor.settingsMenu.deletePost();
this guarantees the unsaved-delete flow is exercised.
- Around line 60-63: The current waitForScheduledPost function (and other
occurrences using waitForTimeout at lines referenced) uses a fixed 6s sleep
which is flaky; replace it with deterministic polling: repeatedly check the
public post URL or the post status endpoint (using
page.goto/page.waitForResponse or fetch from the test context) until the post
returns the expected published state or HTTP 200, with a capped timeout and
short interval between attempts. Update waitForScheduledPost to accept the post
URL or ID, implement a retry loop that queries the public URL/status and breaks
when published (or throws after timeout), and remove other hard-coded
waitForTimeout uses in the same test file (search for waitForTimeout calls at
the referenced locations) in favor of this polling helper so Playwright
auto-waiting and deterministic checks are used instead of fixed sleeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ff99021-4fb2-4482-b5d5-fcf83c8fd185
📒 Files selected for processing (1)
e2e/tests/admin/posts/publishing.test.ts
Wait for autosave to complete, then type additional content to create a genuinely unsaved state before deleting. The previous version raced with autosave and might delete an already-saved post.
- Added lexical-editor.test.ts with primary and secondary editor tests - Added secondaryEditor locator to PostEditorPage - Removed publishing.spec.js (all 20 tests now ported to e2e/)
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23610226252 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
- Wait for title save before typing body in createDraft to prevent POST response from overwriting local editor state - Use .first() on postStatus locator to avoid strict mode violations when multiple posts exist in the database - Override titleInput in PageEditorPage to match 'Page title' instead of 'Post title' - Relaxed date format assertion to work across theme variations
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23617117476 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Read magic link from input field instead of navigator.clipboard.readText() which fails in headless CI browsers without clipboard permissions.
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23618498754 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
- Read impersonation link from input instead of clipboard API - Wait for magic link URL to populate before reading - Close impersonation modal after getting link - Use separate browser contexts for member impersonation tests - Navigate to frontend URL directly instead of relying on popup
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23621093743 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
|
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23651526230 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
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
1 participant
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
ghost/core/test/e2e-browser/admin/publishing.spec.jsto the new Playwright e2e frameworke2e/tests/admin/posts/publishing.test.ts(13 tests: publish, schedule, update, delete flows)e2e/tests/admin/posts/post-access.test.ts(6 tests: visibility gates, tier restrictions, timezone, default recipients)PostEditorPage(added PublishFlow, UpdateFlow, SettingsMenu),PostPage,MemberDetailsPage,SettingsPage,PostsPagePageEditorPagefor page-specific editor testspublishing.spec.jsto keep only 2 Lexical rendering tests (not yet ported)Test plan
cd e2e && yarn test tests/admin/posts/publishing.test.tscd e2e && yarn test tests/admin/posts/post-access.test.tscd e2e && yarn lint && yarn test:typescd ghost/core && yarn test:browser --grep "Lexical"