Replaced should.equal() with assert.equal()#26113
Conversation
📝 WalkthroughWalkthroughReplaced should.js assertions with Node's strict Changes
Possibly related PRs
🚥 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)
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/members-comments/comments.test.jsghost/core/test/legacy/models/model-posts.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 |
EvanHahn
changed the base branch from
main
to
memberCountRounding-improve-types
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ghost/core/test/unit/frontend/src/privacy.test.js (2)
68-68:⚠️ Potential issue | 🟡 MinorMissed conversion:
should.deepEqual()→assert.deepEqual().This assertion was not converted as part of the automated migration. For consistency with the rest of the file and the PR objective, convert to
assert.deepEqual().🔧 Proposed fix
- should.deepEqual(parsed, {}); + assert.deepEqual(parsed, {});
138-138:⚠️ Potential issue | 🟡 MinorMissed conversion:
should.deepEqual()→assert.deepEqual().Same issue as line 68—this
should.deepEqual()call was not converted.🔧 Proposed fix
- should.deepEqual(parsed, {}); + assert.deepEqual(parsed, {});ghost/core/test/legacy/models/model-member-stripe-customer.test.js (1)
74-81:⚠️ Potential issue | 🟡 MinorFix assertion message to match expected count (Line 78).
The assertion expects 1 subscription, but the message says “two subscriptions,” which is misleading when the test fails.
Proposed fix
- assert.equal(subscriptions.length, 1, 'Should be two subscriptions'); + assert.equal(subscriptions.length, 1, 'Should be one subscription');
🤖 Fix all issues with AI agents
In `@ghost/core/test/unit/server/services/stripe/stripe-api.test.js`:
- Around line 267-271: The test title "returns null if customer exists" is
inconsistent with the assertion that expects undefined; update the test
description or the assertion to match—either rename the it(...) title to
"returns undefined if customer exists" or change the assert.equal to expect
null; locate the test containing api.getCustomerIdByEmail and adjust the string
in the it(...) call or the expected value in the assertion so both the title and
the assertion align.
In `@ghost/core/test/unit/server/web/api/middleware/cors.test.js`:
- Around line 1-2: Remove the unused assert import statement by deleting the
line that declares const assert = require('node:assert/strict'); and keep the
const should = require('should'); import as-is so remaining should.not.exist and
.should.equal assertions continue to work; ensure no other references to the
assert symbol remain in the file after removal.
🧹 Nitpick comments (13)
ghost/core/test/unit/server/web/api/middleware/update-user-last-seen.test.js (1)
1-2: Remove unusedshouldimport.The
shouldmodule is imported but no longer used after migrating toassert.equal(). This is likely a leftover from the automated replacement.🧹 Proposed fix to remove unused import
const assert = require('node:assert/strict'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/services/auth/members/index.test.js (1)
1-3: Remove unusedshouldimport.The
shouldmodule is still imported on line 3 but is no longer used after migrating all assertions toassert.equal().🧹 Proposed fix
const assert = require('node:assert/strict'); const jwt = require('jsonwebtoken'); -const should = require('should'); const {UnauthorizedError} = require('@tryghost/errors'); const members = require('../../../../../../core/server/services/auth/members');ghost/core/test/unit/frontend/src/privacy.test.js (1)
1-2: Unusedshouldimport can be removed.The
shouldimport on line 2 is retained, but the only remaining usages areshould.deepEqual()calls on lines 68 and 138. Once those are converted toassert.deepEqual(), this import should be removed.♻️ Proposed fix after converting remaining should.deepEqual calls
const assert = require('node:assert/strict'); -const should = require('should');ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js (1)
84-84: Consider swapping argument order for conventionalassert.equalusage.The
assert.equalconvention isassert.equal(actual, expected). Havingnull(the expected value) as the first argument will produce inverted error messages on failure. This is a side effect of the automatedsedreplacement preserving the originalshould.equalargument order.♻️ Suggested fix
- assert.equal(null, adapter.schedule.args[0][0].extra.oldTime); + assert.equal(adapter.schedule.args[0][0].extra.oldTime, null);ghost/core/test/unit/server/services/members/members-api/services/geolocation-service.test.js (1)
61-76: Swap argument order to followassert.equal(actual, expected)convention.The
assert.equalarguments are reversed. Node's assert functions expect(actual, expected)order. While the assertion will still work correctly (equality is symmetric), the error messages on failure will be confusing—showing "expected undefined to equal X" instead of "expected X to equal undefined".Proposed fix
- assert.equal(undefined, result); + assert.equal(result, undefined); scope = nock('https://get.geojs.io').get('/v1/ip/geo/null.json').reply(200, {test: true}); result = await service.getGeolocationFromIP(null); scope.isDone().should.eql(false); - assert.equal(undefined, result); + assert.equal(result, undefined); scope = nock('https://get.geojs.io').get('/v1/ip/geo/undefined.json').reply(200, {test: true}); result = await service.getGeolocationFromIP(undefined); scope.isDone().should.eql(false); - assert.equal(undefined, result); + assert.equal(result, undefined); scope = nock('https://get.geojs.io').get('/v1/ip/geo/test.json').reply(200, {test: true}); result = await service.getGeolocationFromIP('test'); scope.isDone().should.eql(false); - assert.equal(undefined, result); + assert.equal(result, undefined);ghost/core/test/unit/server/services/url/local-file-cache.test.js (1)
85-86: Mixed assertion styles in the same test case.This test uses
assert.equal()on line 85 butshould.equal()on line 86. While this is consistent with the PR's approach of only replacingshould.equal(x, null)patterns, having both styles in consecutive lines may look inconsistent.This is a minor observation - the code is functionally correct.
ghost/core/test/unit/server/web/api/middleware/cors.test.js (1)
133-134: Inconsistent assertion:calledvscalledOnce.This test uses
sinon.assert.called(res.end)while similar tests at lines 64, 79, and 111 usesinon.assert.calledOnce(res.end). Ifres.endshould only be called once, consider usingcalledOncefor consistency and stricter verification.♻️ Suggested change for consistency
- sinon.assert.called(res.end); + sinon.assert.calledOnce(res.end);ghost/core/test/unit/server/services/themes/validate.test.js (1)
6-6: Optional: Consider usingnode:assert/strictfor consistency with newer test files.Some test files in this codebase use
require('node:assert/strict')while others userequire('assert/strict'). Thenode:prefix makes it explicit that this is a built-in Node.js module. This is a very minor nit and can be addressed in a follow-up cleanup if desired.ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (1)
80-80: LGTM!The
should.equal()calls are correctly replaced withassert.equal(). The assertions verify that the cached image object retains the original URL.,
🔧 Optional: Use the `url` variable instead of hardcoding
Both tests already define a
urlvariable with the same value. Using it in the assertion reduces duplication:- assert.equal(image.url, 'http://mysite.com/content/image/mypostcoverimage.jpg'); + assert.equal(image.url, url);Also applies to: 101-101
ghost/core/test/integration/migrations/nullable-utils.test.js (1)
1-2: Mixed assertion libraries remain in this file.The
shouldimport is still required because severalshould.ok()calls remain (lines 162, 238, 324, 329, 338). Since the PR scope is specificallyshould.equal()→assert.equal(), this is expected. Consider completing the migration by replacingshould.ok()withassert.ok()in a follow-up to remove theshoulddependency entirely.ghost/core/test/unit/server/services/lib/dynamic-redirect-manager.test.js (1)
57-57: Consider completing the assertion migration for consistency.The file now mixes assertion styles:
assert.equal()alongsideshould.fail(),should.ok(),.should.equal(), and.should.be.empty(). If the goal is to eventually remove theshoulddependency, these could be migrated to:
should.fail(...)→assert.fail(...)should.ok(...)→assert.ok(...)x.should.equal(y)→assert.equal(x, y)arr.should.be.empty()→assert.deepStrictEqual(arr, [])orassert.equal(arr.length, 0)This is optional and can be deferred since the PR explicitly targets
should.equal()only.Also applies to: 72-72, 126-128, 141-141
ghost/core/test/unit/server/data/importer/import-manager.test.js (1)
1-2: Consider removing unusedshouldimport.The
shouldlibrary is imported but no longer used in this file after the migration. This could be cleaned up.♻️ Suggested cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const fs = require('fs-extra');ghost/core/test/unit/server/services/adapter-manager/options-resolver.test.js (1)
1-2: Complete the assert/strict migration in this test file for consistency.Now that
node:assert/strictis imported, dropshouldentirely by converting the remaining assertions. This aligns with Node.js best practices—node:assert/strictis the recommended standard for new code, and the simple config object comparisons in this test have no edge cases that would cause behavioral differences.♻️ Suggested refactor
const assert = require('node:assert/strict'); -const should = require('should'); @@ - adapterClassName.should.equal('Memory'); - assert.equal(adapterConfig, undefined); + assert.equal(adapterClassName, 'Memory'); + assert.equal(adapterConfig, undefined); @@ - adapterClassName.should.equal('cloud-storage'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'cloud-storage'); + assert.deepEqual(adapterConfig, { custom: 'configValue' }); @@ - adapterClassName.should.equal('local-storage'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'local-storage'); + assert.deepEqual(adapterConfig, { custom: 'localStorageConfig' }); @@ - adapterClassName.should.equal('cloud-storage'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'cloud-storage'); + assert.deepEqual(adapterConfig, { custom: 'configValue' }); @@ - adapterClassName.should.equal('Redis'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'Redis'); + assert.deepEqual(adapterConfig, { commonConfigValue: 'common_config_value', adapterConfigValue: 'images_redis_value' }); @@ - secondadapterClassName.should.equal('Redis'); - secondAdapterConfig.should.deepEqual({ + assert.equal(secondadapterClassName, 'Redis'); + assert.deepEqual(secondAdapterConfig, { commonConfigValue: 'common_config_value', adapterConfigValue: 'settings_redis_value' }); @@ - adapterClassName.should.equal('Redis'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'Redis'); + assert.deepEqual(adapterConfig, { adapterConfigValue: 'images_redis_value' }); @@ - secondadapterClassName.should.equal('Redis'); - secondAdapterConfig.should.deepEqual({ + assert.equal(secondadapterClassName, 'Redis'); + assert.deepEqual(secondAdapterConfig, { adapterConfigValue: 'settings_redis_value' }); @@ - adapterClassName.should.equal('Redis'); - adapterConfig.should.deepEqual({ + assert.equal(adapterClassName, 'Redis'); + assert.deepEqual(adapterConfig, { commonConfigValue: 'common_config_value', adapterConfigValue: 'images_redis_value', overrideMe: 'images_override' });
EvanHahn
force-pushed
the
should.equal-begone
branch
from
de9da98 to
e9727ce
Compare
EvanHahn
force-pushed
the
should.equal-begone
branch
from
e9727ce to
a9039da
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ghost/core/test/legacy/models/model-member-stripe-customer.test.js`:
- Line 78: Update the assertion message for the assert.equal call that checks
subscriptions.length: the message currently reads "Should be two subscriptions"
but the assertion expects 1 and contains a double-space; change it to a correct,
concise message like "Should be one subscription" (refer to the
assert.equal(subscriptions.length, 1, ...) usage) so the message matches the
expected value and remove the extra space.
In
`@ghost/core/test/unit/server/services/adapter-manager/options-resolver.test.js`:
- Around line 16-19: Replace the mixed assertion on adapterConfig with the
file's predominant should-style: in the test that calls
resolveAdapterOptions(name, adapterServiceConfig) change the
assert.equal(adapterConfig, undefined) assertion to use
adapterConfig.should.equal(undefined) so both adapterClassName and adapterConfig
use should.equal() consistently.
🧹 Nitpick comments (4)
ghost/core/test/unit/server/services/auth/session/store.test.js (2)
7-7: Consider replacingshould.deepEqualwithassert.deepEqualto complete the migration.The
shouldimport on line 7 is retained only forshould.deepEqualon line 109. Replacing it withassert.deepEqual()would allow removing theshoulddependency entirely from this file.♻️ Suggested change
Remove the
shouldimport:-const should = require('should');And update line 109:
- should.deepEqual(session, { - ice: 'cube' - }); + assert.deepEqual(session, { + ice: 'cube' + });Also applies to: 109-111
20-20: Consider usingassert.ok()for boolean checks.For
instanceofchecks,assert.ok()is more idiomatic thanassert.equal(..., true).♻️ Suggested change
- assert.equal(store instanceof EventEmitter, true); + assert.ok(store instanceof EventEmitter);- assert.equal(store instanceof Store, true); + assert.ok(store instanceof Store);Also applies to: 25-25
ghost/core/test/unit/server/services/settings-helpers/settings-helpers.test.js (1)
1-6: Unusedshouldimport can be removed.The
shouldlibrary is imported on line 1 but is no longer used anywhere in this file after the migration toassert. Consider removing it to keep the imports clean.🧹 Suggested cleanup
-const should = require('should'); const sinon = require('sinon'); const configUtils = require('../../../../utils/config-utils'); const SettingsHelpers = require('../../../../../core/server/services/settings-helpers/settings-helpers'); const crypto = require('crypto'); const assert = require('assert').strict;ghost/core/test/unit/server/models/session.test.js (1)
52-57: Consider asserting property absence explicitly.The test description says the key should be missing; checking for
undefinedallows a present-but-undefined key. A direct “no key” assertion makes the intent precise.♻️ Suggested tweak
const formatted = format(attrs); - assert.equal(formatted.session_data, undefined); + assert.equal(Object.prototype.hasOwnProperty.call(formatted, 'session_data'), false);
ghost/core/test/legacy/models/model-member-stripe-customer.test.js
Outdated
Show resolved
Hide resolved
ghost/core/test/unit/server/services/adapter-manager/options-resolver.test.js
Show resolved
Hide resolved
This test-only change should have no user impact. This replaces (with a simple `sed` invocation) all occurrences of `should.equal()` with `assert.equal()`.
Mostly, checking for `null`/`undefined` needs to be more explicit now.
EvanHahn
force-pushed
the
should.equal-begone
branch
from
2aa979f to
81fc4a8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| @@ -1,10 +1,11 @@ | |||
| const assert = require('node:assert/strict'); | |||
There was a problem hiding this comment.
Unused should import after assertion conversion
Low Severity
const should = require('should'); is imported but never used in these files after converting all should.equal() calls to assert.equal(). Five test files have this unused import: total-paid-members.test.js, og-type.test.js, modified-date.test.js, published-date.test.js, and overrides.test.js.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js`:
- Around line 322-325: Several assertions were partially converted: while
addFixturesForModelStub and addFixturesForRelationStub calls now use
assert.equal, there remain multiple `.should.equal()` usages left (e.g.,
assertions tied to the same fixture test cases and stubs). Replace all remaining
`.should.equal(expected)` occurrences with the equivalent assert.equal(actual,
expected) form (preserving the original actual value and expected order) by
searching for `.should.equal(` and converting each to
assert.equal(actualExpression, expectedExpression); ensure assertions related to
addFixturesForModelStub, addFixturesForRelationStub and any other test variables
use the same assert style for consistency.
In `@ghost/core/test/unit/server/web/api/middleware/cors.test.js`:
- Line 132: Test assertion for res.end is less strict than others; update the
assertion in cors.test.js from sinon.assert.called(res.end) to
sinon.assert.calledOnce(res.end) so it matches other tests (lines that use
calledOnce) and will catch multiple calls to res.end; locate the assertion
referencing res.end in the failing test and replace the sinon.assert.called call
with sinon.assert.calledOnce.
🧹 Nitpick comments (5)
ghost/core/test/unit/frontend/meta/modified-date.test.js (1)
1-2: Remove unusedshouldimport.The
shouldimport on line 2 is no longer used after migrating all assertions toassert.equal().🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const getModifiedDate = require('../../../../core/frontend/meta/modified-date');ghost/core/test/unit/frontend/meta/og-type.test.js (1)
1-2: Unusedshouldimport can be removed.The
shouldimport on line 2 is no longer used after the assertion migration and should be removed.🧹 Proposed fix to remove unused import
const assert = require('node:assert/strict'); -const should = require('should'); const getOgType = require('../../../../core/frontend/meta/og-type');ghost/core/test/unit/frontend/src/privacy.test.js (1)
1-2: Consider migratingshould.deepEqual()toassert.deepEqual()for consistency.The
shouldimport is retained solely forshould.deepEqual()on lines 68 and 138. Migrating these toassert.deepEqual()would allow removing theshoulddependency entirely from this file.♻️ Proposed consolidation
const assert = require('node:assert/strict'); -const should = require('should');And at lines 68 and 138:
- should.deepEqual(parsed, {}); + assert.deepEqual(parsed, {});Based on learnings: keeping PRs focused on their primary objective is preferred, so this could also be addressed in a follow-up PR if the current scope is intentional.
ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js (1)
84-84: Consider conventional argument order forassert.equal.Node's
assert.equal()conventionally usesassert.equal(actual, expected). The current order places the expected value (null) first, which will still work but may produce less intuitive error messages on failure (e.g., "expected [actual] to equal null" vs "expected null to equal [actual]").♻️ Suggested fix
- assert.equal(null, adapter.schedule.args[0][0].extra.oldTime); + assert.equal(adapter.schedule.args[0][0].extra.oldTime, null);ghost/core/test/unit/frontend/helpers/facebook-url.test.js (1)
1-2: Consider completing the migration toassertin this file.Only one assertion moved to
assert.equalwhile the rest still useshould.equal, which keeps mixed styles. If the PR’s goal is full migration, consider converting the remaining assertions and droppingshould.Proposed cleanup
-const should = require('should'); +// should no longer needed if all assertions use assert @@ - facebook_url.call({}, options).should.equal('https://www.facebook.com/hey'); + assert.equal(facebook_url.call({}, options), 'https://www.facebook.com/hey'); @@ - facebook_url.call({facebook: 'you/there'}, options).should.equal('https://www.facebook.com/you/there'); + assert.equal(facebook_url.call({facebook: 'you/there'}, options), 'https://www.facebook.com/you/there'); @@ - facebook_url.call({facebook: 'you/there'}, 'i/see/you/over/there', options) - .should.equal('https://www.facebook.com/i/see/you/over/there'); + assert.equal( + facebook_url.call({facebook: 'you/there'}, 'i/see/you/over/there', options), + 'https://www.facebook.com/i/see/you/over/there' + );
ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
Show resolved
Hide resolved
1 participant
This test-only change should have no user impact.
This change is split into two commits:
should.equal()withassert.equal()across the codebase. Usessed, and then importsnode:assert/strictwhere relevant.Summary by CodeRabbit
Tests
Refactor
Note
Low Risk
Test-only refactor with minimal logic impact; risk is limited to potential assertion semantic differences causing flaky or newly-failing tests.
Overview
Standardizes the test suite to use Node’s strict assertions by importing
node:assert/strictand replacing manyshould.equal()calls withassert.equal()across e2e, integration, legacy, and unit tests.Includes minor follow-up fixes to keep tests passing under stricter semantics (notably adjusting an importer test expectation for a lexical post’s
mobiledocto beundefinedrather thannull).Written by Cursor Bugbot for commit 81fc4a8. This will update automatically on new commits. Configure here.