Added fake Mailgun server for e2e newsletter testing#26983
Merged
Conversation
Bulk newsletter sends go through the Mailgun API, bypassing SMTP/MailPit. The root e2e framework runs Ghost in Docker where in-process mocking isn't possible, so this adds a fake Mailgun HTTP server (following the fake Stripe pattern) that accepts email batches and forwards personalized emails to MailPit for test assertions. - FakeMailgunServer: Express server handling POST /v3/:domain/messages - MailgunTestService: test-friendly wrapper with waitForMessages polling - mailgunEnabled fixture: configures Ghost via bulkEmail__mailgun__* env vars - emailClient fixture: provides MailPit client to any test - PublishFlow page object: added selectPublishType() and confirm() methods - Smoke test verifying end-to-end newsletter delivery via MailPit
Contributor
WalkthroughAdds E2E email testing: an abstract 🚥 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.
🧹 Nitpick comments (6)
e2e/helpers/pages/admin/posts/post/post-editor-page.ts (2)
43-46: Sibling combinator selector couples to DOM structure.The
+ labeladjacent sibling selector relies on specific DOM structure. If the markup changes, this selector will silently fail to find the element.Consider using a more robust approach, such as a combined data-testid on the clickable element itself or using Playwright's
getByLabel()after the radio button.🤖 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 43 - 46, The selector in selectPublishType currently relies on an adjacent sibling (`[data-test-publish-type="${type}"] + label`) which couples to DOM layout; change it to target the clickable control itself or a stable ARIA/query API: update selectPublishType (and the use of publishTypeButton) to either click a dedicated clickable element that contains the data-test-publish-type attribute (add a combined data-test/data-testid to that element in markup) or use Playwright's semantic queries like page.getByLabel(type) or page.getByRole('radio', { name: /.../ }) to find and click the radio option, removing the `+ label` sibling combinator.
48-52: Usingforce: truemay mask real UI issues.The
force: trueoption bypasses actionability checks. If the confirm button is legitimately not actionable (covered, disabled, etc.), this could hide bugs.Consider adding a
waitFor({state: 'visible'})before clicking instead, or document why forcing is necessary (e.g., known animation timing issue).🤖 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 48 - 52, The confirm() method uses confirmButton.click({force: true}) which masks actionability problems; instead ensure the button is actionable by waiting for visibility and enabled state before clicking: in the confirm() function, after awaiting this.continueButton.click(), add waits for this.confirmButton to be visible and enabled (e.g., waitFor({state: 'visible'}) and a check that it is not disabled) and then perform a normal click without force; if force is required for a documented animation/overlay issue, add a comment in confirm() explaining the exact reason and reference any issue/PR that justifies using {force: true}.e2e/helpers/services/mailgun/fake-mailgun-server.ts (2)
137-149: Consider extracting the nested ternary for clarity.The nested ternary on line 139 handles three cases (array, string, other) but is dense to read at a glance.
♻️ Suggested refactor
bb.on('close', () => { const toField = fields.to; - const toArray = Array.isArray(toField) ? toField : (typeof toField === 'string' ? toField.split(',').map(e => e.trim()) : []); + let toArray: string[]; + if (Array.isArray(toField)) { + toArray = toField; + } else if (typeof toField === 'string') { + toArray = toField.split(',').map(e => e.trim()); + } else { + toArray = []; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/mailgun/fake-mailgun-server.ts` around lines 137 - 149, The nested ternary that builds toArray from toField inside the bb.on('close') handler is hard to read; replace it with a simple conditional block using Array.isArray(toField) to set toArray, else if typeof toField === 'string' then split and trim, else set toArray = []; update the variables referenced (toField and toArray) in the bb.on('close' callback so behavior remains identical but clearer and easier to maintain.
186-188: Silent failure on MailPit forwarding may make debugging difficult.Errors forwarding to MailPit are logged but the original request still returns success. This is likely intentional for test resilience, but consider adding a mechanism (e.g., a
forwardingErrorsarray) to allow tests to detect forwarding failures when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/mailgun/fake-mailgun-server.ts` around lines 186 - 188, The current fire-and-forget call to this.forwardToMailPit(message).catch(...) swallows errors and makes tests unable to assert forwarding failures; add a member array (e.g., forwardingErrors) on the FakeMailgunServer class and, inside forwardToMailPit's catch handler, push the error (or an error record with message id/timestamp) into forwardingErrors in addition to debug logging so tests can inspect forwardingErrors to detect failures; keep the original behavior of not failing the HTTP response but expose the errors via the new forwardingErrors array.e2e/helpers/services/mailgun/mailgun-service.ts (1)
14-17: Prefer.at(-1)for accessing the last array element.Using
.at(-1)is more readable and idiomatic for accessing the last element.♻️ Suggested fix
getLastMessage(): SentMessage | undefined { const messages = this.server.messages; - return messages[messages.length - 1]; + return messages.at(-1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/mailgun/mailgun-service.ts` around lines 14 - 17, The getLastMessage method currently retrieves the last element with messages[messages.length - 1]; replace this with the more idiomatic messages.at(-1) in getLastMessage (method name) so the last message is accessed via Array.prototype.at and naturally returns undefined for empty arrays; update the return expression to use this.messages.at(-1) (or messages.at(-1)) referencing the server.messages array and keep the return type SentMessage | undefined.e2e/tests/admin/posts/newsletter-send.test.ts (1)
21-25: Consider typing the newsletters parameter properly instead of usingas never.The
as nevercast bypasses type checking entirely. A cleaner approach would be to type thegetNewslettersreturn value to match what the factory expects, or extract the required fields explicitly.♻️ Suggested approach
If the factory expects
{id: string}[], the currentgetNewslettersshould already work. If there's a type mismatch, consider updating the factory types or explicitly mapping:const member = await memberFactory.create({ name: 'Newsletter Recipient', email: 'newsletter-test@example.com', newsletters: newsletters.map(n => ({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/newsletter-send.test.ts` around lines 21 - 25, The test is silencing type errors by casting `newsletters` to `never`; instead ensure the value matches the factory's expected shape: update the `getNewsletters` return type or transform `newsletters` into the expected array (e.g., map to objects containing only the required id field) before passing it to `memberFactory.create`; locate uses of `newsletters`, `getNewsletters()` and `memberFactory.create` in the test and change the input so it conforms to the factory's expected `{ id: string }[]` (or adjust the factory's types) rather than using `as never`.
🤖 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/admin/posts/post/post-editor-page.ts`:
- Around line 43-46: The selector in selectPublishType currently relies on an
adjacent sibling (`[data-test-publish-type="${type}"] + label`) which couples to
DOM layout; change it to target the clickable control itself or a stable
ARIA/query API: update selectPublishType (and the use of publishTypeButton) to
either click a dedicated clickable element that contains the
data-test-publish-type attribute (add a combined data-test/data-testid to that
element in markup) or use Playwright's semantic queries like
page.getByLabel(type) or page.getByRole('radio', { name: /.../ }) to find and
click the radio option, removing the `+ label` sibling combinator.
- Around line 48-52: The confirm() method uses confirmButton.click({force:
true}) which masks actionability problems; instead ensure the button is
actionable by waiting for visibility and enabled state before clicking: in the
confirm() function, after awaiting this.continueButton.click(), add waits for
this.confirmButton to be visible and enabled (e.g., waitFor({state: 'visible'})
and a check that it is not disabled) and then perform a normal click without
force; if force is required for a documented animation/overlay issue, add a
comment in confirm() explaining the exact reason and reference any issue/PR that
justifies using {force: true}.
In `@e2e/helpers/services/mailgun/fake-mailgun-server.ts`:
- Around line 137-149: The nested ternary that builds toArray from toField
inside the bb.on('close') handler is hard to read; replace it with a simple
conditional block using Array.isArray(toField) to set toArray, else if typeof
toField === 'string' then split and trim, else set toArray = []; update the
variables referenced (toField and toArray) in the bb.on('close' callback so
behavior remains identical but clearer and easier to maintain.
- Around line 186-188: The current fire-and-forget call to
this.forwardToMailPit(message).catch(...) swallows errors and makes tests unable
to assert forwarding failures; add a member array (e.g., forwardingErrors) on
the FakeMailgunServer class and, inside forwardToMailPit's catch handler, push
the error (or an error record with message id/timestamp) into forwardingErrors
in addition to debug logging so tests can inspect forwardingErrors to detect
failures; keep the original behavior of not failing the HTTP response but expose
the errors via the new forwardingErrors array.
In `@e2e/helpers/services/mailgun/mailgun-service.ts`:
- Around line 14-17: The getLastMessage method currently retrieves the last
element with messages[messages.length - 1]; replace this with the more idiomatic
messages.at(-1) in getLastMessage (method name) so the last message is accessed
via Array.prototype.at and naturally returns undefined for empty arrays; update
the return expression to use this.messages.at(-1) (or messages.at(-1))
referencing the server.messages array and keep the return type SentMessage |
undefined.
In `@e2e/tests/admin/posts/newsletter-send.test.ts`:
- Around line 21-25: The test is silencing type errors by casting `newsletters`
to `never`; instead ensure the value matches the factory's expected shape:
update the `getNewsletters` return type or transform `newsletters` into the
expected array (e.g., map to objects containing only the required id field)
before passing it to `memberFactory.create`; locate uses of `newsletters`,
`getNewsletters()` and `memberFactory.create` in the test and change the input
so it conforms to the factory's expected `{ id: string }[]` (or adjust the
factory's types) rather than using `as never`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ec3d00c-ebda-4f00-be71-a5c631e85237
📒 Files selected for processing (7)
e2e/helpers/pages/admin/posts/post/post-editor-page.tse2e/helpers/playwright/fixture.tse2e/helpers/services/mailgun/fake-mailgun-server.tse2e/helpers/services/mailgun/index.tse2e/helpers/services/mailgun/mailgun-service.tse2e/tests/admin/posts/newsletter-send.test.tse2e/types.d.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/helpers/services/mailgun/fake-mailgun-server.ts (1)
1-3: Remove unusedexpressimport.The
expressmodule is imported but not used directly in this file. The Express app instance is inherited fromFakeServer, andexpress.Request/express.Responsetypes can be imported as types from the express package without importing the default export.🧹 Suggested fix
import busboy from 'busboy'; -import express from 'express'; +import type {Request, Response} from 'express'; import {FakeServer} from '@/helpers/services/fake-server';Then update line 72:
- private handleSendMessage(req: express.Request, res: express.Response): void { + private handleSendMessage(req: Request, res: Response): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/mailgun/fake-mailgun-server.ts` around lines 1 - 3, The file imports the default express export but never uses it; remove the default import and instead import only the types used (e.g. import type { Request, Response } from 'express') so the code uses express types without pulling in the runtime module; update any references that currently use express.Request / express.Response to use the imported Request and Response types and remove the unused express identifier (the FakeServer class remains the source of the app instance).
🤖 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/services/mailgun/fake-mailgun-server.ts`:
- Around line 1-3: The file imports the default express export but never uses
it; remove the default import and instead import only the types used (e.g.
import type { Request, Response } from 'express') so the code uses express types
without pulling in the runtime module; update any references that currently use
express.Request / express.Response to use the imported Request and Response
types and remove the unused express identifier (the FakeServer class remains the
source of the app instance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86b32a07-8250-4607-af7e-78eb6bc9d097
📒 Files selected for processing (3)
e2e/helpers/services/fake-server.tse2e/helpers/services/mailgun/fake-mailgun-server.tse2e/helpers/services/stripe/fake-stripe-server.ts
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/services/mailgun/fake-mailgun-server.ts`:
- Around line 39-42: The fake Mailgun server currently accepts all /v3/:domain/*
requests; add an expected API key check so requests without proper Mailgun Basic
auth are rejected: extend the constructor (FakeMailgunServer or its constructor)
to accept options.mailgunApiKey, store it, and compute the expected
Authorization header value as "Basic " + base64("api:" + mailgunApiKey); then in
the request handler for /v3/:domain/* (the route handling logic around lines
~52-64) validate req.headers.authorization against that expected value and
return a 401 response (and no downstream forwarding) when missing or incorrect.
Ensure both the constructor and the route handler are updated so the server only
accepts calls with the configured Mailgun credentials.
- Around line 66-69: The catch-all middleware registered via this.app.use
currently returns a 200 OK for unimplemented Mailgun routes; change it to return
a failing status (e.g., 404 Not Found or 501 Not Implemented) so unexpected
endpoints fail tests: update the catch-all handler in fake-mailgun-server (the
this.app.use anonymous middleware that calls this.debug) to respond with a
non-200 status and include the method and originalUrl in the JSON body/log to
make failures obvious.
- Around line 95-103: The current code silently swallows JSON parse errors for
fields['recipient-variables'] (rvField) and continues with recipientVariables =
{}, which lets malformed personalization payloads pass; change the catch to fail
fast: when JSON.parse(rvField) throws, log the error (include the exception) and
return a 400 error response (or throw an error) indicating malformed
"recipient-variables" instead of proceeding, so that the handler for the Mailgun
endpoint (the code around fields, rvField, recipientVariables and this.debug in
fake-mailgun-server.ts) does not forward unexpanded placeholders.
🪄 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: de2e7a51-1b18-46e3-ac3f-decca42d62b0
📒 Files selected for processing (1)
e2e/helpers/services/mailgun/fake-mailgun-server.ts
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
mailgunEnabledfixture to configure Ghost'sbulkEmail.mailgun.*settings to point at the fake serveremailClientfixture providing a shared MailPit client to any testPublishFlowpage object withselectPublishType()andconfirm()methodsContext
Bulk newsletter sends go through the Mailgun API, bypassing SMTP/MailPit entirely. The root e2e framework runs Ghost in Docker containers where in-process mocking (
sinon.stub) isn't possible. This fake server bridges that gap, enabling migration of the remainingpublishing.spec.jsbrowser tests.Test plan
yarn test tests/admin/posts/newsletter-send.test.tspassesyarn lintclean (0 errors)yarn test:typesclean