Cleaned up DB test-suite error-log noise (Stripe boot, 4xx, image-size, batch-sending)#28909
Open
9larsons wants to merge 4 commits into
Open
Cleaned up DB test-suite error-log noise (Stripe boot, 4xx, image-size, batch-sending)#289099larsons wants to merge 4 commits into
9larsons wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (18)
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)ghost/core/test/e2e-api/admin/members.test.jsThanks 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:ci:integration |
✅ Succeeded | 2m 35s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 18s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 52s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 9s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 3s | View ↗ |
nx run-many -t lint -p ghost |
✅ Succeeded | 34s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 28s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 7s | 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-06-25 18:50:32 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
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. ~1,050 fewer error lines per full run (≈86%: ~1,225 → 172 remaining).
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. (342 → 76)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). The 27 tests that capture+assert their 4xx log were moved towarn, and a newlog-requestunit test pins the contract (5xx → error;4xx → errorby default /warnwhen configured;404 → info). (315 → 0)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);cards.test.jsopts back in for the two tests that exercise the lookup itself. (~250 → 0)batch-sending — two tests deliberately exercise the "not pending or failed" guards; they now stub
logging.errorand assert it instead of printing. (206 → 2)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.
Full DB suite: 2,526 passing, no snapshot changes.