Conversation
no ref We'd like to build out Stripe mocks in the /e2e/ package in order to move all existing browser tests into our new /e2e/ testing suite. This is the first of several slices in an attempt to break up all of the pieces of Stripe functionality we need.
WalkthroughThis PR introduces a comprehensive Stripe testing framework for end-to-end tests. It replaces the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
e2e/tests/admin/members/stripe-webhooks.test.ts (1)
7-7: Consider removing the inline comment.The test name
'member created via webhooks - has paid status'already conveys the scenario clearly. As per coding guidelines, prefer less comments and giving things clear names in E2E tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/stripe-webhooks.test.ts` at line 7, Remove the inline comment "// Ghost creates a member from Stripe checkout webhooks; this tests the post-Portal redirect flow" and rely on the existing test name 'member created via webhooks - has paid status' in the test block to convey intent; if you feel clarity is needed, instead of an inline comment rename the test declaration (the string passed to the test or it(...) call) to a slightly more explicit form and delete the comment.e2e/helpers/services/stripe/stripe-service.ts (1)
39-40: Thecustomer.subscriptions.data.push(subscription)on line 40 is redundant.The
MockStripeServer.handleRequestforGET /v1/customers/:iddynamically rebuilds the subscriptions list from its subscriptions map (mock-stripe-server.ts lines 104–111), so the embeddedsubscriptions.dataon the stored customer object is never read. This push keeps the local object consistent but has no effect on mock server responses.Not blocking — just a note for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/stripe/stripe-service.ts` around lines 39 - 40, Remove the redundant push to the stored customer's embedded subscriptions array: in the function that adds a subscription to a customer (where customer.subscriptions.data.push(subscription) is called), delete that line because MockStripeServer.handleRequest for GET /v1/customers/:id rebuilds subscriptions from the server's subscriptions map (see mock-stripe-server.ts logic), so updating customer.subscriptions.data has no effect on responses; simply ensure the subscription is stored in the server's subscriptions map and return/acknowledge without mutating customer.subscriptions.data.e2e/helpers/services/stripe/mock-stripe-server.ts (1)
183-186: Silent 200 fallback may mask unimplemented endpoints.Returning
200for every unhandled route means Ghost API calls to unimplemented Stripe endpoints (e.g., webhook endpoint CRUD, price/product lookups) will silently succeed with a nonsensical response. This could hide real bugs in the test flow. Consider returning501or logging a warning-level message to make unexpected calls more visible during test debugging.Proposed: return 501 for unhandled routes
// Fallback: return 200 with empty object for unhandled routes - debug(`Unhandled route: ${method} ${url} — returning fallback`); - res.writeHead(200, {'Content-Type': 'application/json'}); - res.end(JSON.stringify({id: 'fake', object: 'unknown'})); + debug(`Unhandled route: ${method} ${url} — returning 501`); + res.writeHead(501, {'Content-Type': 'application/json'}); + res.end(JSON.stringify({error: {type: 'api_error', message: `Mock not implemented: ${method} ${url}`}}));Note: if this causes tests to fail due to Ghost making expected calls to unimplemented endpoints, those endpoints should be added to the mock rather than silently swallowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/stripe/mock-stripe-server.ts` around lines 183 - 186, The current fallback returns a silent 200 for unhandled routes, which can mask missing mock endpoints; update the fallback so that instead of res.writeHead(200, ...) it returns a 501 Not Implemented (e.g., res.writeHead(501, {'Content-Type':'application/json'})) and include a warning-level log (using debug or a warning logger) that contains method and url and a short message indicating the route is unimplemented; ensure the JSON body conveys the error (e.g., {error:'unimplemented', method, url}) so tests fail visibly if Ghost hits an unmocked Stripe endpoint and add proper mock handlers for any legitimately expected calls.e2e/helpers/playwright/fixture.ts (1)
99-115: Missing try/finally leavesMockStripeServerrunning if setup throws beforeuse()If any line between
server.start()andawait use(service)throws (however unlikely),server.stop()is never called because teardown only runs for code after a successfulawait use().♻️ Proposed fix: wrap server lifecycle in try/finally
stripe: async ({stripeEnabled, baseURL}, use) => { if (!stripeEnabled || !baseURL) { await use(undefined); return; } const server = new MockStripeServer(STRIPE_MOCK_PORT); await server.start(); debug('Mock Stripe server started on port', STRIPE_MOCK_PORT); + try { const webhookClient = new WebhookClient(baseURL); const service = new StripeTestService(server, webhookClient); await use(service); + } finally { await server.stop(); debug('Mock Stripe server stopped'); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/playwright/fixture.ts` around lines 99 - 115, The stripe fixture can leak MockStripeServer if an exception occurs between server.start() and await use(service); wrap the server lifecycle in a try/finally so server.stop() always runs: after creating and starting the MockStripeServer(STRIPE_MOCK_PORT) call server.start(), then use a try { await use(service); } finally { await server.stop(); } (ensure debug('Mock Stripe server stopped') stays in the finally) to guarantee teardown even when setup throws.
🤖 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/playwright/fixture.ts`:
- Around line 99-115: The stripe fixture can leak MockStripeServer if an
exception occurs between server.start() and await use(service); wrap the server
lifecycle in a try/finally so server.stop() always runs: after creating and
starting the MockStripeServer(STRIPE_MOCK_PORT) call server.start(), then use a
try { await use(service); } finally { await server.stop(); } (ensure debug('Mock
Stripe server stopped') stays in the finally) to guarantee teardown even when
setup throws.
In `@e2e/helpers/services/stripe/mock-stripe-server.ts`:
- Around line 183-186: The current fallback returns a silent 200 for unhandled
routes, which can mask missing mock endpoints; update the fallback so that
instead of res.writeHead(200, ...) it returns a 501 Not Implemented (e.g.,
res.writeHead(501, {'Content-Type':'application/json'})) and include a
warning-level log (using debug or a warning logger) that contains method and url
and a short message indicating the route is unimplemented; ensure the JSON body
conveys the error (e.g., {error:'unimplemented', method, url}) so tests fail
visibly if Ghost hits an unmocked Stripe endpoint and add proper mock handlers
for any legitimately expected calls.
In `@e2e/helpers/services/stripe/stripe-service.ts`:
- Around line 39-40: Remove the redundant push to the stored customer's embedded
subscriptions array: in the function that adds a subscription to a customer
(where customer.subscriptions.data.push(subscription) is called), delete that
line because MockStripeServer.handleRequest for GET /v1/customers/:id rebuilds
subscriptions from the server's subscriptions map (see mock-stripe-server.ts
logic), so updating customer.subscriptions.data has no effect on responses;
simply ensure the subscription is stored in the server's subscriptions map and
return/acknowledge without mutating customer.subscriptions.data.
In `@e2e/tests/admin/members/stripe-webhooks.test.ts`:
- Line 7: Remove the inline comment "// Ghost creates a member from Stripe
checkout webhooks; this tests the post-Portal redirect flow" and rely on the
existing test name 'member created via webhooks - has paid status' in the test
block to convey intent; if you feel clarity is needed, instead of an inline
comment rename the test declaration (the string passed to the test or it(...)
call) to a slightly more explicit form and delete the comment.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 22192081554 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
| STRIPE_API_PORT: String(STRIPE_MOCK_PORT), | ||
| STRIPE_API_PROTOCOL: 'http' | ||
| } : {}; | ||
| const mergedConfig = {...(config || {}), ...stripeConfig}; |
There was a problem hiding this comment.
thought: Maybe stripeEnabled should be part of the config? Not sure.
There was a problem hiding this comment.
Stripe config is determined via the presence of the sk_ and pk_ keys. This handling has been updated so I think it's much more clear now as that data is able to be seeded in the db.
| @@ -0,0 +1,182 @@ | |||
| import crypto from 'crypto'; | |||
There was a problem hiding this comment.
suggestion: this file feels like it reinvents our Factory concept, which I think we should reuse here - e.g. StripeCustomerFactory, StripePriceFactory, etc.
There was a problem hiding this comment.
I would like more discussion on this. I think we could rename them to factory-style names but I don't want them to live in the /data-factory dir nor do I want them to be confused with the function of those factories.
Services are distinct and I think there's limited use case (if any?) where we'd want to create Stripe data in a test that isn't via Ghost. Conceptually I think that's more where I draw the line.
e2e/helpers/playwright/fixture.ts
Outdated
| stripe: async ({stripeEnabled, baseURL}, use) => { | ||
| if (!stripeEnabled || !baseURL) { | ||
| await use(undefined); | ||
| return; | ||
| } | ||
|
|
||
| const server = new MockStripeServer(STRIPE_MOCK_PORT); | ||
| await server.start(); | ||
| debug('Mock Stripe server started on port', STRIPE_MOCK_PORT); | ||
|
|
||
| const webhookClient = new WebhookClient(baseURL); | ||
| const service = new StripeTestService(server, webhookClient); | ||
| await use(service); | ||
|
|
||
| await server.stop(); | ||
| debug('Mock Stripe server stopped'); | ||
| }, |
There was a problem hiding this comment.
I'm not sure, but this may need to come after the baseURL fixture below, which is where it gets set properly for each Ghost instance.
There was a problem hiding this comment.
Dependencies are not based on where it appears in the file, if that's what you mean. I think we're ok here, though if we want to order it such that it feels progressive, I'm good w/ that.
|
One higher level thought/suggestion: this creates a new instance of the stripe server for each instance of Ghost, so we can pass the Ghost URL to the stripe server for webhooks. Did you consider creating a single persistent instance that would live for the lifetime of the test run? Maybe not worth the additional complexity, but we could conceivably handle the webhook URL mapping within the StripeTestService via the Stripe API keys or something along those lines. No strong opinions on which way is better, just curious if you explored this at all |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/helpers/playwright/fixture.ts (1)
177-181:needsReloadis a one-use alias forlabsFlagsSpecified— minor readability nit.♻️ Suggested simplification
- const needsReload = labsFlagsSpecified; - if (needsReload) { + if (labsFlagsSpecified) { await page.reload({waitUntil: 'load'}); debug('Settings applied and page reloaded'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/playwright/fixture.ts` around lines 177 - 181, The variable needsReload is an unnecessary one-use alias for labsFlagsSpecified; simplify by removing needsReload and using labsFlagsSpecified directly in the conditional around page.reload and debug. Locate the block that declares needsReload and the subsequent if (needsReload) { await page.reload({waitUntil: 'load'}); debug('Settings applied and page reloaded'); } and replace it so the if uses labsFlagsSpecified directly (i.e., if (labsFlagsSpecified) { await page.reload({waitUntil: 'load'}); debug('Settings applied and page reloaded'); }), removing the now-unused needsReload binding and any related dead code/imports.e2e/helpers/services/stripe/fake-stripe-server.ts (2)
9-10: Missingexpress.json()body-parser middleware.The Express app has no JSON body-parser, so
req.bodywill beundefinedfor any POST route that accesses request body. The currentPOST /v1/billing_portal/configurationsdoesn't read the body, but this will silently break the first time a new route needs it (e.g., subscription update, payment intent creation).♻️ Suggested fix
constructor(port: number) { this._port = port; + this.app.use(express.json()); this.setupRoutes(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/stripe/fake-stripe-server.ts` around lines 9 - 10, Add Express's JSON body-parsing middleware to the fake Stripe server so POST handlers receive parsed req.body: initialize the middleware on the app instance (the private readonly app = express())—e.g., call app.use(express.json()) early (constructor or before route definitions in fake-stripe-server.ts) so handlers like the POST /v1/billing_portal/configurations and any future POST routes can access req.body correctly.
46-57:stop()may stall if Ghost has open HTTP keep-alive connections.
server.close()stops accepting new connections but waits for existing ones to drain. Ghost (running in Docker) may hold keep-alive connections to the fake server open, causingstop()to block indefinitely during test teardown. Node.js 18.2+ providesserver.closeAllConnections()to force-close them.♻️ Suggested fix
async stop(): Promise<void> { return new Promise((resolve) => { if (!this.server) { resolve(); return; } + this.server.closeAllConnections(); this.server.close(() => { this.server = null; resolve(); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/stripe/fake-stripe-server.ts` around lines 46 - 57, The stop() promise can hang because existing keep-alive connections are not terminated; update the stop implementation to first force-close any open connections by calling server.closeAllConnections() when available (check (this.server as any).closeAllConnections or similar), then call this.server.close(...) as before and resolve in the callback; ensure you still handle the case where this.server is null and keep current nulling of this.server after close so stop() always resolves even if closeAllConnections is used.e2e/helpers/services/stripe/stripe-service.ts (1)
35-36:customer.subscriptions.data.push(subscription)has no observable effect and is misleading.
FakeStripeServer.GET /v1/customers/:idbuilds the subscriptions list dynamically from its ownthis.subscriptionsmap (filtering bycustomer === customerId), ignoring thesubscriptionsfield of the stored customer object. ThecheckoutEventalso only carriescustomerId, not the full customer. The push mutates a local object that's never read by any consumer in this flow.♻️ Suggested cleanup
- // Add subscription to customer - customer.subscriptions.data.push(subscription); - // Seed server so Ghost can look up all objects this.server.upsertCustomer(customer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/services/stripe/stripe-service.ts` around lines 35 - 36, The line customer.subscriptions.data.push(subscription) is a no-op because FakeStripeServer.GET /v1/customers/:id builds subscriptions from this.subscriptions and checkoutEvent only has customerId; remove that push to avoid misleading mutation, or if the intent was to persist the new subscription for later GETs, add the subscription into the server's this.subscriptions store (e.g., append to the array/map entry keyed by customerId) instead of mutating the local customer object; update the handler around where the subscription is created so subscribers are taken from this.subscriptions consistently.e2e/helpers/environment/service-managers/mysql-manager.ts (1)
184-190: Escape only covers';",\, and;in key values would break the shell command.The constructed command is wrapped in double quotes (
-e "..."), so a"in a key value would terminate the shell argument prematurely. Similarly, unescaped\sequences are interpreted by MySQL's string parser inside single-quoted literals.In practice, the two hardcoded test keys (
sk_test_e2eTestKey,pk_test_e2eTestKey) contain no special characters, so this is low risk today. Consider adding a note or using a multi-value prepared-statement style if the method becomes more general-purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/environment/service-managers/mysql-manager.ts` around lines 184 - 190, The current command construction in mysql-manager.ts builds a shell-quoted -e "..." string and only escapes single quotes, so double quotes, backslashes, and semicolons in the key values can break the shell or MySQL literal parsing; update the code that sets escapedSecretKey, escapedPublishableKey and command (references: escapedSecretKey, escapedPublishableKey, command) to either (preferred) avoid shell interpolation entirely by running the UPDATE via a MySQL client library/parameterized query, or (minimal fix) additionally escape double quotes (") and backslashes (\) and semicolons (;) before embedding into the double-quoted -e argument and ensure MySQL single-quoted literals are safe (e.g., escape backslashes for MySQL). Make sure the final approach prevents shell injection and preserves correct MySQL string quoting.
🤖 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/playwright/fixture.ts`:
- Around line 12-13: Gitleaks flags the STRIPE_SECRET_KEY constant value because
it matches the `stripe-access-token` rule; add an inline suppression comment
immediately next to the STRIPE_SECRET_KEY declaration (the constant named
STRIPE_SECRET_KEY in e2e/helpers/playwright/fixture.ts) to mark it as an
intentional fake key and silence the false positive—e.g., add the repository’s
accepted Gitleaks suppression pragma/comment format (not a blanket ignore)
referencing the rule or a local-only exemption so the constant remains unchanged
but CI no longer reports it.
---
Nitpick comments:
In `@e2e/helpers/environment/service-managers/mysql-manager.ts`:
- Around line 184-190: The current command construction in mysql-manager.ts
builds a shell-quoted -e "..." string and only escapes single quotes, so double
quotes, backslashes, and semicolons in the key values can break the shell or
MySQL literal parsing; update the code that sets escapedSecretKey,
escapedPublishableKey and command (references: escapedSecretKey,
escapedPublishableKey, command) to either (preferred) avoid shell interpolation
entirely by running the UPDATE via a MySQL client library/parameterized query,
or (minimal fix) additionally escape double quotes (") and backslashes (\) and
semicolons (;) before embedding into the double-quoted -e argument and ensure
MySQL single-quoted literals are safe (e.g., escape backslashes for MySQL). Make
sure the final approach prevents shell injection and preserves correct MySQL
string quoting.
In `@e2e/helpers/playwright/fixture.ts`:
- Around line 177-181: The variable needsReload is an unnecessary one-use alias
for labsFlagsSpecified; simplify by removing needsReload and using
labsFlagsSpecified directly in the conditional around page.reload and debug.
Locate the block that declares needsReload and the subsequent if (needsReload) {
await page.reload({waitUntil: 'load'}); debug('Settings applied and page
reloaded'); } and replace it so the if uses labsFlagsSpecified directly (i.e.,
if (labsFlagsSpecified) { await page.reload({waitUntil: 'load'});
debug('Settings applied and page reloaded'); }), removing the now-unused
needsReload binding and any related dead code/imports.
In `@e2e/helpers/services/stripe/fake-stripe-server.ts`:
- Around line 9-10: Add Express's JSON body-parsing middleware to the fake
Stripe server so POST handlers receive parsed req.body: initialize the
middleware on the app instance (the private readonly app = express())—e.g., call
app.use(express.json()) early (constructor or before route definitions in
fake-stripe-server.ts) so handlers like the POST
/v1/billing_portal/configurations and any future POST routes can access req.body
correctly.
- Around line 46-57: The stop() promise can hang because existing keep-alive
connections are not terminated; update the stop implementation to first
force-close any open connections by calling server.closeAllConnections() when
available (check (this.server as any).closeAllConnections or similar), then call
this.server.close(...) as before and resolve in the callback; ensure you still
handle the case where this.server is null and keep current nulling of
this.server after close so stop() always resolves even if closeAllConnections is
used.
In `@e2e/helpers/services/stripe/stripe-service.ts`:
- Around line 35-36: The line customer.subscriptions.data.push(subscription) is
a no-op because FakeStripeServer.GET /v1/customers/:id builds subscriptions from
this.subscriptions and checkoutEvent only has customerId; remove that push to
avoid misleading mutation, or if the intent was to persist the new subscription
for later GETs, add the subscription into the server's this.subscriptions store
(e.g., append to the array/map entry keyed by customerId) instead of mutating
the local customer object; update the handler around where the subscription is
created so subscribers are taken from this.subscriptions consistently.
| const STRIPE_SECRET_KEY = 'sk_test_e2eTestKey'; | ||
| const STRIPE_PUBLISHABLE_KEY = 'pk_test_e2eTestKey'; |
There was a problem hiding this comment.
Gitleaks flags sk_test_e2eTestKey as a live Stripe secret key — add a suppression comment.
The key is intentionally fake, but its sk_test_ prefix matches Gitleaks' Stripe access-token rule (stripe-access-token), which will keep triggering [high] alerts in CI on every scan. Add an inline suppression to silence the false positive without granting a blanket override.
🛡️ Suggested fix
-const STRIPE_SECRET_KEY = 'sk_test_e2eTestKey';
-const STRIPE_PUBLISHABLE_KEY = 'pk_test_e2eTestKey';
+const STRIPE_SECRET_KEY = 'sk_test_e2eTestKey'; // gitleaks:allow - intentionally fake test key
+const STRIPE_PUBLISHABLE_KEY = 'pk_test_e2eTestKey'; // gitleaks:allow - intentionally fake test key🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 12-12: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/playwright/fixture.ts` around lines 12 - 13, Gitleaks flags the
STRIPE_SECRET_KEY constant value because it matches the `stripe-access-token`
rule; add an inline suppression comment immediately next to the
STRIPE_SECRET_KEY declaration (the constant named STRIPE_SECRET_KEY in
e2e/helpers/playwright/fixture.ts) to mark it as an intentional fake key and
silence the false positive—e.g., add the repository’s accepted Gitleaks
suppression pragma/comment format (not a blanket ignore) referencing the rule or
a local-only exemption so the constant remains unchanged but CI no longer
reports it.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 22227217870 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
| import crypto from 'crypto'; | ||
| import type {StripeEvent} from './builders'; | ||
|
|
||
| const DEFAULT_WEBHOOK_SECRET = 'DEFAULT_WEBHOOK_SECRET'; |
There was a problem hiding this comment.
should this be process.env.WEBHOOK_SECRET || 'DEFAULT_WEBHOOK_SECRET' like it is in config.js?
from what I/agents can tell it's at least possible to end up with a mismatch depending on how your environment is set up
4 participants
no ref
We'd like to build out Stripe mocks in the /e2e/ package in order to move all existing browser tests into our new /e2e/ testing suite. This is the first of several slices in an attempt to break up all of the pieces of Stripe functionality we need.
The first test here is creating a Ghost member on receiving the checkout webhook, which is effectively the back end of the Portal workflow where Portal -> Stripe -> Ghost.
The Stripe server is stateful, unlike the other options out there for Stripe mocking/testing, meaning we are stuck rolling our own.