Merged
Conversation
Contributor
WalkthroughThis pull request updates numerous test files to replace Should.js-style assertions with Node's built-in assert, Sinon assertion helpers, or equivalent explicit checks. Changes are limited to test code (assertion style and occasional assertion semantics refinements) and do not modify production code or public API signatures. Affected tests include integration, legacy, unit, and service tests across multiple modules. Possibly related PRs
Suggested reviewers
🚥 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/legacy/models/model-posts.test.jsTip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/integration/exporter/exporter.test.js`:
- Around line 114-118: The current assertion using assert(...) with
Object.keys(exportedBodyLatest().db[0].data).every(...) yields an unhelpful
failure message; update the test to iterate the keys returned by
exportedBodyLatest().db[0].data and assert each one exists in exportData.data
(or compute a list of missing keys and assert that list is empty), e.g. loop
over exportedBodyLatest().db[0].data keys and call
assert(Object.hasOwnProperty.call(exportData.data, key), `missing key ${key}`)
so failures clearly indicate which key is absent.
In `@ghost/core/test/unit/frontend/meta/og-image.test.js`:
- Line 2: Remove the unused require('should') import at the top of the test file
(the stray "const should = require('should');" statement) since all assertions
now use assert; locate the require call and delete that line so no unused
variable remains and run tests to confirm nothing else depends on should in
og-image.test.js.
In `@ghost/core/test/unit/frontend/services/theme-engine/active.test.js`:
- Line 2: The test file contains an unused import "should" causing dead code;
remove the require('should') line (the "should" identifier) from active.test.js
and ensure any assertions use the existing assertion style in the file (e.g.,
assert/expect) so no references to "should" remain; run the unit tests to
confirm nothing else depends on that import.
In `@ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js`:
- Line 4: The test file contains an unused import "const should =
require('should');" — remove that unused require from
ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (or if
assertions were intended to use "should", update the test assertions to
reference the imported should variable); in short, delete the unused "should"
declaration or convert assertions to use it so the "should" symbol is actually
used.
In
`@ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js`:
- Around line 873-874: Replace the two sinon.assert usages with the file's
existing assert.equal style for consistency: change the sinon.assert.calledOnce
on service.queries.aggregateEmailStats to an assert.equal that checks
service.queries.aggregateEmailStats.calledOnce equals true, and change the
sinon.assert.calledWith check to an assert.equal that checks
service.queries.aggregateEmailStats.calledWith('memberId') equals true; this
keeps assertions consistent with other tests in this file while still validating
the same behavior.
In
`@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js`:
- Line 3: Remove the now-unused assertion import by deleting the top-level
require for "should" (the const should = require('should'); statement) from the
test file; ensure no other references to the identifier should remain and run
the unit tests to confirm nothing else depends on it (target the
member-welcome-email-renderer test that previously used Should.js).
In `@ghost/core/test/unit/server/services/permissions/index.test.js`:
- Line 3: Remove the unused "should" require at the top of the test file: delete
the const should = require('should'); statement in
ghost/core/test/unit/server/services/permissions/index.test.js and ensure the
file relies only on existing assert/assertExists calls; run the unit tests to
verify nothing else depends on the removed symbol.
🧹 Nitpick comments (7)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/exporter/exporter.test.js`: - Around line 114-118: The current assertion using assert(...) with Object.keys(exportedBodyLatest().db[0].data).every(...) yields an unhelpful failure message; update the test to iterate the keys returned by exportedBodyLatest().db[0].data and assert each one exists in exportData.data (or compute a list of missing keys and assert that list is empty), e.g. loop over exportedBodyLatest().db[0].data keys and call assert(Object.hasOwnProperty.call(exportData.data, key), `missing key ${key}`) so failures clearly indicate which key is absent. In `@ghost/core/test/unit/frontend/meta/og-image.test.js`: - Line 2: Remove the unused require('should') import at the top of the test file (the stray "const should = require('should');" statement) since all assertions now use assert; locate the require call and delete that line so no unused variable remains and run tests to confirm nothing else depends on should in og-image.test.js. In `@ghost/core/test/unit/frontend/services/theme-engine/active.test.js`: - Line 2: The test file contains an unused import "should" causing dead code; remove the require('should') line (the "should" identifier) from active.test.js and ensure any assertions use the existing assertion style in the file (e.g., assert/expect) so no references to "should" remain; run the unit tests to confirm nothing else depends on that import. In `@ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js`: - Line 4: The test file contains an unused import "const should = require('should');" — remove that unused require from ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (or if assertions were intended to use "should", update the test assertions to reference the imported should variable); in short, delete the unused "should" declaration or convert assertions to use it so the "should" symbol is actually used. In `@ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js`: - Around line 873-874: Replace the two sinon.assert usages with the file's existing assert.equal style for consistency: change the sinon.assert.calledOnce on service.queries.aggregateEmailStats to an assert.equal that checks service.queries.aggregateEmailStats.calledOnce equals true, and change the sinon.assert.calledWith check to an assert.equal that checks service.queries.aggregateEmailStats.calledWith('memberId') equals true; this keeps assertions consistent with other tests in this file while still validating the same behavior. In `@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js`: - Line 3: Remove the now-unused assertion import by deleting the top-level require for "should" (the const should = require('should'); statement) from the test file; ensure no other references to the identifier should remain and run the unit tests to confirm nothing else depends on it (target the member-welcome-email-renderer test that previously used Should.js). In `@ghost/core/test/unit/server/services/permissions/index.test.js`: - Line 3: Remove the unused "should" require at the top of the test file: delete the const should = require('should'); statement in ghost/core/test/unit/server/services/permissions/index.test.js and ensure the file relies only on existing assert/assertExists calls; run the unit tests to verify nothing else depends on the removed symbol.ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js (1)
3-3: Unusedshouldimport can be removed.After replacing the last Should.js assertion on line 243,
shouldis no longer referenced anywhere in this file. Removing it would complete the migration for this file.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js` at line 3, Remove the now-unused assertion import by deleting the top-level require for "should" (the const should = require('should'); statement) from the test file; ensure no other references to the identifier should remain and run the unit tests to confirm nothing else depends on it (target the member-welcome-email-renderer test that previously used Should.js).ghost/core/test/unit/frontend/services/theme-engine/active.test.js (1)
2-2:shouldimport appears unused after migration.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/services/theme-engine/active.test.js` at line 2, The test file contains an unused import "should" causing dead code; remove the require('should') line (the "should" identifier) from active.test.js and ensure any assertions use the existing assertion style in the file (e.g., assert/expect) so no references to "should" remain; run the unit tests to confirm nothing else depends on that import.ghost/core/test/unit/frontend/meta/og-image.test.js (1)
2-2:shouldimport appears unused after migration.All assertions in this file now use
assert. Theshouldrequire on line 2 can be removed.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/meta/og-image.test.js` at line 2, Remove the unused require('should') import at the top of the test file (the stray "const should = require('should');" statement) since all assertions now use assert; locate the require call and delete that line so no unused variable remains and run tests to confirm nothing else depends on should in og-image.test.js.ghost/core/test/unit/server/services/permissions/index.test.js (1)
3-3:shouldimport appears unused after migration.All assertions in this file now use
assertorassertExists. Theshouldrequire on line 3 can be removed.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/permissions/index.test.js` at line 3, Remove the unused "should" require at the top of the test file: delete the const should = require('should'); statement in ghost/core/test/unit/server/services/permissions/index.test.js and ensure the file relies only on existing assert/assertExists calls; run the unit tests to verify nothing else depends on the removed symbol.ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (1)
4-4:shouldimport appears unused after migration.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js` at line 4, The test file contains an unused import "const should = require('should');" — remove that unused require from ghost/core/test/unit/server/lib/image/cached-image-size-from-url.test.js (or if assertions were intended to use "should", update the test assertions to reference the imported should variable); in short, delete the unused "should" declaration or convert assertions to use it so the "should" symbol is actually used.ghost/core/test/integration/exporter/exporter.test.js (1)
114-118: Bareassert()produces unhelpful failure messages when a key is missing.If this assertion fails, you'll get
AssertionError: false != truewith no indication of which key fromexportedBodyLatest()is absent inexportData.data. Consider adding a message or using a loop withassert()per key.♻️ Suggested improvement for better failure diagnostics
- assert( - Object.keys(exportedBodyLatest().db[0].data).every(key => ( - Object.hasOwnProperty.call(exportData.data, key) - )) - ); + for (const key of Object.keys(exportedBodyLatest().db[0].data)) { + assert( + Object.hasOwnProperty.call(exportData.data, key), + `Expected exportData.data to contain key "${key}"` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/exporter/exporter.test.js` around lines 114 - 118, The current assertion using assert(...) with Object.keys(exportedBodyLatest().db[0].data).every(...) yields an unhelpful failure message; update the test to iterate the keys returned by exportedBodyLatest().db[0].data and assert each one exists in exportData.data (or compute a list of missing keys and assert that list is empty), e.g. loop over exportedBodyLatest().db[0].data keys and call assert(Object.hasOwnProperty.call(exportData.data, key), `missing key ${key}`) so failures clearly indicate which key is absent.ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js (1)
873-874: Minor style inconsistency — not blocking.These two tests use
sinon.assert.calledOnce/sinon.assert.calledWith, while the rest of this file uses theassert.equal(stub.calledOnce, true)pattern. Both work correctly;sinon.assertactually provides better error messages on failure. Just noting the inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js` around lines 873 - 874, Replace the two sinon.assert usages with the file's existing assert.equal style for consistency: change the sinon.assert.calledOnce on service.queries.aggregateEmailStats to an assert.equal that checks service.queries.aggregateEmailStats.calledOnce equals true, and change the sinon.assert.calledWith check to an assert.equal that checks service.queries.aggregateEmailStats.calledWith('memberId') equals true; this keeps assertions consistent with other tests in this file while still validating the same behavior.
EvanHahn
force-pushed
the
more-should-removals
branch
from
e21ca60 to
c0b9bec
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/permissions/can-this.test.js (1)
2-2: Remove the unusedshouldimport.The
shouldlibrary is no longer used in this file—all.should-style assertions have been replaced. Only test description strings contain the word "should" as natural English text.Proposed fix
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/permissions/can-this.test.js` at line 2, Remove the unused import "const should = require('should');" from the test file since no .should assertions are present; locate the top-level require statement that defines the symbol "should" and delete it so the file no longer imports the unused should library.ghost/core/test/integration/exporter/exporter.test.js (1)
113-117: Consider adding a failure message for easier debugging.The bare
assert(...)won't indicate which key is missing on failure. A small improvement:Suggested improvement
- assert( - Object.keys(exportedBodyLatest().db[0].data).every(key => ( - Object.hasOwnProperty.call(exportData.data, key) - )) - ); + for (const key of Object.keys(exportedBodyLatest().db[0].data)) { + assert(Object.hasOwnProperty.call(exportData.data, key), `exportData.data should have key "${key}"`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/exporter/exporter.test.js` around lines 113 - 117, The assertion in the test using exportedBodyLatest() and exportData is bare and won’t show which key is missing on failure; change the check to compute the missing keys (e.g., filter Object.keys(exportedBodyLatest().db[0].data] to those where Object.hasOwnProperty.call(exportData.data, key) is false) and assert that no missing keys remain, passing a failure message that includes the missing key(s) (e.g., join the missingKeys array) so the test failure clearly shows which keys are absent.ghost/core/test/legacy/models/model-users.test.js (1)
462-462: Missed should.js assertion — still usesshould.not.eql.This is the only remaining should.js usage in the file. Converting it would also allow removing the
shouldimport on line 4.Proposed fix
- user.get('password').should.not.eql(userData.password); + assert.notEqual(user.get('password'), userData.password);And remove the now-unused import:
-const should = require('should');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/legacy/models/model-users.test.js` at line 462, Replace the remaining should.js assertion by using the project's chai expect: change the call that asserts the stored password differs (user.get('password').should.not.eql(userData.password)) to use expect(user.get('password')).to.not.equal(userData.password) (or .to.not.eql for deep equality if preferred), and then remove the now-unused should import; update references to user.get('password') and userData.password accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/integration/exporter/exporter.test.js`:
- Around line 113-117: The assertion in the test using exportedBodyLatest() and
exportData is bare and won’t show which key is missing on failure; change the
check to compute the missing keys (e.g., filter
Object.keys(exportedBodyLatest().db[0].data] to those where
Object.hasOwnProperty.call(exportData.data, key) is false) and assert that no
missing keys remain, passing a failure message that includes the missing key(s)
(e.g., join the missingKeys array) so the test failure clearly shows which keys
are absent.
In `@ghost/core/test/legacy/models/model-users.test.js`:
- Line 462: Replace the remaining should.js assertion by using the project's
chai expect: change the call that asserts the stored password differs
(user.get('password').should.not.eql(userData.password)) to use
expect(user.get('password')).to.not.equal(userData.password) (or .to.not.eql for
deep equality if preferred), and then remove the now-unused should import;
update references to user.get('password') and userData.password accordingly.
In `@ghost/core/test/unit/server/services/permissions/can-this.test.js`:
- Line 2: Remove the unused import "const should = require('should');" from the
test file since no .should assertions are present; locate the top-level require
statement that defines the symbol "should" and delete it so the file no longer
imports the unused should library.
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.
This test-only change should have no user impact.
This replaces the following Should.js assertions with
node:assert:.should.be.a.Date.should.be.a.Function.should.be.a.String.should.be.aboveOrEqual.should.be.rejectedWith.should.be.true.should.containDeep.should.endWith.should.have.length.should.have.size.should.not.be.undefined.should.not.equal.should.not.have.keys.should.not.throwError