Moved core publishing browser tests to e2e suite (slice 1/4)#27004
Open
Moved core publishing browser tests to e2e suite (slice 1/4)#27004
Conversation
Moved 7 tests (publish only, publish+email, email only, delete saved post, delete with unsaved changes, lexical editor, secondary lexical editor) from ghost/core/test/e2e-browser/admin/publishing.spec.js to the e2e suite. This is the first slice of an incremental migration — the simplest, most deterministic tests that don't involve scheduling or access control.
Contributor
WalkthroughEnd-to-end test code was refactored: new helper locators and methods were added to admin post editor helpers ( 🚥 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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/tests/admin/posts/publishing.test.ts (2)
42-46: Type mismatch hidden byas nevercast.The
newslettersparameter expectsstring[]per theMemberFactoryinterface, butgetNewsletters()returns{id: string}[]. Theas nevercast bypasses TypeScript's type checking. This works at runtime because the API adapter handles the transformation, but it's fragile.Consider updating the type signature or using a more explicit cast:
Suggested improvement
await memberFactory.create({ email: 'publish-email-test@example.com', name: 'Publishing member', - newsletters: newsletters as never + newsletters: newsletters as unknown as string[] });Or better, update the
MemberFactoryinterface to accept{id: string}[]for newsletters since that's what the API expects.Also applies to: 71-75
🤖 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 42 - 46, The test is hiding a type mismatch by using `as never` when calling `memberFactory.create`; `getNewsletters()` returns `{id: string}[]` but `MemberFactory` currently expects `string[]`. Fix by either updating the `MemberFactory` interface to accept `Array<{id: string}>` for the `newsletters` parameter (so `memberFactory.create` and tests pass naturally) or explicitly map the result of `getNewsletters()` to a `string[]` (e.g., `getNewsletters().map(n => n.id)`) before passing it to `memberFactory.create`; update calls at the two test sites (the call at `memberFactory.create` around lines ~42 and the similar one around lines ~71) and adjust types on the `MemberFactory` definition accordingly to keep typesafe code.
7-11: Consider adding error handling for the API call.The
getNewsletters()helper doesn't check the response status before parsing JSON. If the API call fails, this could produce confusing errors.Suggested improvement
async function getNewsletters(request: APIRequestContext): Promise<{id: string}[]> { const response = await request.get('/ghost/api/admin/newsletters/?status=active&limit=all'); + if (!response.ok()) { + throw new Error(`Failed to fetch newsletters: ${response.status()}`); + } const data = await response.json(); return data.newsletters.map((n: {id: string}) => ({id: n.id})); }🤖 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 7 - 11, The getNewsletters helper currently calls request.get and immediately parses response.json which can produce confusing errors on non-2xx responses; update getNewsletters to check response.ok (or response.status) after request.get('/ghost/api/admin/newsletters/?status=active&limit=all') and if not ok throw or return a clear error that includes response.status and response.statusText (optionally include the response body for debugging by awaiting response.text()), otherwise proceed to await response.json() and map data.newsletters to ({id: n.id}) as before.
🤖 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/helpers/pages/admin/posts/post/post-editor-page.ts`:
- Around line 115-122: The hard-coded waitForTimeout(100) inside createDraft()
causes flakiness; replace it with an explicit wait that asserts the editor has
completed the new->draft transition before typing the body. Concretely, after
filling title and pressing Enter (in createDraft), wait on the editor locator
returned by page.locator('[data-lexical-editor="true"]').first() for a stable
condition such as: the editor becoming editable/focused, its innerText or child
paragraph element appearing/containing expected content, or a specific
attribute/class that indicates the draft state has been applied; then proceed
with page.keyboard.type(body). Use createDraft, titleInput, and the editor
locator to locate and wait for that deterministic condition instead of
waitForTimeout.
---
Nitpick comments:
In `@e2e/tests/admin/posts/publishing.test.ts`:
- Around line 42-46: The test is hiding a type mismatch by using `as never` when
calling `memberFactory.create`; `getNewsletters()` returns `{id: string}[]` but
`MemberFactory` currently expects `string[]`. Fix by either updating the
`MemberFactory` interface to accept `Array<{id: string}>` for the `newsletters`
parameter (so `memberFactory.create` and tests pass naturally) or explicitly map
the result of `getNewsletters()` to a `string[]` (e.g., `getNewsletters().map(n
=> n.id)`) before passing it to `memberFactory.create`; update calls at the two
test sites (the call at `memberFactory.create` around lines ~42 and the similar
one around lines ~71) and adjust types on the `MemberFactory` definition
accordingly to keep typesafe code.
- Around line 7-11: The getNewsletters helper currently calls request.get and
immediately parses response.json which can produce confusing errors on non-2xx
responses; update getNewsletters to check response.ok (or response.status) after
request.get('/ghost/api/admin/newsletters/?status=active&limit=all') and if not
ok throw or return a clear error that includes response.status and
response.statusText (optionally include the response body for debugging by
awaiting response.text()), otherwise proceed to await response.json() and map
data.newsletters to ({id: n.id}) as before.
🪄 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: a2a16031-7556-4f40-9baf-7e317fc4f284
📒 Files selected for processing (5)
e2e/helpers/pages/admin/posts/post/post-editor-page.tse2e/helpers/pages/public/post-page.tse2e/tests/admin/posts/lexical-editor.test.tse2e/tests/admin/posts/publishing.test.tsghost/core/test/e2e-browser/admin/publishing.spec.js
Comment on lines
+115
to
+122
| async createDraft({title = 'Hello world', body = 'This is my post body.'} = {}): Promise<void> { | ||
| await this.titleInput.click(); | ||
| await this.titleInput.fill(title); | ||
| await this.page.locator('[data-lexical-editor="true"]').first().waitFor({state: 'visible'}); | ||
| await this.page.keyboard.press('Enter'); | ||
| await this.page.waitForTimeout(100); | ||
| await this.page.keyboard.type(body); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid hard-coded waitForTimeout in createDraft().
Line 120 uses page.waitForTimeout(100) which violates the coding guidelines. The comment in the legacy test explains this wait allows the "new->draft switch to occur fully," but this introduces flakiness.
Consider replacing with a proper wait condition, such as waiting for the editor state to stabilize or for a specific element attribute change.
Suggested fix using a more reliable wait
async createDraft({title = 'Hello world', body = 'This is my post body.'} = {}): Promise<void> {
await this.titleInput.click();
await this.titleInput.fill(title);
await this.page.locator('[data-lexical-editor="true"]').first().waitFor({state: 'visible'});
await this.page.keyboard.press('Enter');
- await this.page.waitForTimeout(100);
+ // Wait for the editor to be ready for input after the title->body transition
+ await this.page.locator('[data-lexical-editor="true"]').first().waitFor({state: 'visible'});
await this.page.keyboard.type(body);
}As per coding guidelines: "Do not use hard-coded waits (waitForTimeout)".
🤖 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 115 -
122, The hard-coded waitForTimeout(100) inside createDraft() causes flakiness;
replace it with an explicit wait that asserts the editor has completed the
new->draft transition before typing the body. Concretely, after filling title
and pressing Enter (in createDraft), wait on the editor locator returned by
page.locator('[data-lexical-editor="true"]').first() for a stable condition such
as: the editor becoming editable/focused, its innerText or child paragraph
element appearing/containing expected content, or a specific attribute/class
that indicates the draft state has been applied; then proceed with
page.keyboard.type(body). Use createDraft, titleInput, and the editor locator to
locate and wait for that deterministic condition instead of waitForTimeout.
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23652926430 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
- Removed premature 'Draft - Saved' wait from createDraft (auto-save may not complete before publish flow opens, matching original behavior) - Added separate waitForSaved() method for tests that need it - Removed 'Published' status assertion after closing publish flow (close now navigates to posts list, not back to editor)
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/tests/admin/posts/publishing.test.ts (3)
52-59: Inconsistent publish flow closing between tests.The first test calls
editor.publishFlow.close()after confirming, but this test navigates directly without closing the modal. While this may work due to page navigation, the inconsistency could lead to flaky behavior if the modal interferes with navigation.Consider either:
- Adding
await editor.publishFlow.close()for consistency, or- Documenting why it's intentionally omitted here
🤖 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 52 - 59, The publish flow modal is left open in this test which is inconsistent with the first test; after calling editor.publishFlow.confirm() you should explicitly close the modal to avoid flakiness by calling await editor.publishFlow.close() before navigating to the PostPage (refer to editor.publishFlow.confirm(), editor.publishFlow.close(), publishFlow.selectPublishType, PostPage, generateSlug and postData in this block), so add the close call immediately after confirm() or add a short comment explaining why the modal remaining open is intentional.
7-11: Add defensive error handling for the API response.The helper assumes a successful response with the expected structure. If the API fails or returns an unexpected shape, the error message will be unclear (e.g., "Cannot read properties of undefined").
🛡️ Suggested improvement
async function getNewsletters(request: APIRequestContext): Promise<{id: string}[]> { const response = await request.get('/ghost/api/admin/newsletters/?status=active&limit=all'); + if (!response.ok()) { + throw new Error(`Failed to fetch newsletters: ${response.status()}`); + } const data = await response.json(); + if (!data.newsletters?.length) { + throw new Error('No active newsletters found - tests require at least one newsletter'); + } return data.newsletters.map((n: {id: string}) => ({id: n.id})); }🤖 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 7 - 11, The helper getNewsletters currently assumes a successful response and valid shape; add defensive checks: after request.get('/ghost/api/admin/newsletters/?status=active&limit=all') verify response.ok and if not throw a descriptive Error that includes response.status and statusText (and optionally response.text()); parse JSON and ensure the parsed object has a newsletters array (Array.isArray(data.newsletters)); if the shape is invalid throw a clear Error describing the unexpected payload; finally return the mapped ids only when validation passes.
39-43: Theas nevertype assertion bypasses type safety.This cast suggests a type mismatch between the newsletter structure from the API and what
memberFactory.createexpects. Consider aligning the types or using a proper type assertion.♻️ Suggested approach
If the factory expects a different newsletter type, define an explicit interface and map accordingly:
- await memberFactory.create({ - email: 'publish-email-test@example.com', - name: 'Publishing member', - newsletters: newsletters as never - }); + await memberFactory.create({ + email: 'publish-email-test@example.com', + name: 'Publishing member', + newsletters + });If the factory's type definition is incorrect, updating the factory's type signature would be cleaner than using
as neverin multiple places.🤖 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 39 - 43, The test is skipping type checking by using "newsletters as never" when calling memberFactory.create; update the call to pass a correctly typed value instead of casting to never: either convert/map the existing newsletters variable to the shape expected by memberFactory.create (create a small mapper or explicit interface to transform newsletter objects) or fix the factory's type signature so it accepts the API newsletter type; locate the memberFactory.create invocation and the newsletters definition and replace the "as never" cast with a proper typed object or adjusted factory type.
🤖 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/publishing.test.ts`:
- Around line 52-59: The publish flow modal is left open in this test which is
inconsistent with the first test; after calling editor.publishFlow.confirm() you
should explicitly close the modal to avoid flakiness by calling await
editor.publishFlow.close() before navigating to the PostPage (refer to
editor.publishFlow.confirm(), editor.publishFlow.close(),
publishFlow.selectPublishType, PostPage, generateSlug and postData in this
block), so add the close call immediately after confirm() or add a short comment
explaining why the modal remaining open is intentional.
- Around line 7-11: The helper getNewsletters currently assumes a successful
response and valid shape; add defensive checks: after
request.get('/ghost/api/admin/newsletters/?status=active&limit=all') verify
response.ok and if not throw a descriptive Error that includes response.status
and statusText (and optionally response.text()); parse JSON and ensure the
parsed object has a newsletters array (Array.isArray(data.newsletters)); if the
shape is invalid throw a clear Error describing the unexpected payload; finally
return the mapped ids only when validation passes.
- Around line 39-43: The test is skipping type checking by using "newsletters as
never" when calling memberFactory.create; update the call to pass a correctly
typed value instead of casting to never: either convert/map the existing
newsletters variable to the shape expected by memberFactory.create (create a
small mapper or explicit interface to transform newsletter objects) or fix the
factory's type signature so it accepts the API newsletter type; locate the
memberFactory.create invocation and the newsletters definition and replace the
"as never" cast with a proper typed object or adjusted factory type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 834d5ef0-fb28-452c-83df-d0e5affda203
📒 Files selected for processing (2)
e2e/helpers/pages/admin/posts/post/post-editor-page.tse2e/tests/admin/posts/publishing.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/helpers/pages/admin/posts/post/post-editor-page.ts
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23654932768 -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 e2e suitePostEditorPagewith publish flow close/delete methods,createDraft()helper, and lexical editor locatorsPostPagewitharticleTitleandarticleBodylocatorsContext
This is slice 1 of 4 in an incremental migration of browser tests to the e2e suite. This slice covers the simplest, most deterministic tests — no scheduling waits, no visibility/access changes, no settings mutations. Remaining slices will cover publish page + update post, schedule post, and post access tests.
Test plan
cd e2e && yarn test tests/admin/posts/publishing.test.tspassescd e2e && yarn test tests/admin/posts/lexical-editor.test.tspassescd e2e && yarn lintpasses (0 new errors)cd e2e && yarn test:typespassespublishing.spec.jsstill pass