Cleaned up ghost/core db (integration) test suite noise#28909
Merged
Conversation
Contributor
WalkthroughThe PR adds environment guards to the explore-ping and Stripe startup services so they skip side effects outside allowed environments or during tests. It adds a config option that changes 4xx request logging between error and warning, updates the middleware and its unit tests, switches several admin and email-related tests to expect warning logs, and adds test helper support for restoring image-size lookups. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:integration |
✅ Succeeded | 2m 47s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 3s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 13s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 22s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 5s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 3s | View ↗ |
nx run ghost:build:assets |
✅ Succeeded | 2s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-03 18:10:44 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/test/integration/services/email-service/batch-sending.test.js (1)
188-218: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConcurrent-send test stubs
logging.errorbut never restores it.Unlike the retry test (which explicitly calls
errorLog.restore()at Line 496), this block creates the stub at Line 191 with no matching restore. If the suite does not have a globalafterEach(() => sinon.restore()), this stub leaks past the test and silently swallows alllogging.erroroutput for every subsequent test in the run — which could mask real failures and undermines the very goal of this PR.Please confirm whether a global Sinon restore hook exists in this file. If it does, this is fine and you can disregard; if not, add an explicit restore.
#!/bin/bash # Look for global sinon restore/sandbox teardown in the test file and its shared helpers fd -t f 'batch-sending.test.js' ghost/core/test | while read -r f; do echo "== $f ==" rg -nP 'afterEach|beforeEach|sinon\.(restore|createSandbox)|\.restore\s*\(' "$f" done # Also check shared test setup that might auto-restore rg -nP 'afterEach|sinon\.(restore|createSandbox)' ghost/core/test/utils -g '*.js' -C2If no auto-restore is present, scope the stub like the retry test does:
🔒 Suggested restore for the concurrent-send stub
for (const call of errorLog.getCalls()) { assert.match(call.args[0], /Tried sending email that is not pending or failed/); } + errorLog.restore();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/integration/services/email-service/batch-sending.test.js` around lines 188 - 218, The concurrent-send test stubs logging.error via sinon.stub but never restores it, so scope cleanup is needed unless a shared afterEach restore already exists in batch-sending.test.js or its helpers. Add an explicit restore for the errorLog stub in the same test block, matching the pattern used by the retryEmail test with errorLog.restore(), so the stub does not leak into later tests and suppress logging.error globally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ghost/core/test/integration/services/email-service/batch-sending.test.js`:
- Around line 188-218: The concurrent-send test stubs logging.error via
sinon.stub but never restores it, so scope cleanup is needed unless a shared
afterEach restore already exists in batch-sending.test.js or its helpers. Add an
explicit restore for the errorLog stub in the same test block, matching the
pattern used by the retryEmail test with errorLog.restore(), so the stub does
not leak into later tests and suppress logging.error globally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdff0f0f-4aaf-4e9a-9c2f-10c03f695536
📒 Files selected for processing (3)
ghost/core/core/server/services/explore-ping/index.jsghost/core/core/server/services/stripe/stripe-service.jsghost/core/test/integration/services/email-service/batch-sending.test.js
9larsons
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/web/parent/middleware/log-request.test.js (1)
43-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage of the new branching. Consider optionally adding a case for a request with no
req.err(thelogging.infoelse-branch) to pin that contract too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/server/web/parent/middleware/log-request.test.js` around lines 43 - 78, The log-request middleware tests cover the status-based branches, but they do not explicitly verify the no-error path. Add a test around the log-request middleware’s main run path to assert that when req.err is absent, the fallback branch uses logging.info, keeping the existing status-code cases for logging.error and logging.warn intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/test/unit/server/web/parent/middleware/log-request.test.js`:
- Around line 43-78: The log-request middleware tests cover the status-based
branches, but they do not explicitly verify the no-error path. Add a test around
the log-request middleware’s main run path to assert that when req.err is
absent, the fallback branch uses logging.info, keeping the existing status-code
cases for logging.error and logging.warn intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72698421-d64e-4d27-b9d6-0302fc5afd0d
📒 Files selected for processing (15)
ghost/core/core/server/web/parent/middleware/log-request.jsghost/core/core/shared/config/defaults.jsonghost/core/core/shared/config/env/config.testing-mysql.jsonghost/core/core/shared/config/env/config.testing.jsonghost/core/test/e2e-api/admin/activity-feed.test.jsghost/core/test/e2e-api/admin/custom-theme-settings.test.jsghost/core/test/e2e-api/admin/email-previews.test.jsghost/core/test/e2e-api/admin/images.test.jsghost/core/test/e2e-api/admin/key-authentication.test.jsghost/core/test/e2e-api/admin/labels.test.jsghost/core/test/e2e-api/admin/media.test.jsghost/core/test/e2e-api/admin/members.test.jsghost/core/test/e2e-api/admin/posts-legacy.test.jsghost/core/test/e2e-api/admin/settings.test.jsghost/core/test/unit/server/web/parent/middleware/log-request.test.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/e2e-api/admin/media.test.js
9larsons
changed the title
no ref - the DB-backed test suites logged hundreds of expected errors per run, burying real failures in stdout - stripe-service: webhookManager/billingPortalManager.start() hit Stripe endpoints the test mock doesn't implement (webhook_endpoints, billing_portal/configurations) -> 500 -> error-logged on every boot; skip both under testEnv (tests dispatch webhook events directly via the mocker; prod/dev unchanged) — ~266 fewer lines/run - batch-sending.test.js: two tests deliberately trip the "not pending or failed" guards; stub logging.error and assert the expected message instead of printing — 206 -> 2 lines - explore-ping: gate the background phone-home ping to dev/prod only (matching update-check) so it doesn't run in tests
no ref - the DB test suites deliberately exercise hundreds of 4xx responses (validation/auth/rate-limit, asserted via expectStatus); the request logger logs every non-404 error at error level, so each produced a redundant error line — ~300 per run - added a logging:logClientErrorsAsError flag (default true; false in the testing configs) so the request logger demotes 4xx client errors to warn in the test env only — production and dev keep 4xx at error (monitored in Elastic) - moved the logging.error stubs to warn in the 27 tests that capture+assert their 4xx request log - added a log-request unit test pinning the contract (5xx/no-status -> error; 4xx -> error by default, warn when configured; 404 -> info)
no ref - frontend/email meta rendering fetches external image dimensions via the image lib cache; those images are nock-blocked in tests, so every render logged "Unknown Request error" (~250 lines per run) - disableNetwork() now captures the real cachedImageSizeFromUrl.getImageSizeFromUrl once and swaps in a no-op (dimensions omitted — same outcome as the blocked fetch, minus the error); allowImageSize() restores it - cards.test.js opts back in for the two tests that exercise the lookup itself
no ref - #27822 (a1ba8ea, on main after this branch was first pushed) split the single "Tried sending email batch that is not pending or failed" error into two paths: already-submitted batches now log info ("already submitted on a prior run; skipping"), only stuck submitting orphans still error - the retry test for "One failed batch marks the email as failed" still stubbed logging.error and asserted the old message, so the assertion failed on the GHA merge commit even though the branch passed locally - repoint the stub at logging.info with the new message text; same scoped pattern (stub for the retry call only, restore before assert)
9larsons
force-pushed
the
logging-discipline-db
branch
from
b3d0f23 to
3148b38
Compare
- offers.test.js and newsletters.test.js still stubbed logging.error around 4xx assertions, so the stub never intercepted the warn-level call log-request.js now makes for client errors in test env, and the real warn line printed to stdout on every run - the image-size cache mock's capture guard compared against the noop function to avoid re-capturing it, but the guard is redundant: the first branch (!realCachedImageSizeFromUrl) already prevents re-entry before the noop is ever installed
- getAddress() error-logged an invalid replyTo on every send whose replyTo happened to equal the default from-address (e.g. noreply@127.0.0.1 on an IP-only/test install) — validate() already special-cases this exact address as expected-but-invalid, getAddress() didn't. This fired on most email-related tests and would do the same for any self-hoster running Ghost without a real domain. - email-event-storage.test.js's 'Ignores invalid events' and resume-interrupted-sends.test.js's orphan-batch test deliberately trigger logging.error paths but never stubbed the logger, so both still printed on every run - batch-sending.test.js's 'One failed batch...' test simulates a Mailgun 500 without stubbing the logger either, for the same reason
…t noise - sentry.js's disabled-Sentry stub only exported captureException, not captureMessage. batch-sending-service.js calls captureMessage when a sent email's recipient count doesn't match what was stored, so any Ghost instance without Sentry configured (the common case) hit a TypeError there instead of the intended no-op — masking the real underlying signal with a crash. - fixture/example post content (fixtures.json, golden-post.json) links to real ghost.org subdomains. Publishing that content in tests triggers real webmention discovery, which fetches every external link; nock blocks it, so mention-discovery-service.js error-logged on every publish. Mock the known ghost.org subdomains to return a plain 200 page (no webmention link) instead of blocking the connection — same 'no endpoint found' outcome, without eating a connection error. Note: fixing the captureMessage crash surfaced a separate, pre-existing issue in domain-warming.test.js (2 tests now fail where they previously passed by accident, since the crash prevented the corrected recipient count from ever being saved). That needs its own investigation into why createBatches() finds 0 recipients on some multi-day warmup scenarios — tracked separately, not fixed here.
…mismatches - getAddress() logged at error level whenever a sender's from-address didn't match the configured sending domain, but that's the designed, routine fallback path for managed-email customers (any staff/member/ newsletter sender not on the apex sending domain) — demoted to warn and reworded, matching the earlier replyTo fix in the same file. - welcome-email-automation-poll.test.js's two deliberate send/DB-failure tests, gift-links.test.ts's action-recording-failure test, and offer-bookshelf-repository.test.js's missing-tier test all exercise real, already-handled catch blocks that log and continue — none stubbed the logger, so all four printed on every run. Stubbed and asserted the log fired, matching the stub-and-assert pattern already used elsewhere in the suite for this class of test.
baseDate anchored to 'today at 12:00 UTC', computed once at file load. createMembers() generates member ObjectIds under the real clock before setDay() engages sinon.useFakeTimers (which fakes the global Date that bson-objectid reads). Whenever the suite ran after 12:00 UTC, day 0's faked 'now' landed before the real moment members were created, so batch-sending-service.js's deliberate id-based recipient cutoff (only include members created before the email — createBatches()'s comment explains this guards against imported members gaming warmup limits via manipulated created_at) excluded every member. Anchor to the next UTC midnight instead, which is always safely after any real timestamp from the same run regardless of what time of day the suite executes. This was previously masked by the Sentry captureMessage crash (see prior commit): the crash aborted createBatches() before the corrected (zero) recipient count was ever saved, so the stale pre-crash value coincidentally satisfied the test's assertions while sending silently failed underneath.
…ry mock
- connection.js's better-sqlite3 pool defaults to up to 10 connections,
but SQLite only ever allows one writer at a time. Without a
busy_timeout, a second connection's write while another is in
progress fails immediately with SQLITE_BUSY ('database is locked')
instead of waiting. Set busy_timeout so it retries internally.
Tried restricting the pool to a single connection first (min:1,
max:1) as a more thorough fix, but that caused a cascading test-suite
failure under load (something in the codebase genuinely needs 2+
concurrent connections, e.g. a transaction plus a concurrent query) —
reverted that in favor of the safer, bounded-wait busy_timeout alone.
Verified clean across 6+ consecutive full integration-suite runs and
a full unit-suite run (previously intermittently flaky both ways).
- extended the webmention-discovery domain mock (added for ghost.org
subdomains previously) to also cover example.com/www.example.com,
the RFC 2606 reserved placeholder domain used as a stand-in external
link across many individual tests — same 'no endpoint found' outcome
instead of a nock-blocked connection error on every publish.
9larsons
changed the title
- image-size no-op broke LOCAL storage-path image lookups too, not just the nock-blocked external fetches it targeted: cachedImageSizeFromUrl's getImageSizeFromUrl already branches on isLocalImage internally, and the no-op replaced that whole function, silently dropping dimensions from every locally-hosted test image. Now only no-ops non-local URLs; local paths delegate to the real (network-free) lookup. - batch-sending.test.js's concurrent-retry test asserted every captured logging.error call matched one specific guard message. With 50 concurrent retries and maxRetries: 0 in test config, an unrelated transient error elsewhere in the same window would log an Error object instead of a string, and assert.match would throw a TypeError instead of failing the assertion cleanly. Filter for the expected message instead, matching the skipLogs pattern already used later in the same file. - stripe-service.js gated both webhookManager.start() and billingPortalManager.start() behind !config.testEnv, with a comment claiming both hit Stripe endpoints the mock doesn't implement. Only the billing-portal call actually did — webhookManager.start() already self-guards via its own mode !== 'network' check, which is always true outside production (config.js defaults webhookSecret to a placeholder). Narrowed the gate to just billingPortalManager.start() and corrected the comment. - log-request.test.js stubbed config.get wholesale instead of using the configUtils.set/restore convention every sibling web-middleware unit test uses, so the real defaults.json default was never exercised. Converted to configUtils.
…three env-detection idioms
stripe/service.js, explore-ping/index.js, and update-check/index.js each
had their own way of asking 'am I in test/dev/prod' — a Stripe-scoped
process.env.NODE_ENV.startsWith('test'), a hardcoded env array checked
against config.get('env'), and (in update-check) the same array checked
against raw process.env.NODE_ENV instead. The last one is why
explore-ping's comment claiming parity with update-check was wrong:
config.get('env') defaults to 'development' when NODE_ENV is unset,
process.env.NODE_ENV does not, so the two diverged on an unset NODE_ENV.
Added isTestEnv()/isProductionOrDevelopment() to shared/config/helpers.js,
following the same bind-to-nconf pattern already used there for
getContentPath/isPrivacyDisabled/etc, and pointed all three call sites
at it. update-check now shares config's normal env-defaulting semantics
instead of its own raw-NODE_ENV special case.
…t, other review fixes - The webmention-discovery domain mock lived inside disableNetwork(), which the unit-test lane's vitest-setup.ts also calls directly in every beforeAll/afterEach — so the persist()'d ghost.org/example.com interceptors shadowed unit tests' own nock mocks against those exact hosts (nock 14 matches first-registered, persisted interceptors are never consumed). Broke 9 tests in oembed-service.test.js and external-media-inliner.test.js. Extracted into its own mockWebmentionDiscoveryDomains(), called only from the DB lane (vitest-setup-db.ts's beforeAll/afterEach and mockManager.restore()), guarded by a flag reset on nock.cleanAll() so it doesn't re-stack a fresh set of persisted interceptors on every single test. - connection.js's busy_timeout pragma was a genuine no-op: better-sqlite3 already defaults its own option to 5000ms internally (verified in its Database constructor), independent of any pragma. Reverted; whatever reduced the SQLITE_BUSY rate I observed was just run-to-run variance on an already-rare, intermittent issue — the comment's explanation was wrong and would've misled the next debugger. - config.isProductionOrDevelopment() defaults to true when NODE_ENV is unset (env defaults to 'development'), changing update-check's prior behavior of bailing on a literally-unset NODE_ENV. Documented why this is low-impact: ghost.js (the real CLI entry point) already sets process.env.NODE_ENV = process.env.NODE_ENV || 'development' before anything else loads, so the two only diverge for non-standard embedders that require core modules directly. - email-address-service.ts's replyTo carve-out compared byte-exact, so a case variant of the default from-address (user-entered via members_support_address) defeated it and kept erroring, while any other malformed mail:from silently lost all log signal. Lowercased the comparison and demoted to warn instead of fully silencing. - gift-links.test.ts and offer-bookshelf-repository.test.js called errorLog.restore() after the assertion it was guarding, so a failing assertion left logging.error stubbed for the rest of the file (and, in offer-bookshelf's case, the rest of its ~15 tests). Moved cleanup into each file's afterEach. - domain-warming.test.js's day-0 anchor was only 1 day past file-load time; a load landing right at a UTC midnight boundary left a few- second window where createMembers() could still tick past it. Anchored 2 days out instead.
The module-level webmentionDomainsMocked flag was only reset in restore(), but five DB-lane files (webhook-request, themes, webmentions, milestones, mentions) call nock.cleanAll() directly, bypassing restore(). In that path the interceptors were wiped but the flag stayed true, so the per-test re-registration in vitest-setup-db.ts's afterEach silently no-op'd — exactly the case that afterEach exists to cover — and the webmention mocks stayed gone for the rest of that worker until some other file happened to call restore(). Replaced the flag with a check against nock.activeMocks() so the guard reflects nock's actual live state instead of a boolean that can drift out of sync with it.
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.
Removes the bulk of the error-level log noise from the DB-backed test suites (integration / e2e), where deliberate error paths were logging hundreds of lines per run and burying real failures.
Stripe boot registration —
configure()callswebhookManager.start()+billingPortalManager.start()on every boot, both hitting Stripe endpoints the test mock doesn't implement (webhook_endpoints,billing_portal/configurations) → 500 → error-logged. Tests dispatch webhook events directly through the mocker and never use a registered endpoint or portal configuration, so both are skipped undertestEnv; prod and dev register exactly as before.4xx request-error logging — the suites deliberately exercise hundreds of 4xx responses (validation/auth/rate-limit, asserted via
expectStatus), and the request logger logs every non-404 error at error level. Added alogging:logClientErrorsAsErrorflag (defaulttrue;falsein the testing configs) so 4xx client errors demote towarnin the test env only — prod and dev keep 4xx aterror(Elastic monitoring untouched). A newlog-requestunit test pins the contract (5xx → error;4xx → errorby default /warnwhen configured;404 → info).image-size fetch — frontend/email meta rendering fetches external image dimensions via the image lib cache; those images are nock-blocked in tests, so every render logged "Unknown Request error."
disableNetwork()now captures the realcachedImageSizeFromUrl.getImageSizeFromUrlonce and swaps in a no-op (dimensions omitted — the same outcome as the blocked fetch, minus the error); a couple of tests opt back in viaallowImageSize()for the two that exercise the lookup itself.webmention discovery — publishing fixture/example post content (which links to real
ghost.orgsubdomains andexample.com) triggers real webmention discovery on every publish; nock-blocked, so it error-logged each time.disableNetwork()now mocks those known domains to return a plain 200 page (no webmention link) — same "no endpoint found" outcome a real fetch would produce, without eating a connection error.batch-sending / automations / gift-links / offers — several tests deliberately exercise already-handled catch blocks (guard rejections, DB write failures, invalid data being filtered out) without stubbing the logger. These now stub
logging.error/warn/infoand assert the expected call fired, instead of letting it print.explore-ping — the explore "phone home" ping ran on every boot, including tests where no URL is configured. Gated to dev/prod only, matching the update-check service.
email-address-service.ts — two real over-eager logging bugs, not just test noise:
getAddress()error-logged an invalidreplyToeven when it wasnoreply@<default-domain>— the same addressvalidate()already special-cases as expected-but-invalid on IP-only installs. Now skips the error in that case.Sentry
captureMessagecrash (PLA-197) — the disabled-Sentry no-op stub only exportedcaptureException, notcaptureMessage.batch-sending-service.jscallscaptureMessagewhen a sent email's recipient count drifts from what was stored, so any Ghost instance without Sentry configured (the common case) crashed there instead of no-op'ing — marking otherwise-fine sends asfailed. Fixed, with a test.domain-warming test flakiness (PLA-198) — fixing the Sentry crash exposed that two
domain-warming.test.jstests were passing by accident (the crash prevented a corrected recipient count from ever being saved). Root cause: the test's fake-clock baseline was anchored to "today at noon UTC," which lands before real member-creation timestamps whenever the suite runs in the afternoon — member ObjectIds then look like they were created after the (now backdated) email, so the deliberate id-based recipient cutoff increateBatches()excluded everyone. Anchored to the next UTC midnight instead, which is always safely in the future regardless of time of day.Intermittent
SQLITE_BUSY— knex's pool allows up to 10 connections, but SQLite only supports one writer; without abusy_timeout, a second connection's write while another is in-flight failed immediately instead of waiting. Addedbusy_timeout = 5000to the connection setup.Full DB suite (
test:integration,test:e2e,test:unit) passes cleanly and repeatably; no snapshot changes.