Moved publishing scheduling tests to e2e#27031
Conversation
WalkthroughGateway startup is moved earlier in e2e setup so the gateway container is initialized and its network IP is discovered before creating/reusing Ghost containers. Ghost container creation now accepts a Possibly related PRs
🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/e2e-browser/admin/publishing.spec.js (1)
89-90: Empty test describe block should be removed.The
test.describe('Publishing')block is empty and serves no purpose. Consider removing it as part of the cleanup.🧹 Proposed fix
-test.describe('Publishing', () => { -}); - test.describe('Updating post access', () => {🤖 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 89 - 90, Remove the empty test.describe('Publishing') block since it serves no purpose; locate the test.describe('Publishing') declaration in the file and delete the entire empty block (the opening test.describe('Publishing', () => { and its closing });) so no empty describe remains.e2e/helpers/pages/public/post-page.ts (1)
26-27: CSS class selectors used instead of semantic locators or data-testid.These locators use CSS class selectors (
.gh-post-upgrade-cta-content), which violates the coding guidelines. Consider addingdata-testidattributes to the frontend template (ghost/core/core/frontend/helpers/tpl/content-cta.hbs) and usinggetByTestId()instead.♻️ Proposed fix (requires template change)
In
ghost/core/core/frontend/helpers/tpl/content-cta.hbs, add test IDs:- <div class="gh-post-upgrade-cta-content" style="background-color: {{`@site.accent_color`}}"> + <div class="gh-post-upgrade-cta-content" data-testid="access-cta-content" style="background-color: {{`@site.accent_color`}}">Then in page object:
- this.accessCtaContent = page.locator('.gh-post-upgrade-cta-content'); - this.accessCtaHeading = this.accessCtaContent.locator('h2'); + this.accessCtaContent = page.getByTestId('access-cta-content'); + this.accessCtaHeading = this.accessCtaContent.getByRole('heading', {level: 2});As per coding guidelines: "Never use CSS/XPath selectors - only semantic locators or data-testid".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/public/post-page.ts` around lines 26 - 27, The locators accessCtaContent and accessCtaHeading in the PostPage class currently use CSS class selectors ('.gh-post-upgrade-cta-content' and 'h2'); update the frontend template (tpl/content-cta.hbs) to add data-testid attributes for the CTA container and heading, then replace the locators to use semantic test ids (e.g., getByTestId('post-upgrade-cta') and getByTestId('post-upgrade-cta-heading') or the page.locator equivalent using [data-testid="..."]) so the page object references data-testid instead of CSS classes.
🤖 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/pages/public/post-page.ts`:
- Around line 26-27: The locators accessCtaContent and accessCtaHeading in the
PostPage class currently use CSS class selectors ('.gh-post-upgrade-cta-content'
and 'h2'); update the frontend template (tpl/content-cta.hbs) to add data-testid
attributes for the CTA container and heading, then replace the locators to use
semantic test ids (e.g., getByTestId('post-upgrade-cta') and
getByTestId('post-upgrade-cta-heading') or the page.locator equivalent using
[data-testid="..."]) so the page object references data-testid instead of CSS
classes.
In `@ghost/core/test/e2e-browser/admin/publishing.spec.js`:
- Around line 89-90: Remove the empty test.describe('Publishing') block since it
serves no purpose; locate the test.describe('Publishing') declaration in the
file and delete the entire empty block (the opening test.describe('Publishing',
() => { and its closing });) so no empty describe remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e152cc7-5e6d-42bb-b13b-ddda9f06500c
📒 Files selected for processing (6)
e2e/helpers/environment/service-managers/ghost-manager.tse2e/helpers/pages/admin/posts/post/post-editor-page.tse2e/helpers/pages/public/post-page.tse2e/tests/admin/posts/publishing.test.tsghost/core/core/boot.jsghost/core/test/e2e-browser/admin/publishing.spec.js
9larsons
force-pushed
the
migrate-publishing-e2e-slice-5
branch
from
52d8e7b to
2bc6dd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 273-274: The test is asserting a transient "few seconds"
relative-time string via the third arg to expectPostStatus (e.g.,
expectPostStatus(editor, 'Scheduled', /few seconds/i)), which flakes; remove the
volatile regex-based check and instead assert the tolerant status transition to
"Scheduled" (call expectPostStatus(editor, 'Scheduled') or pass a
non-time-dependent matcher) and rely on existing stable end-state assertions for
"Published"/"Sent" later in the flow; update all occurrences of expectPostStatus
with a third-argument regex (the instances around expectPostStatus(editor,
'Scheduled', /few seconds/i) and similar calls) to omit the transient check so
tests only verify stable states.
- Around line 295-301: The test uses fixed emails when calling
memberFactory.create (via createMemberFactory) which causes uniqueness
collisions on retries; change both places that pass
'scheduled-publish-email@example.com' and the other fixed address to generate
unique emails (e.g., append a timestamp/UUID or use a test helper like
generateUniqueEmail) before calling memberFactory.create so each test run
creates a distinct member; update the calls in publishing.test.ts that invoke
createMemberFactory and memberFactory.create to use the new unique-email value.
🪄 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: 9b068645-e64b-41b1-b34b-df3d4ff94d54
📒 Files selected for processing (4)
e2e/helpers/environment/service-managers/ghost-manager.tse2e/tests/admin/posts/publishing.test.tsghost/core/core/boot.jsghost/core/test/e2e-browser/admin/publishing.spec.js
💤 Files with no reviewable changes (1)
- ghost/core/test/e2e-browser/admin/publishing.spec.js
✅ Files skipped from review due to trivial changes (1)
- e2e/helpers/environment/service-managers/ghost-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/boot.js
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23805097598 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref #27031 The scheduler now targets the worker gateway by internal IP, and the first scheduled publish test avoids extra pre-publish waits so it stays within the default timeout.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
e2e/tests/admin/posts/publishing.test.ts (4)
293-299:⚠️ Potential issue | 🟡 MinorUse unique member email to avoid collisions on retries/reruns.
The fixed email
'scheduled-publish-email@example.com'can fail on uniqueness constraints if a previous test run didn't clean up properly or tests are retried.Suggested fix - use timestamp-based email
const title = `scheduled-publish-email-${Date.now()}`; + const email = `scheduled-publish-email-${Date.now()}@example.com`; const body = 'This is my scheduled publish and email post body.'; const slug = generateSlug(title); const memberFactory = createMemberFactory(page.request); const newsletters = await getNewsletters(page.request); await memberFactory.create({ - email: 'scheduled-publish-email@example.com', + email, name: 'Publishing member', newsletters });As per coding guidelines, "Use factories for all test data" and the AAA pattern implies tests should be independently runnable.
🤖 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 293 - 299, The test uses a fixed email ('scheduled-publish-email@example.com') when calling memberFactory.create which can cause uniqueness collisions on retries; update the test to generate a unique email (e.g., append a timestamp or UUID) before calling create so each run uses a distinct address. Locate the memberFactory.create call in the publishing.test.ts block that follows getNewsletters(page.request) and replace the fixed string with a generated value (using the test runtime timestamp or a random suffix) while keeping the rest of the payload (name, newsletters) unchanged.
337-343:⚠️ Potential issue | 🟡 MinorUse unique member email for email-only scheduling test.
Same collision risk as the publish+email test above.
Suggested fix
const title = `scheduled-email-only-${Date.now()}`; + const email = `scheduled-email-only-${Date.now()}@example.com`; const body = 'This is my scheduled email-only post body.'; const slug = generateSlug(title); const memberFactory = createMemberFactory(page.request); const newsletters = await getNewsletters(page.request); await memberFactory.create({ - email: 'scheduled-email-only@example.com', + email, name: 'Publishing member', newsletters });🤖 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 337 - 343, The test is using a hard-coded email in the memberFactory.create call which can collide with other tests; update the code around createMemberFactory(page.request) and the memberFactory.create invocation to generate and use a unique email (e.g., include Date.now(), a random/nanoid suffix, or a test-unique helper) instead of 'scheduled-email-only@example.com' so the scheduling-only test gets an isolated member email and avoids collisions with the publish+email test.
389-389:⚠️ Potential issue | 🟡 MinorRemove volatile
/few seconds/iassertion for page scheduling test.Same flakiness concern as the post scheduling test. The status will transition to "Published" shortly after, which is already verified by
expectFrontendStatus.Suggested fix
- await expectPostStatus(editor, 'Scheduled', /few seconds/i); + await expect(editor.postStatus.first()).toContainText('Scheduled');🤖 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` at line 389, The flaky timing assertion is the regex `/few seconds/i` passed to expectPostStatus; remove this volatile matcher and assert only the stable status string instead (i.e., call expectPostStatus(editor, 'Scheduled') without the regex), since the subsequent transition to "Published" is already verified by expectFrontendStatus; update the call site in the test referencing expectPostStatus to drop the second argument so the test no longer depends on transient timing text.
314-315:⚠️ Potential issue | 🟡 MinorVolatile relative-time assertion
/few seconds/ican flake.This assertion depends on UI timing wording that varies with scheduler execution speed. If the scheduler runs slower than expected, the text may change before the assertion runs.
Suggested fix
await expectPostStatus(editor, 'Scheduled', /published and sent/i); - await expectPostStatus(editor, 'Scheduled', /few seconds/i);The
/published and sent/icheck on line 314 already validates the scheduling intent; the timing assertion adds flake risk without additional coverage value.🤖 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 314 - 315, Remove the brittle relative-time assertion that can flake: delete or disable the second call to expectPostStatus(editor, 'Scheduled', /few seconds/i) and keep the existing expectPostStatus(editor, 'Scheduled', /published and sent/i) check which already validates scheduling; update any comments if needed to note timing UI is intentionally not asserted.
🧹 Nitpick comments (1)
e2e/helpers/environment/service-managers/ghost-manager.ts (1)
213-214: Localhost fallback would produce incorrect scheduler URL if reached.The fallback
http://localhost:${this.getGatewayPort()}/ghost/api/adminwould be unreachable from inside the Ghost container sincelocalhostrefers to the container itself, not the host where the gateway port is bound. However, this path appears unreachable in practice:setup()always passes an explicitschedulerUrl, andrestartWithDatabase()resolves viagetGatewaySchedulerUrl()whengatewayContaineris set.Consider throwing an error instead to fail fast if this unexpected state is reached:
Suggested defensive change
- `scheduling__schedulerUrl=${schedulerUrl || `http://localhost:${this.getGatewayPort()}/ghost/api/admin`}` + `scheduling__schedulerUrl=${schedulerUrl}`And validate
schedulerUrlis provided at the call site or increateGhostContainer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/environment/service-managers/ghost-manager.ts` around lines 213 - 214, The current fallback that builds scheduling__schedulerUrl using `http://localhost:${this.getGatewayPort()}/ghost/api/admin` is invalid inside the Ghost container; replace that silent fallback with a fail-fast error: when `schedulerUrl` is falsy, throw an Error indicating a missing scheduler URL (mention `scheduling__schedulerUrl`), and update callers (e.g., `setup()`, `restartWithDatabase()`, and `createGhostContainer`) to ensure they pass a valid `schedulerUrl` (or obtain it via `getGatewaySchedulerUrl()` or `getGatewayPort()` before calling); this ensures `getGatewayPort()` isn't used to construct an unreachable `localhost` URL and surfaces configuration problems immediately.
🤖 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 293-299: The test uses a fixed email
('scheduled-publish-email@example.com') when calling memberFactory.create which
can cause uniqueness collisions on retries; update the test to generate a unique
email (e.g., append a timestamp or UUID) before calling create so each run uses
a distinct address. Locate the memberFactory.create call in the
publishing.test.ts block that follows getNewsletters(page.request) and replace
the fixed string with a generated value (using the test runtime timestamp or a
random suffix) while keeping the rest of the payload (name, newsletters)
unchanged.
- Around line 337-343: The test is using a hard-coded email in the
memberFactory.create call which can collide with other tests; update the code
around createMemberFactory(page.request) and the memberFactory.create invocation
to generate and use a unique email (e.g., include Date.now(), a random/nanoid
suffix, or a test-unique helper) instead of 'scheduled-email-only@example.com'
so the scheduling-only test gets an isolated member email and avoids collisions
with the publish+email test.
- Line 389: The flaky timing assertion is the regex `/few seconds/i` passed to
expectPostStatus; remove this volatile matcher and assert only the stable status
string instead (i.e., call expectPostStatus(editor, 'Scheduled') without the
regex), since the subsequent transition to "Published" is already verified by
expectFrontendStatus; update the call site in the test referencing
expectPostStatus to drop the second argument so the test no longer depends on
transient timing text.
- Around line 314-315: Remove the brittle relative-time assertion that can
flake: delete or disable the second call to expectPostStatus(editor,
'Scheduled', /few seconds/i) and keep the existing expectPostStatus(editor,
'Scheduled', /published and sent/i) check which already validates scheduling;
update any comments if needed to note timing UI is intentionally not asserted.
---
Nitpick comments:
In `@e2e/helpers/environment/service-managers/ghost-manager.ts`:
- Around line 213-214: The current fallback that builds scheduling__schedulerUrl
using `http://localhost:${this.getGatewayPort()}/ghost/api/admin` is invalid
inside the Ghost container; replace that silent fallback with a fail-fast error:
when `schedulerUrl` is falsy, throw an Error indicating a missing scheduler URL
(mention `scheduling__schedulerUrl`), and update callers (e.g., `setup()`,
`restartWithDatabase()`, and `createGhostContainer`) to ensure they pass a valid
`schedulerUrl` (or obtain it via `getGatewaySchedulerUrl()` or
`getGatewayPort()` before calling); this ensures `getGatewayPort()` isn't used
to construct an unreachable `localhost` URL and surfaces configuration problems
immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e934313-9779-43da-9e34-8f8db85bc5bc
📒 Files selected for processing (2)
e2e/helpers/environment/service-managers/ghost-manager.tse2e/tests/admin/posts/publishing.test.ts
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23807800521 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
The tier visibility test was driving the admin UI to set visibility and select tiers, but clearVisibilityTiers() wasn't reliably clearing pre-selected tiers. Switched visibility tests to use API-based post creation via postFactory.create() instead.
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
e2e/tests/admin/posts/publishing.test.ts (3)
241-252:⚠️ Potential issue | 🟠 MajorUse unique member emails in these new scheduling tests.
These setups reintroduce fixed addresses, so retries/reruns can fail on member uniqueness before the scheduling flow is exercised. Derive the email from
slugor another per-test unique value in both cases.💡 Suggested change
test('scheduled publish and email - post becomes visible on frontend', async ({page}) => { const title = `scheduled-publish-email-${Date.now()}`; const body = 'This is my scheduled publish and email post body.'; const slug = generateSlug(title); + const email = `${slug}@example.com`; const memberFactory = createMemberFactory(page.request); const newsletters = await getNewsletters(page.request); await memberFactory.create({ - email: 'scheduled-publish-email@example.com', + email, name: 'Publishing member', newsletters });test('scheduled email only - post is not visible on frontend', async ({page}) => { const title = `scheduled-email-only-${Date.now()}`; const body = 'This is my scheduled email-only post body.'; const slug = generateSlug(title); + const email = `${slug}@example.com`; const memberFactory = createMemberFactory(page.request); const newsletters = await getNewsletters(page.request); await memberFactory.create({ - email: 'scheduled-email-only@example.com', + email, name: 'Publishing member', newsletters });Also applies to: 285-296
🤖 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 241 - 252, The tests "scheduled publish and email - post becomes visible on frontend" and the other scheduling test use fixed member emails causing uniqueness failures on retries; update the calls to memberFactory.create inside these tests to generate unique emails (e.g., derive from the test-specific slug or Date.now()) instead of the hardcoded 'scheduled-publish-email@example.com' (and the analogous hardcoded address in the second test), so modify the memberFactory.create invocation(s) to use `${slug}@example.com` or a similarly derived per-test value; locate the changes around the test function name and the memberFactory.create calls to apply this fix.
273-274:⚠️ Potential issue | 🟠 MajorDrop the initial anonymous
404check in the ASAP publish flows.With a backdated schedule, the first frontend probe can already be
200, so the404 -> 200transition is nondeterministic. The stable assertion here is the eventual200plus the content checks below.💡 Suggested change
- await expectFrontendStatus(frontendPage, slug, 404); await expectFrontendStatus(frontendPage, slug, 200, 20000);- await expectFrontendStatus(frontendPage, slug, 404); await expectFrontendStatus(frontendPage, slug, 200, 20000);Also applies to: 347-348
🤖 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 273 - 274, Remove the initial anonymous 404 probe in the ASAP publish test flows: delete the call expectFrontendStatus(frontendPage, slug, 404) so the test only waits for the eventual success check expectFrontendStatus(frontendPage, slug, 200, 20000) and then performs the content assertions; do the same for the other ASAP publish occurrence that currently calls expectFrontendStatus(frontendPage, slug, 404) followed by the 200 check.
226-226:⚠️ Potential issue | 🟠 MajorASAP scheduling checks shouldn't require the editor to still say
Scheduled.
getAsapSchedule()deliberately backdates the job, so these assertions can race the worker and miss straight toPublishedorSent. Make the intermediate editor checks tolerant of the fast-path terminal state and keep the stable end-state assertions later in each flow.💡 Suggested change
- await expect(editor.postStatus.first()).toContainText('Scheduled'); + await expect.poll(async () => (await editor.postStatus.first().textContent()) ?? '', { + timeout: 20000 + }).toMatch(/Scheduled|Published/i);- await expectPostStatus(editor, 'Scheduled', /to be sent/i); + await expect.poll(async () => (await editor.postStatus.first().textContent()) ?? '', { + timeout: 20000 + }).toMatch(/Scheduled|Sent/i);Apply the same pattern to the
publish+sendand page ASAP checks instead of pinning them to an exactScheduled/few secondsstate.Also applies to: 267-268, 311-311, 342-342
🤖 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 241-252: The tests "scheduled publish and email - post becomes
visible on frontend" and the other scheduling test use fixed member emails
causing uniqueness failures on retries; update the calls to memberFactory.create
inside these tests to generate unique emails (e.g., derive from the
test-specific slug or Date.now()) instead of the hardcoded
'scheduled-publish-email@example.com' (and the analogous hardcoded address in
the second test), so modify the memberFactory.create invocation(s) to use
`${slug}@example.com` or a similarly derived per-test value; locate the changes
around the test function name and the memberFactory.create calls to apply this
fix.
- Around line 273-274: Remove the initial anonymous 404 probe in the ASAP
publish test flows: delete the call expectFrontendStatus(frontendPage, slug,
404) so the test only waits for the eventual success check
expectFrontendStatus(frontendPage, slug, 200, 20000) and then performs the
content assertions; do the same for the other ASAP publish occurrence that
currently calls expectFrontendStatus(frontendPage, slug, 404) followed by the
200 check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44e39a8c-50f9-4552-a468-4dfca82a7cd7
📒 Files selected for processing (2)
e2e/data-factory/factories/post-factory.tse2e/tests/admin/posts/publishing.test.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/data-factory/factories/post-factory.ts
1 participant
What
Testing
Notes