Replaced calls to should() function with assert

[EvanHahn]

This test-only change should have no user impact.

We had various calls like should(actual).eql(expected) and similar. This replaces all of them with calls to node:assert, or wrappers around that.

git grep -F 'should(' returns no results after this change.

[coderabbitai]

Walkthrough

This pull request migrates numerous test files from the Should.js assertion style to Node's built-in assert API. It replaces should-based assertions with assert.equal / assert.notEqual / assert.deepEqual / assert.rejects and updates imports accordingly. The test utilities module was extended: assertArrayContainsDeep was added and assertObjectMatches was refactored to recurse into plain objects and use deep-equality checks. Changes are limited to tests and test utilities; no production API signatures were modified.

Possibly related PRs

• Replace more Should.js calls with node:assert #26284: Similar migration of test assertions from Should.js to Node's assert, including use of helpers like assertArrayContainsDeep.
• Replaced Should.js's should.exist() with node:assert #26278: Modifies the test assertion utilities (ghost/core/test/utils/assertions.js) and updates tests to use assert-based helpers.
• Replaced some Should.js assertions #26253: Overlapping assertion-style changes across many test files (members-related tests), showing strong code-level overlap.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: replacing should() function calls with assert. It directly reflects the primary objective of the pull request.
Description check	✅ Passed	The description is clearly related to the changeset, explaining the purpose (test-only change with no user impact) and the specific migration from should() to node:assert across test files.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch remove-should-fn-calls

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.40.5)

ghost/core/test/e2e-api/admin/members.test.js

ghost/core/test/e2e-api/members-comments/comments.test.js

ghost/core/test/e2e-api/members/webhooks.test.js

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@ghost/core/test/legacy/api/admin/update-user-last-seen.test.js`:
- Line 90: Change the bogus Date comparison using assert.notEqual to a
value-based comparison: replace the assertion that references
assert.notEqual(ownerAfter.get('last_seen'), lastSeen) with a deep/value
comparison (for example use assert.notDeepEqual(ownerAfter.get('last_seen'),
lastSeen) or compare timestamps via ownerAfter.get('last_seen').getTime() !==
lastSeen.getTime()) so the test actually verifies the last_seen timestamp
changed.

In `@ghost/core/test/utils/assertions.js`:
- Around line 49-57: The recursion guard in assertObjectMatches only checks
isPlainObject(obj[key]) and may recurse when the corresponding value is
null/primitive/array, causing Object.entries(value) to throw; update the
condition in assertObjectMatches so it only recurses when both
isPlainObject(obj[key]) and isPlainObject(value) are true, and otherwise fall
back to the existing assert.deepEqual(obj[key], value, message || `Property
mismatch for key "${key}"`); reference assertObjectMatches, isPlainObject,
obj[key], value, and assert.deepEqual to locate and change the check.

🧹 Nitpick comments (2)

ghost/core/test/legacy/api/admin/update-user-last-seen.test.js (1)

1-1: Minor: Use node:assert/strict for consistency with the rest of the codebase.

Other test files (e.g., link-click-tracking-service.test.js, members.test.js, and test/utils/assertions.js) all use require('node:assert/strict').

Suggested fix

-const assert = require('assert/strict');
+const assert = require('node:assert/strict');

ghost/core/test/e2e-api/members-comments/comments.test.js (1)

480-481: Missed should.notEqual calls on lines 480–481.

These still use should.notEqual(...), while the analogous assertion on line 438 was already converted to assert.notEqual(...). Consider converting for consistency — these are functionally identical patterns and the should import (line 4) could potentially be removed if all such calls are migrated.

Proposed fix

                data2.body.comments.forEach((comment) => {
-                    should.notEqual(comment.html, 'This is a hidden comment');
-                    should.notEqual(comment.html, 'This is a deleted comment');
+                    assert.notEqual(comment.html, 'This is a hidden comment');
+                    assert.notEqual(comment.html, 'This is a deleted comment');
                });

Note: Lines 263, 266, 297, and 300 (testCanCommentOnPost and testCanReply helpers) also still use should.notEqual. Converting all of them would allow removing the should import on line 4.

[coderabbitai]

🧹 Nitpick comments (1)

ghost/core/test/e2e-api/members/webhooks.test.js (1)

42-45: Move the length assertion before the loop for clearer failure diagnostics.

If events is shorter than asserts, events[i] will be undefined and assertObjectMatches will throw a confusing error before the descriptive length-check message on line 45 is reached. Swapping the order gives a meaningful message first.

Suggested reorder

 async function assertMemberEvents({eventType, memberId, asserts}) {
     const events = (await models[eventType].where('member_id', memberId).fetchAll()).toJSON();
+    assert.equal(events.length, asserts.length, `Only ${asserts.length} ${eventType} should have been added.`);
     for (let i = 0; i < asserts.length; i++) {
         assertObjectMatches(events[i], asserts[i]);
     }
-    assert.equal(events.length, asserts.length, `Only ${asserts.length} ${eventType} should have been added.`);
 }