Fixed urlUtils.restore() async behavior
#24651
Conversation
no issue - the `urlUtils.restore()` test util calls `configUtils.restore()` which returns a promise but it was not awaiting it meaning you could end up with confusing test behaviour if the timing was just right - changed `urlUtils.restore()` to be an async method that correctly awaits the config restore and updated all uses to match - added missing `urlUtils.restore()` to the newsletters service unit test that was found after it was preventing a new unit test in a different PR from passing because it caused a double-wrapped stub
WalkthroughAcross multiple test files, calls to urlUtils.restore() in after/afterEach hooks were updated to await urlUtils.restore(). Additionally, ghost/core/test/utils/urlUtils.js changed restore from a synchronous arrow function to an async function that awaits configUtils.restore(). No assertions or other test logic were modified, and no production code changed. Public API names remain the same; only the restore function’s signature is now async. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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.38.6)ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
ghost/core/test/unit/server/services/members/config.test.js (1)
62-66: Deduplicate teardown callsurlUtils.restore() already awaits configUtils.restore(). You can safely drop the explicit configUtils.restore() here to avoid redundant work.
- await configUtils.restore(); await urlUtils.restore(); sinon.restore();ghost/core/test/legacy/api/content/posts.test.js (1)
19-22: Teardown duplication (minor)urlUtils.restore() awaits configUtils.restore() internally. Consider removing the direct config restore to avoid doing it twice.
- await configUtils.restore(); await urlUtils.restore();ghost/core/test/e2e-server/admin.test.js (1)
91-94: Order and duplication in restoreurlUtils.restore() internally awaits configUtils.restore(), so calling configUtils.restore() again right after is redundant. Consider keeping only urlUtils.restore().
- await urlUtils.restore(); - await configUtils.restore(); + await urlUtils.restore(); // includes awaiting config restoreghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js (4)
18-22: Minor: redundant config restoreurlUtils.restore() awaits configUtils.restore(). You can remove the explicit configUtils.restore() call here.
- await configUtils.restore(); await urlUtils.restore(); sinon.restore();
38-42: Minor: redundant config restore (same pattern)Drop configUtils.restore(); urlUtils.restore() already does it.
- await configUtils.restore(); await urlUtils.restore(); sinon.restore();
145-149: Minor: redundant config restore (same pattern)Remove duplicate config restore.
- await configUtils.restore(); await urlUtils.restore(); sinon.restore();
297-301: Minor: redundant config restore (same pattern)Remove duplicate config restore.
- await configUtils.restore(); await urlUtils.restore(); sinon.restore();ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js (1)
45-49: Teardown consolidation: manual verification requiredThe provided
rgsearch didn’t find any occurrences ofawait configUtils.restore(); await urlUtils.restore();in
ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js. Please confirm the file path and verify that both calls exist in each teardown hook before removing the explicitconfigUtils.restore().You can list all instances with:
rg -n "configUtils\\.restore" ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js rg -n "urlUtils\\.restore" ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.jsghost/core/test/utils/urlUtils.js (1)
53-57: Repo-wide consistency: single source of truth for config restoreBecause restore() calls configUtils.restore(), tests that also call configUtils.restore() now do it twice. Either:
- Document that tests should only call urlUtils.restore() when using url stubs, or
- Stop calling configUtils.restore() here and let tests manage config teardown explicitly.
Given this PR’s goal, the first option is preferred; consider a short JSDoc above restore() clarifying the behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ghost/core/test/e2e-server/admin.test.js(1 hunks)ghost/core/test/integration/services/q-email-addresses.test.js(1 hunks)ghost/core/test/legacy/api/content/posts.test.js(1 hunks)ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js(13 hunks)ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js(4 hunks)ghost/core/test/unit/server/services/members/config.test.js(1 hunks)ghost/core/test/unit/server/services/newsletters/service.test.js(1 hunks)ghost/core/test/utils/urlUtils.js(1 hunks)
🔇 Additional comments (3)
ghost/core/test/unit/server/services/newsletters/service.test.js (2)
84-86: Good addition: await urlUtils.restore in after()Async teardown prevents leaked stubs between suites. Looks correct.
84-86: LGTMAsynchronous url utils restore in after() aligns with the updated helper. No duplication with config restore here.
ghost/core/test/utils/urlUtils.js (1)
53-57: Great fix: make restore async and await config restoreThis resolves the previous race in teardown. The ordering (sandbox restore, then config restore) is sensible.
| afterEach(async function () { | ||
| await configUtils.restore(); | ||
| urlUtils.restore(); | ||
| await urlUtils.restore(); | ||
| mockManager.restore(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid double config restore
urlUtils.restore() already awaits configUtils.restore() internally. Calling both here restores config twice. Consider removing the explicit configUtils.restore() for cleaner, faster teardown.
Example change:
- await configUtils.restore();
await urlUtils.restore();To find similar duplicates across the repo:
🏁 Script executed:
#!/usr/bin/env bash
rg -n -A2 -B2 $'await\\s+configUtils\\.restore\\(\\)[\\s\\S]*?await\\s+urlUtils\\.restore\\(\\)' test | sed -n '1,200p'Length of output: 153
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "### Searching for configUtils.restore occurrences in the repo ###"
rg -n "await configUtils\\.restore\\(\\)" .
echo ""
echo "### Searching for urlUtils.restore occurrences in the repo ###"
rg -n "await urlUtils\\.restore\\(\\)" .
echo ""
echo "### Showing q-email-addresses test around teardown ###"
sed -n '130,150p' ghost/core/test/integration/services/q-email-addresses.test.js
echo ""
echo "### Locating url-utils implementation files ###"
URL_UTIL_FILES=$(find . -type f | grep -i "url-utils.*\\.js" || true)
echo "$URL_UTIL_FILES"
echo ""
for file in $URL_UTIL_FILES; do
echo "---- Contents of $file ----"
sed -n '1,200p' "$file"
echo ""
doneLength of output: 16940
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "### Opening ghost/core/test/utils/urlUtils.js ###"
sed -n '1,300p' ghost/core/test/utils/urlUtils.js
echo ""
echo "### Looking for restore function definitions ###"
rg -n -C3 "restore" ghost/core/test/utils/urlUtils.jsLength of output: 2842
Remove redundant configUtils.restore() in afterEach
urlUtils.restore() already invokes configUtils.restore(), so the explicit call duplicates work and can be dropped.
• File: ghost/core/test/integration/services/q-email-addresses.test.js
In the afterEach teardown, change:
afterEach(async function () {
- await configUtils.restore();
await urlUtils.restore();
mockManager.restore();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(async function () { | |
| await configUtils.restore(); | |
| urlUtils.restore(); | |
| await urlUtils.restore(); | |
| mockManager.restore(); | |
| }); | |
| afterEach(async function () { | |
| await urlUtils.restore(); | |
| mockManager.restore(); | |
| }); |
🤖 Prompt for AI Agents
In ghost/core/test/integration/services/q-email-addresses.test.js around lines
138 to 142, the afterEach teardown calls configUtils.restore() explicitly even
though urlUtils.restore() already calls configUtils.restore(); remove the
redundant await configUtils.restore() line so the afterEach only awaits
urlUtils.restore() and mockManager.restore() to avoid duplicate work.
There was a problem hiding this comment.
The configUtils.restore() is otherwise hidden if we rely on only urlUtils.restore(). I think it's better to be explicit here so tests are easier to understand and there's no confusion if urlUtils usage is removed from the test at some point.
There was a problem hiding this comment.
@kevinansfield That's a fair point about test clarity and maintainability. You're right that being explicit about cleanup dependencies makes the tests more self-documenting and resilient to future changes. The minor duplicate work is a reasonable trade-off for clearer test intent.
✏️ Learnings added
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/integration/services/q-email-addresses.test.js:138-142
Timestamp: 2025-08-11T19:37:41.008Z
Learning: In Ghost's test suite, prefer explicit cleanup calls (like `configUtils.restore()`) in test teardown even when they might be called by other utilities (like `urlUtils.restore()`), as this improves test readability and prevents issues if test dependencies change in the future.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const restore = async () => { | ||
| defaultSandbox.restore(); | ||
| // eslint-disable-next-line no-console | ||
| configUtils.restore().catch(console.error); | ||
| await configUtils.restore().catch(console.error); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t swallow teardown errors
await configUtils.restore().catch(console.error) logs but hides failures. Prefer surfacing errors so broken teardowns fail fast.
Suggested change:
-const restore = async () => {
- defaultSandbox.restore();
- // eslint-disable-next-line no-console
- await configUtils.restore().catch(console.error);
-};
+const restore = async () => {
+ defaultSandbox.restore();
+ try {
+ await configUtils.restore();
+ } catch (err) {
+ // eslint-disable-next-line no-console
+ console.error(err);
+ throw err; // fail the test when teardown fails
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const restore = async () => { | |
| defaultSandbox.restore(); | |
| // eslint-disable-next-line no-console | |
| configUtils.restore().catch(console.error); | |
| await configUtils.restore().catch(console.error); | |
| }; | |
| const restore = async () => { | |
| defaultSandbox.restore(); | |
| try { | |
| await configUtils.restore(); | |
| } catch (err) { | |
| // eslint-disable-next-line no-console | |
| console.error(err); | |
| throw err; // fail the test when teardown fails | |
| } | |
| }; |
🤖 Prompt for AI Agents
In ghost/core/test/utils/urlUtils.js around lines 53 to 57, the teardown
currently uses await configUtils.restore().catch(console.error) which logs
errors but swallows them; change this to let the error propagate so test
teardown failures fail the test run: remove the .catch(console.error) and either
await configUtils.restore() directly or catch the error, log it, then rethrow
(e.g., catch(err){ console.error(err); throw err; }) so failures are surfaced.
There was a problem hiding this comment.
This was kept as-is to keep the PR focused on the async behaviour.
There was a problem hiding this comment.
@kevinansfield, understood! That makes perfect sense - keeping the PR focused on the async behavior fix without introducing additional changes to error handling patterns. This approach maintains clarity about what the PR is solving and makes it easier to review.
✏️ Learnings added
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.388Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/integration/services/q-email-addresses.test.js:138-142
Timestamp: 2025-08-11T19:37:41.008Z
Learning: In Ghost's test suite, prefer explicit cleanup calls (like `configUtils.restore()`) in test teardown even when they might be called by other utilities (like `urlUtils.restore()`), as this improves test readability and prevents issues if test dependencies change in the future.
no issue
urlUtils.restore()test util callsconfigUtils.restore()which returns a promise but it was not awaiting or returning it meaning tests that stubbedurlUtilsmultiple times would have very confusing/incorrect test behaviour if the timing was just righturlUtils.restore()to be an async method that correctly awaits the config restore and updated all uses to matchurlUtils.restore()to the newsletters service unit test that was found after it was preventing a new unit test in a different PR from passing because it caused a double-wrapped stub