Conversation
ref #27087 The above commit disabled the scheduling tests to patch a fix. In effect, we should never have the scheduler point to anything but an admin API endpoint. While the scheduler was kept flexible, at this point there doesn't seem to be a reason to not enforce more rigidity/safety. W/r to testing, it's difficult to test the full scheduling workflow. I've updated the tests to successfully emit the scheduler request - we don't ensure that the scheduler pushes a message back to Ghost, though we may be able to do that with a standalone SchedulerService in the future, or something of the like. Right now, Docker communication is difficult, and that's what caused the original issue.
WalkthroughThe end-to-end publishing tests were updated to replace dynamic "asap" scheduling with a fixed future schedule ( 🚥 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.
Actionable comments posted: 1
🤖 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 46-57: The helper waitForScheduledSaveResponse currently only
checks for a PUT 200 to the admin resource and can be fooled by ordinary
updates; update the predicate to also inspect the request payload from
networkResponse.request().postData(), parse it as JSON, and confirm it carries
the scheduled intent (e.g. contains status: 'scheduled' or a scheduled/publish
date field) before returning true. In practice modify the lambda used by
page.waitForResponse to read request().postData(), safely JSON.parse it, and
return false if parsing fails or the expected scheduled flag/field is absent;
keep the existing method/status/URL checks (refer to
waitForScheduledSaveResponse, networkResponse.request().method(),
networkResponse.status(), and the RegExp matching
`/ghost/api/admin/${resource}/[^/]+/?$`) so only requests that are PUT, 200, to
the right endpoint and contain scheduled payload pass the wait.
🪄 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: 1d6e1fbb-edb4-4ebc-94e5-f4599b431254
📒 Files selected for processing (1)
e2e/tests/admin/posts/publishing.test.ts
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/tests/admin/posts/publishing.test.ts (1)
46-46: Rename the helper to match what it actually waits for.This only observes the editor's admin
PUT, not the scheduler dispatch or callback. A name likewaitForScheduledAdminSaveResponsewould set the right expectation for future readers.As per coding guidelines,
e2e/**/*.{ts,tsx,js,jsx}: Prefer less comments and giving things clear names.🤖 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 46, Rename the helper function waitForScheduledSaveResponse to waitForScheduledAdminSaveResponse to reflect that it only waits for the editor's admin PUT response; update the function declaration (async function waitForScheduledAdminSaveResponse(page: Page, resource: 'posts' | 'pages')) and replace every call/reference to waitForScheduledSaveResponse across tests (imports/usages) to the new name so signatures stay consistent and no references break.
🤖 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 23-25: getFutureSchedule currently returns a hard-coded date
string that tests don't reuse, letting the waiter predicate pass with the wrong
scheduled value; change getFutureSchedule() to return a single source-of-truth
object (e.g., { date, year }) and update all call sites to pass date into the
waiter/helper that asserts published_at so the predicate verifies the exact
scheduled date/year (update references in the scheduled post test and the
waiter/predicate function to compare against the returned date/year from
getFutureSchedule()).
---
Nitpick comments:
In `@e2e/tests/admin/posts/publishing.test.ts`:
- Line 46: Rename the helper function waitForScheduledSaveResponse to
waitForScheduledAdminSaveResponse to reflect that it only waits for the editor's
admin PUT response; update the function declaration (async function
waitForScheduledAdminSaveResponse(page: Page, resource: 'posts' | 'pages')) and
replace every call/reference to waitForScheduledSaveResponse across tests
(imports/usages) to the new name so signatures stay consistent and no references
break.
🪄 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: 5e0a5b7e-7d26-47ff-a8e6-f60c3f0493a3
📒 Files selected for processing (1)
e2e/tests/admin/posts/publishing.test.ts
| function getFutureSchedule() { | ||
| return { | ||
| date: formatDateInput(yesterday), | ||
| time: '00:00' | ||
| date: '2050-01-01' |
There was a problem hiding this comment.
Use one schedule source of truth and validate it in the waiter.
published_at only has to be some string here, so a request with the wrong scheduled day/time can still resolve the wait. Because the tests later hard-code 2050 in their UI assertions, the input and the checks can drift apart. Return the expected date/year from getFutureSchedule() and pass the date into this helper so the predicate verifies the exact schedule value.
🎯 Suggested shape
function getFutureSchedule() {
return {
- date: '2050-01-01'
+ date: '2050-01-01',
+ year: '2050'
};
}
-async function waitForScheduledSaveResponse(page: Page, resource: 'posts' | 'pages') {
+async function waitForScheduledSaveResponse(page: Page, resource: 'posts' | 'pages', scheduledDate: string) {
@@
const resourcePayload = item as Record<string, unknown>;
return resourcePayload.status === 'scheduled' ||
(typeof resourcePayload.published_at === 'string' &&
+ resourcePayload.published_at.startsWith(scheduledDate) &&
resourcePayload.status !== 'published' &&
resourcePayload.status !== 'sent');
});Then reuse the returned date/year in the scheduled test call sites and status assertions.
Also applies to: 46-80
🤖 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 23 - 25,
getFutureSchedule currently returns a hard-coded date string that tests don't
reuse, letting the waiter predicate pass with the wrong scheduled value; change
getFutureSchedule() to return a single source-of-truth object (e.g., { date,
year }) and update all call sites to pass date into the waiter/helper that
asserts published_at so the predicate verifies the exact scheduled date/year
(update references in the scheduled post test and the waiter/predicate function
to compare against the returned date/year from getFutureSchedule()).
1 participant
ref #27087
The above commit disabled the scheduling tests to patch a fix. In effect, we should never have the scheduler point to anything but an admin API endpoint. While the scheduler was kept flexible, at this point there doesn't seem to be a reason to not enforce more rigidity/safety.
W/r to testing, it's difficult to test the full scheduling workflow. I've updated the tests to successfully emit the scheduler request - we don't ensure that the scheduler pushes a message back to Ghost, though we may be able to do that with a standalone SchedulerService in the future, or something of the like. Right now, Docker communication is difficult, and that's what caused the original issue.