Replace more Should.js calls with node:assert#26284
Conversation
WalkthroughThis PR converts test assertions across the codebase from Should.js-style syntax to Node's built-in assert API (e.g., Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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/admin/members.test.jsghost/core/test/legacy/models/model-posts.test.jsghost/core/test/unit/server/services/email-service/email-renderer.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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ghost/core/test/unit/server/lib/image/image-size.test.js (2)
453-456:⚠️ Potential issue | 🟡 MinorIncomplete migration: Should.js usage remains.
This assertion was not converted. Use
assert.fail()for intentional test failures.Proposed fix
imageSize.getImageSizeFromUrl(url) .then(() => { - true.should.be.false('succeeded when expecting failure'); + assert.fail('succeeded when expecting failure'); })
487-490:⚠️ Potential issue | 🟡 MinorIncomplete migration: Should.js usage remains.
Same issue as above — this Should.js assertion was not converted.
Proposed fix
imageSize.getImageSizeFromUrl(url) .then(() => { - true.should.be.false('succeeded when expecting failure'); + assert.fail('succeeded when expecting failure'); })
🤖 Fix all issues with AI agents
In `@ghost/core/test/unit/frontend/apps/private-blogging/input-password.test.js`:
- Around line 1-4: Remove the unused "should" import by deleting the
require('should') statement (the "should" identifier) from the test file; ensure
there are no remaining references to the "should" symbol and that tests continue
to use the existing assert/assertExists calls.
🧹 Nitpick comments (38)
ghost/core/test/unit/frontend/services/data/checks.test.js (1)
2-2: Remove unusedshouldimport.The
shouldimport is no longer used anywhere in this file—all assertions now use Node'sassert. This leftover can be removed.🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const {checks} = require('../../../../../core/frontend/services/data');ghost/core/test/unit/server/web/parent/middleware/ghost-locals.test.js (1)
4-4: Unusedshouldimport can be removed.The
shouldimport is no longer used in this file after the assertion conversions.🧹 Proposed fix
const _ = require('lodash'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/integration/importer/v5.js (1)
76-76: Missed Should.js assertion.This line still uses Should.js style (
.should.not.equal()) while the rest of the file has been converted toassert. For consistency with the PR's objective, this should also be converted.♻️ Suggested fix
- user2.id.should.not.equal(LEGACY_HARDCODED_USER_ID); + assert.notEqual(user2.id, LEGACY_HARDCODED_USER_ID);ghost/core/test/unit/server/models/integration.test.js (1)
2-2: Unusedshouldimport can be removed.The
shouldmodule is imported but no longer used anywhere in this file after the conversions toassert. Consider removing this dead import to complete the cleanup.🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/apps/private-blogging/controller.test.js (1)
80-90: Correct conversion, though inconsistent within this test case.The
assert.deepEqualreplacement is semantically equivalent to.should.eql(). However, line 84 still usesview.should.eql(defaultPath)while line 85 usesassert.deepEqual, creating mixed assertion styles within the same test.Consider converting line 84 as well for consistency:
res.render = function (view, context) { - view.should.eql(defaultPath); + assert.equal(view, defaultPath); assert.deepEqual(context, {error: 'Test Error'});Similar conversions could be applied to lines 60 in the first test case. This would also allow removing the
shouldimport on line 3.ghost/core/test/unit/server/services/stats/mrr.test.js (1)
6-6: Incomplete migration:shouldis still used inassertTotals.The
shouldimport on line 6 is still required becauseassertTotals(line 83) usestotals.should.eql(expectedTotals). For consistency with this PR's objective and the convertedassertMrrEntryhelper, consider convertingassertTotalsas well.♻️ Suggested refactor
-const should = require('should');function assertTotals(totals, expected) { const expectedTotals = expected.map(item => ({ mrr: item.mrr, currency: item.currency || 'usd' })); - totals.should.eql(expectedTotals); + assert.deepEqual(totals, expectedTotals); }Also applies to: 78-84
ghost/core/test/unit/server/web/api/middleware/cors.test.js (1)
2-2: Unusedshouldimport can be removed.The
shouldmodule is imported but no longer used in this file after the assertion conversions. Remove the dead import to complete the migration.🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/services/assets-minification/minifier.test.js (1)
100-117: Conversion looks correct; minor style nit on extra parentheses.The migration from Should.js to
assert.throws()is functionally correct. The extra parentheses wrapping each function expression are unnecessary but harmless:assert.throws((function noObject(){ ... })); // could be simplified to: assert.throws(function noObject() { ... }); // or with arrow functions: assert.throws(() => { new Minifier(); });This is a very minor style nit and can be left as-is given the PR's focus on migration.
ghost/core/test/unit/frontend/helpers/link.test.js (1)
2-2: Unusedshouldimport can be removed.The
shouldmodule is no longer used in this file after the migration toassert. Consider removing this import to complete the cleanup.🧹 Suggested cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const concat = require('../../../../core/frontend/helpers/concat');ghost/core/test/unit/frontend/helpers/link-class.test.js (1)
2-2: Unusedshouldimport can be removed.The
shouldmodule is no longer used in this file after the migration toassert. Consider removing this import to complete the cleanup.🧹 Suggested cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const concat = require('../../../../core/frontend/helpers/concat');ghost/core/test/unit/frontend/helpers/tags.test.js (1)
3-3: Remove unusedshouldimport.The
shouldimport is no longer used in this file after converting all assertions toassert.equalandassertExists. This can be safely removed as part of the migration.🧹 Proposed fix
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/lib/image/image-size.test.js (1)
1-3:shouldimport can be removed after completing the migration.The
shouldimport on line 3 is still present. Once the remaining Should.js usages on lines 455 and 489 are converted, this import can be removed entirely.ghost/core/test/unit/server/services/route-settings/validate.test.js (1)
2-6: Consider removing the unusedshouldimport.The
shouldlibrary import and the dummyshould.equal(true, true)statement appear to be unused now that all assertions in this file have been converted to useassert. These lines can likely be removed.🧹 Proposed cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const errors = require('@tryghost/errors'); const validate = require('../../../../../core/server/services/route-settings/validate'); - -should.equal(true, true);ghost/core/test/unit/server/services/notifications/notifications.test.js (1)
41-43: Optional: Remaining Should.js usages could be converted.Lines 41 and 43 still use Should.js style assertions. Per the PR's mapping pattern, these could be converted to the project's
assertExistshelper:-createdNotification.id.should.not.be.undefined(); +assertExists(createdNotification.id); assert.equal(createdNotification.custom, true); -createdNotification.createdAt.should.not.be.undefined(); +assertExists(createdNotification.createdAt);This would also allow removing the
shouldimport on line 3. However, this can be deferred to a future PR if preferred.ghost/core/test/integration/importer/v2.test.js (1)
628-654: Conversions are correct; consider converting remaining Should.js assertions for consistency.The email and timestamp assertions (lines 628-648) are correctly converted to
assert.equal(). However, lines 650-654 still useusers[n].roles[0].id.should.eql(...)within the same test block, creating a mixed assertion style.This is fine to leave as-is for this PR if the goal is incremental migration, but flagging for potential follow-up to maintain consistency within test functions.
ghost/core/test/unit/server/services/auth/session/session-service.test.js (1)
1-3: Note: Additional Should.js usages remain in this file.The
shouldimport is still required for remaining usages at lines 63, 152, 460, and 507. These could be addressed in a follow-up PR:
should.ok(spy.calledWith(...))→sinon.assert.calledWith(spy, ...)should.notEqual(a, b)→assert.notEqual(a, b).should.be.fulfilled()→ Can be removed if the promise rejection would naturally fail the testThis is not blocking since the PR scope is incremental migration.
ghost/core/test/unit/server/services/link-tracking/link-click-tracking-service.test.js (1)
346-346: Consider converting this assertion toassert.rejectsfor consistency.This remaining Should.js usage could be converted to Node's built-in assert, which would also allow removing the
shouldimport on line 3.♻️ Suggested conversion
- await should(service.bulkEdit(data, options)).be.rejectedWith(errors.BadRequestError); + await assert.rejects(service.bulkEdit(data, options), errors.BadRequestError);And remove the unused import:
-const should = require('should');ghost/core/test/unit/server/lib/package-json/parse.test.js (1)
1-2: Consider removing the unusedrequire('should')import.After the assertion conversions, the
shouldimport on line 2 appears unused in this file. This can be cleaned up now or in a follow-up PR.🧹 Proposed cleanup
const assert = require('node:assert/strict'); -require('should');ghost/core/test/unit/server/lib/package-json/read.test.js (1)
1-2: Consider removing the unusedrequire('should')import.Same as in
parse.test.js, theshouldimport is no longer used after these conversions.🧹 Proposed cleanup
const assert = require('node:assert/strict'); -require('should');ghost/core/test/unit/server/models/base/relations.test.js (1)
37-41: Optional: Remaining.should.eql()calls could be converted in a follow-up.Lines 37, 41, 72, and 76 still use
.should.eql(). These could be converted toassert.deepEqual()to fully remove theshoulddependency from this file, but fine to defer given this is an incremental migration.ghost/core/test/unit/frontend/services/routing/taxonomy-router.test.js (1)
50-64: Consider converting remaining.should.eql()calls for consistency.Several assertions in this test still use Should.js syntax (e.g.,
taxonomyRouter.mountRouter.args[0][1].should.eql(...)). For consistency with the PR's migration effort, these could also be converted toassert.deepEqual().ghost/core/test/unit/frontend/services/routing/static-routes-router.test.js (1)
54-54: Consider converting remaining.should.eql()calls for consistency.This file still has several
.should.eql()calls (e.g., lines 54, 76, 136, 140, 163, 167, 192, 196) that compare spy arguments to controllers. These could be converted toassert.deepEqual()in a follow-up for full consistency.ghost/core/test/unit/frontend/services/routing/collection-router.test.js (1)
59-71: Consider converting remaining.should.eql()calls for consistency.Several assertions still use Should.js syntax (e.g.,
mountRouteSpy.args[0][1].should.eql(controllers.collection)). These could be converted toassert.deepEqual()in a follow-up for full consistency with the PR's migration effort.ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (1)
44-44: Consider completing the Should.js conversion for consistency.Several Should.js assertions remain that could be converted to
assertExists():
- Line 44:
cacheStore.get(url).should.not.be.undefined;- Line 57:
cacheStore.get(url).should.not.be.undefined;- Line 79:
cacheStore.get(url).should.not.be.undefined;- Line 100:
cacheStore.get(url).should.not.be.undefined;These could be replaced with
assertExists(cacheStore.get(url))to align with the PR's goal of migrating away from Should.js.♻️ Proposed conversion to assertExists
-cacheStore.get(url).should.not.be.undefined; +assertExists(cacheStore.get(url));Apply this pattern at lines 44, 57, 79, and 100. Once complete, the
shouldimport on line 4 can be removed.Also applies to: 57-57, 79-79, 100-100
ghost/core/test/unit/frontend/helpers/get.test.js (1)
299-299: Consider converting these remaining.eql()calls.While this PR focuses on targeted conversions, these two
.eql()calls could also be migrated for consistency:
- Line 299: String comparison could use
assert.equal()- Line 437: Object comparison could use
assert.deepEqual()Feel free to address these in a follow-up if you prefer to keep this PR focused on the current scope.
♻️ Proposed conversions
- browseStub.firstCall.args[0].filter.should.eql(`published_at:<='${pubDate.toISOString()}'`); + assert.equal(browseStub.firstCall.args[0].filter, `published_at:<='${pubDate.toISOString()}'`);- browseStub.firstCall.args[0].context.member.should.eql(member); + assert.deepEqual(browseStub.firstCall.args[0].context.member, member);Also applies to: 437-437
ghost/core/test/unit/frontend/helpers/content.test.js (1)
3-3: Consider removing unusedshouldimport.The
shouldimport appears to be unused in this file now that all assertions have been migrated toassert. This can be cleaned up in this PR or deferred to a follow-up.🧹 Proposed cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should'); const hbs = require('../../../../core/frontend/services/theme-engine/engine');ghost/core/test/unit/frontend/helpers/img-url.test.js (1)
3-3: Consider removing unusedshouldimport.The
shouldimport appears to be unused in this file after the migration.🧹 Proposed cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/services/settings/settings-bread-service.test.js (1)
2-2: Minor: Inconsistent import style and unusedshould.Two minor observations:
- Line 2 uses
require('assert/strict')while most other files in this PR userequire('node:assert/strict'). Both work, butnode:prefix is the more modern convention.- The
shouldimport on line 7 appears unused after this migration.These are optional cleanups that can be addressed now or later.
🧹 Proposed cleanup
const sinon = require('sinon'); -const assert = require('assert/strict'); +const assert = require('node:assert/strict'); const mail = require('../../../../../core/server/services/mail'); const SettingsBreadService = require('../../../../../core/server/services/settings/settings-bread-service'); const urlUtils = require('../../../../../core/shared/url-utils.js'); const {mockManager} = require('../../../../utils/e2e-framework'); -const should = require('should'); const emailAddress = require('../../../../../core/server/services/email-address');Also applies to: 7-7
ghost/core/test/legacy/models/model-tags.test.js (1)
3-3: Consider removing unusedshouldimport.The
shouldimport appears unused after the migration.🧹 Proposed cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/meta/asset-url.test.js (1)
2-2: Consider removing unusedshouldimport.The
shouldimport appears unused after the migration.🧹 Proposed cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/helpers/excerpt.test.js (1)
3-3: Consider removing unusedshouldimport.The
shouldimport appears unused after the migration.🧹 Proposed cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should'); // Stuff we are testingghost/core/test/unit/server/services/settings/settings-utils.test.js (1)
3-3: Consider removing unusedshouldimport.After the assertion migrations, the
shouldimport appears to be unused in this file.🧹 Suggested cleanup
const assert = require('node:assert/strict'); const sinon = require('sinon'); -const should = require('should');ghost/core/test/unit/server/services/url/queue.test.js (1)
4-4: Consider removing unusedshouldimport.After the assertion migrations, the
shouldimport appears to be unused in this file.🧹 Suggested cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../../../utils/assertions'); const _ = require('lodash'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/e2e-api/admin/storage-adapter-switching.test.js (1)
3-3: Consider removing unusedshouldimport.After the assertion migrations in this file, the
shouldimport appears to be unused. Removing it would complete the migration for this file.♻️ Proposed fix
const assert = require('node:assert/strict'); const {assertExists} = require('../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/legacy/models/model-snippets.test.js (1)
3-3: Consider removing unusedshouldimport.After the assertion migrations in this file, the
shouldimport appears to be unused. Removing it would complete the migration for this file.♻️ Proposed fix
const assert = require('node:assert/strict'); const {assertExists} = require('../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/data/schema/schema.test.js (1)
3-3: Unusedshouldimport can be removed.The
shouldimport is no longer used in this file after the assertion conversions.♻️ Proposed fix
const assert = require('node:assert/strict'); const {assertExists} = require('../../../../utils/assertions'); -const should = require('should'); const _ = require('lodash');ghost/core/test/unit/server/services/themes/validate.test.js (1)
135-149: Swap the remainingshould.not.exist()for an assert-based null/undefined check.
This is the only Should.js usage in this file, so the conversion allows removing theshouldimport.♻️ Proposed change
- checkedTheme.should.not.exist(); + assert(checkedTheme == null);ghost/core/test/unit/server/models/set-is-roles.test.js (1)
5-5: Consider replacing the lodashisPlainObjectimport with a tiny local helper to keep the test self-contained and avoid an extra dependency.The proposed helper preserves exact semantics, including support for
null-prototype objects as lodash does.🔧 Suggested refactor (apply to all occurrences)
-const _ = require('lodash'); +const isPlainObject = (value) => { + if (!value || typeof value !== 'object') { + return false; + } + const proto = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +}; @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result)); @@ - assert(_.isPlainObject(result)); + assert(isPlainObject(result));Also applies to: 66-66, 71-71, 83-83, 95-95, 107-107, 119-119, 130-130, 141-141, 152-152
| const assert = require('node:assert/strict'); | ||
| const {assertExists} = require('../../../../utils/assertions'); | ||
| // We use the name input_password to match the helper for consistency: | ||
| const should = require('should'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused should import.
The should import on line 4 is no longer used after converting all assertions to assert.equal(). This should be removed to complete the migration.
🧹 Proposed fix to remove unused import
const assert = require('node:assert/strict');
const {assertExists} = require('../../../../utils/assertions');
-// We use the name input_password to match the helper for consistency:
-const should = require('should');
// Stuff we are testing
const input_password = require('../../../../../core/frontend/apps/private-blogging/lib/helpers/input_password');🤖 Prompt for AI Agents
In `@ghost/core/test/unit/frontend/apps/private-blogging/input-password.test.js`
around lines 1 - 4, Remove the unused "should" import by deleting the
require('should') statement (the "should" identifier) from the test file; ensure
there are no remaining references to the "should" symbol and that tests continue
to use the existing assert/assertExists calls.
Never necessary.
EvanHahn
force-pushed
the
more-should-destruction
branch
from
89d9deb to
3891144
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ghost/core/test/unit/frontend/services/card-assets.test.js`:
- Around line 1-2: Remove the unused should import by deleting the "const should
= require('should');" line from the top of the test file (the import shown in
the diff) so only the used "const assert = require('node:assert/strict');"
remains; ensure no other references to the should variable exist in
card-assets.test.js after removal.
In `@ghost/core/test/unit/server/services/permissions/parse-context.test.js`:
- Around line 1-3: Remove the unused should import by deleting the const should
= require('should'); line in the test file (ensure no remaining references to
should in this file); keep the assert and parseContext imports intact and run
the tests to verify nothing else depends on the should symbol.
🧹 Nitpick comments (18)
ghost/core/test/legacy/models/model-settings.test.js (1)
2-2: Remove unusedshouldimport.The
shouldmodule is still imported but no longer used in this file after the migration toassert. This can be removed.🧹 Suggested cleanup
const assert = require('node:assert/strict'); -const should = require('should'); const testUtils = require('../../utils');ghost/core/test/unit/frontend/helpers/excerpt.test.js (1)
1-3: Remove unusedshouldimport.The
shouldlibrary on line 3 is no longer used after converting the assertion toassert.equal. This import can be safely removed.🧹 Proposed cleanup
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should');ghost/core/test/unit/server/models/integration.test.js (1)
2-2: Unusedshouldimport can be removed.The
shouldimport is no longer used in this file after the assertion conversions. Consider removing it to complete the cleanup.🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/services/data/checks.test.js (1)
2-2: Remove unusedshouldimport.The
shouldmodule is no longer used anywhere in this file after the conversion toassert.deepEqual. This should be removed to complete the migration.🧹 Proposed fix
const assert = require('node:assert/strict'); -const should = require('should'); const {checks} = require('../../../../../core/frontend/services/data');ghost/core/test/unit/server/data/importer/importers/data/newsletters.test.js (1)
2-2: Remove unusedshouldimport.After converting the assertions to
assert.equal(), theshouldimport is no longer used anywhere in this file.♻️ Suggested fix
const assert = require('node:assert/strict'); -const should = require('should'); const NewslettersImporter = require('../../../../../../../core/server/data/importer/importers/data/newsletters-importer');ghost/core/test/unit/server/web/parent/middleware/ghost-locals.test.js (1)
3-4: Remove unusedshouldimport.The
shouldimport is no longer used after migrating assertions toassertand lodash. This should be removed to complete the Should.js removal.♻️ Proposed fix
const _ = require('lodash'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/services/link-tracking/link-click-tracking-service.test.js (2)
3-4: Consider usingnode:assert/strictfor consistency.The related file
link-redirect-repository.test.jsusesrequire('node:assert/strict')while this file usesrequire('assert/strict'). Both work identically, but thenode:prefix is the modern convention and makes it explicit that this is a built-in module.Suggested change
-const assert = require('assert/strict'); +const assert = require('node:assert/strict');
346-346: Opportunity to convert remainingshouldusage and remove the import.This line still uses Should.js. Per the PR's mapping, this could be converted to
assert.rejects(), which would allow removing theshouldimport entirely from this file. If this is intentionally deferred for a future PR, feel free to skip.Suggested change
- await should(service.bulkEdit(data, options)).be.rejectedWith(errors.BadRequestError); + await assert.rejects( + service.bulkEdit(data, options), + errors.BadRequestError + );Then remove the unused import:
-const should = require('should');ghost/core/test/unit/frontend/helpers/tags.test.js (1)
3-3: Unusedshouldimport can be removed.The
shouldimport is no longer used in this file after the assertion migration tonode:assert. Consider removing it to complete the cleanup.🧹 Proposed fix
const assert = require('node:assert/strict'); const {assertExists} = require('../../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/frontend/services/rendering/templates.test.js (1)
3-3: Unusedshouldimport can be removed.The
shouldmodule is still imported but no longer used anywhere in this file after the assertion conversions. Consider removing this dead import to complete the migration.const assert = require('node:assert/strict'); const {assertExists} = require('../../../../utils/assertions'); -const should = require('should'); const sinon = require('sinon');ghost/core/test/unit/server/lib/package-json/read.test.js (1)
1-2: Unusedshouldimport can be removed.Line 2 imports Should.js, but all assertions in this file have been converted to use
assert.deepEqualandassert.equal. Theshouldimport is now dead code.🧹 Suggested cleanup
const assert = require('node:assert/strict'); -require('should');ghost/core/test/unit/server/services/themes/list.test.js (3)
29-32: Incomplete conversion: Should.js assertion remains.Line 30 still uses Should.js syntax. This can be converted to match the pattern used elsewhere in this file.
♻️ Suggested conversion
it('getAll() returns all themes', function () { - themeList.getAll().should.be.an.Object().with.properties('casper', 'not-casper'); + const result = themeList.getAll(); + assert(_.isPlainObject(result)); + assert('casper' in result); + assert('not-casper' in result); assert.equal(Object.keys(themeList.getAll()).length, 2); });
34-40: Incomplete conversion: Should.js assertion remains on line 38.Line 38 still uses
should.not.eql. Consider converting or removing it since line 39's assertion already verifies the updated value (which implicitly proves it changed fromorigCasper).♻️ Suggested conversion
it('set() updates an existing theme', function () { const origCasper = _.cloneDeep(themeList.get('casper')); themeList.set('casper', {magic: 'update'}); - themeList.get('casper').should.not.eql(origCasper); + assert.notDeepEqual(themeList.get('casper'), origCasper); assert.deepEqual(themeList.get('casper'), {magic: 'update'}); });Or simply remove line 38 as it's redundant given the assertion on line 39.
3-3:shouldimport can be removed after converting remaining assertions.Once lines 30 and 38 are converted, this import will be unused and can be deleted.
ghost/core/test/unit/server/services/themes/validate.test.js (1)
139-149: Line 141 still uses Should.js syntax.The assertion
checkedTheme.should.not.exist()wasn't converted. Consider usingassert.fail('Expected promise to reject')or refactoring to useassert.rejects()for consistency with the PR objective.Option 1: Replace with assert.fail
return validate.check(testTheme.name, testTheme, {isZip: true}) .then((checkedTheme) => { - checkedTheme.should.not.exist(); + assert.fail('Expected promise to reject'); }).catch((error) => {Option 2: Refactor to use assert.rejects
- it('[failure] can handle a corrupt zip file', function () { + it('[failure] can handle a corrupt zip file', async function () { checkZipStub.rejects(new Error('invalid zip file')); formatStub.returns({results: {error: []}}); - return validate.check(testTheme.name, testTheme, {isZip: true}) - .then((checkedTheme) => { - checkedTheme.should.not.exist(); - }).catch((error) => { - assert(error instanceof Error); - assert.equal(error.message, 'invalid zip file'); - assert.equal(checkZipStub.calledOnce, true); - assert.equal(checkZipStub.calledWith(testTheme), true); - sinon.assert.notCalled(checkStub); - assert.equal(formatStub.calledOnce, false); - }); + await assert.rejects( + validate.check(testTheme.name, testTheme, {isZip: true}), + {message: 'invalid zip file'} + ); + assert.equal(checkZipStub.calledOnce, true); + assert.equal(checkZipStub.calledWith(testTheme), true); + sinon.assert.notCalled(checkStub); + assert.equal(formatStub.calledOnce, false); });Note: If all Should.js usages are removed, the
shouldimport on line 1 can also be removed.ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js (1)
870-884: Optional: Consider converting remaining.should.be.fulfilled()calls.These tests still use Should.js's
.should.be.fulfilled()pattern. For consistency with the PR's migration effort, these could be simplified since a fulfilled promise just needs to be awaited—if it rejects, the test will fail automatically.♻️ Suggested simplification
it('Sends emails when honeypot is not filled', async function () { const controller = createRouterController(); - await controller.sendMagicLink(req, res).should.be.fulfilled(); + await controller.sendMagicLink(req, res); assert.equal(sendEmailWithMagicLinkStub.calledOnce, true); }); it('Does not send emails when honeypot is filled', async function () { const controller = createRouterController(); req.body.honeypot = 'filled!'; - await controller.sendMagicLink(req, res).should.be.fulfilled(); + await controller.sendMagicLink(req, res); assert.equal(sendEmailWithMagicLinkStub.notCalled, true); });This would also allow removing
require('should');from line 1 if no other usages remain.ghost/core/test/unit/frontend/services/routing/static-routes-router.test.js (1)
54-54: Optional: Remaining.eql()calls could be converted in a follow-up.Several
should.eql(controllers.static/channel)assertions on spy arguments (lines 54, 76, 136, 140, 163, 167, 192, 196) appear convertible toassert.deepEqual. For example:-mountRouteSpy.args[0][1].should.eql(controllers.static); +assert.deepEqual(mountRouteSpy.args[0][1], controllers.static);This would allow removing the
shouldimport entirely. However, I understand this PR focuses on specific automated conversions and this can be deferred.ghost/core/test/unit/api/canary/utils/serializers/output/mapper.test.js (1)
229-229: Consider converting remainingshould.eqlfor consistency.This line still uses Should.js while all other similar assertions in this file have been converted to
assert.deepEqual. If converted, theshouldimport on line 3 could also be removed.♻️ Proposed fix
- mapped.should.eql(newsletter.toJSON()); + assert.deepEqual(mapped, newsletter.toJSON());And if no other
shouldusages remain:-const should = require('should');
| const assert = require('node:assert/strict'); | ||
| const should = require('should'); |
There was a problem hiding this comment.
Unused should import can be removed.
The should import on line 2 is no longer used anywhere in this file after converting all assertions to assert.deepEqual. Consider removing it to complete the migration.
Proposed fix
const assert = require('node:assert/strict');
-const should = require('should');📝 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 assert = require('node:assert/strict'); | |
| const should = require('should'); | |
| const assert = require('node:assert/strict'); |
🤖 Prompt for AI Agents
In `@ghost/core/test/unit/frontend/services/card-assets.test.js` around lines 1 -
2, Remove the unused should import by deleting the "const should =
require('should');" line from the top of the test file (the import shown in the
diff) so only the used "const assert = require('node:assert/strict');" remains;
ensure no other references to the should variable exist in card-assets.test.js
after removal.
| const assert = require('node:assert/strict'); | ||
| const should = require('should'); | ||
| const parseContext = require('../../../../../core/server/services/permissions/parse-context'); |
There was a problem hiding this comment.
Remove unused should import.
The should import is no longer used in this file after converting all assertions to assert.deepEqual. This leftover import should be removed to complete the migration.
🧹 Proposed fix
const assert = require('node:assert/strict');
-const should = require('should');
const parseContext = require('../../../../../core/server/services/permissions/parse-context');📝 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 assert = require('node:assert/strict'); | |
| const should = require('should'); | |
| const parseContext = require('../../../../../core/server/services/permissions/parse-context'); | |
| const assert = require('node:assert/strict'); | |
| const parseContext = require('../../../../../core/server/services/permissions/parse-context'); |
🤖 Prompt for AI Agents
In `@ghost/core/test/unit/server/services/permissions/parse-context.test.js`
around lines 1 - 3, Remove the unused should import by deleting the const should
= require('should'); line in the test file (ensure no remaining references to
should in this file); keep the assert and parseContext imports intact and run
the tests to verify nothing else depends on the should symbol.
1 participant
Replaced various Should.js usages. The majority of them:
a.should.equal(b)assert.equal(a, b)should(a).be.exactly(b)assert.equal(a, b)should(value).be.Array()assert(Array.isArray(value))should(fn).throw()assert.throws()should(fn).rejectedWith(err)assert.rejects(fn, err)should(value).eql([])assert.deepEqual(value, [])should(value).eql({})assert.deepEqual(value, {})should(value).not.be.undefined()assertExistsI also converted several
.eql()calls, but those can't be done automatically because there isn't an obvious analog inassert-land.