Merged
Conversation
This replaces `should.eql` with `assert.equal` and `assert.deepEqual`.
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThis pull request replaces Should.js assertion calls with Node.js Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
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 (1)
ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js (1)
874-874:⚠️ Potential issue | 🟡 MinorMissed
should-style assertions on lines 874 and 890.These two lines still use
should.be.trueand were not converted as part of this migration:service.queries.aggregateEmailStats.calledWith('memberId').should.be.true; service.queries.aggregateMemberStats.calledWith('memberId').should.be.true;Proposed fix
- service.queries.aggregateEmailStats.calledWith('memberId').should.be.true; + assert.equal(service.queries.aggregateEmailStats.calledWith('memberId'), true);- service.queries.aggregateMemberStats.calledWith('memberId').should.be.true; + assert.equal(service.queries.aggregateMemberStats.calledWith('memberId'), true);Also applies to: 890-890
🤖 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` at line 874, Two assertions still use the old should-style chaining; update them to use expect() assertions: replace the expression on service.queries.aggregateEmailStats (calledWith('memberId')) and the one on service.queries.aggregateMemberStats (calledWith('memberId')) with expect(service.queries.aggregateEmailStats.calledWith('memberId')).to.be.true and expect(service.queries.aggregateMemberStats.calledWith('memberId')).to.be.true respectively so the tests use the migrated expect-style assertions.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/integration/importer/v2.test.js`:
- Around line 688-689: The two identical assertions comparing users[1].slug to
exportData.data.users[0].slug are duplicated; update one of them in the test
file (v2.test.js) so it validates a different user property (for example replace
the second duplicate assertion with an assertion that users[1].email equals
exportData.data.users[0].email or users[1].name equals
exportData.data.users[0].name) to ensure the test checks both slug and another
distinct field; locate the assertions by the symbols users and
exportData.data.users in the test and change the second assert.equal
accordingly.
In `@ghost/core/test/legacy/api/admin/redirects.test.js`:
- Line 2: The file imports the legacy assertion library via "const should =
require('should');" but there are no should-style assertions used in
redirects.test.js; remove this unused import to clean up the test file (delete
the require('should') line where "should" is declared) and run tests to ensure
no code path depends on the "should" symbol.
In `@ghost/core/test/legacy/api/admin/schedules.test.js`:
- Line 4: Remove the unused `should` import in the test file by deleting the
`const should = require('should');` statement in
ghost/core/test/legacy/api/admin/schedules.test.js (the `should` identifier and
its require call are not used anywhere in this file); after removal run the test
suite to ensure no implicit dependency on `should` remains.
In `@ghost/core/test/legacy/api/content/posts.test.js`:
- Line 3: The test file imports the legacy assertion library via the const
should = require('should'); statement but no `should`-style assertions remain,
so remove the unused import to clean up the file; locate the top-level `should`
require statement and delete it (or replace with a needed assertion import if
tests require one), then run the test suite to verify no references to the
`should` variable remain.
In `@ghost/core/test/unit/api/canary/utils/serializers/output/mapper.test.js`:
- Line 229: The test currently uses assert.equal(mapped, newsletter.toJSON())
which checks reference equality instead of deep structural equality; update the
assertion in the test (the line using assert.equal with mapped and
newsletter.toJSON()) to use assert.deepEqual so the mapper result is compared
deeply to newsletter.toJSON() preserving the original should.eql semantics.
In `@ghost/core/test/unit/server/adapters/scheduling/scheduling-default.test.js`:
- Line 2: Remove the unused CommonJS import "should" (the require('should')
statement) from the test file since there are no should-style assertions left;
locate the top-level require('should') expression in scheduling-default.test.js
and delete it, then run the test suite to confirm there are no remaining
references to the symbol.
In `@ghost/core/test/unit/server/data/exporter/index.test.js`:
- Around line 54-55: The test has inconsistent assertion styles: one assertion
uses assert.equal while adjacent assertions still use the older should.have.eql
form (e.g., queryMock.select.callCount.should.have.eql(expectedCallCount) and
the occurrences around the second block flagged). Replace those should.have.eql
usages with the same assert.equal form (or assert.strictEqual if preferred) to
match the converted assertion style; specifically update assertions that
reference queryMock.select.callCount and any other callCount checks in the same
test to use assert.equal(queryMock.select.callCount, expectedCallCount) (or
assert.strictEqual) so all assertions are consistent.
In
`@ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.js`:
- Line 874: Two assertions still use the old should-style chaining; update them
to use expect() assertions: replace the expression on
service.queries.aggregateEmailStats (calledWith('memberId')) and the one on
service.queries.aggregateMemberStats (calledWith('memberId')) with
expect(service.queries.aggregateEmailStats.calledWith('memberId')).to.be.true
and
expect(service.queries.aggregateMemberStats.calledWith('memberId')).to.be.true
respectively so the tests use the migrated expect-style assertions.
In `@ghost/core/test/unit/server/services/verification-trigger.test.js`:
- Line 5: Remove the now-unused require('should') import from the top of the
verification-trigger.test.js file: delete the standalone require('should')
statement so no dead/unused assertion library is imported; ensure no other code
relies on "should" style assertions in this test (e.g., any should.* calls)
before removing.
In `@ghost/core/test/unit/shared/config/loader.test.js`:
- Line 2: Remove the unused "should" import from the test file: delete the line
that requires 'should' (the const should = require('should'); at the top of
ghost/core/test/unit/shared/config/loader.test.js) since there are no
should-style assertions remaining; run tests to ensure nothing else relied on
that import and commit the cleaned file.
🧹 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/legacy/api/admin/redirects.test.js`: - Line 2: The file imports the legacy assertion library via "const should = require('should');" but there are no should-style assertions used in redirects.test.js; remove this unused import to clean up the test file (delete the require('should') line where "should" is declared) and run tests to ensure no code path depends on the "should" symbol. In `@ghost/core/test/legacy/api/admin/schedules.test.js`: - Line 4: Remove the unused `should` import in the test file by deleting the `const should = require('should');` statement in ghost/core/test/legacy/api/admin/schedules.test.js (the `should` identifier and its require call are not used anywhere in this file); after removal run the test suite to ensure no implicit dependency on `should` remains. In `@ghost/core/test/legacy/api/content/posts.test.js`: - Line 3: The test file imports the legacy assertion library via the const should = require('should'); statement but no `should`-style assertions remain, so remove the unused import to clean up the file; locate the top-level `should` require statement and delete it (or replace with a needed assertion import if tests require one), then run the test suite to verify no references to the `should` variable remain. In `@ghost/core/test/unit/server/adapters/scheduling/scheduling-default.test.js`: - Line 2: Remove the unused CommonJS import "should" (the require('should') statement) from the test file since there are no should-style assertions left; locate the top-level require('should') expression in scheduling-default.test.js and delete it, then run the test suite to confirm there are no remaining references to the symbol. In `@ghost/core/test/unit/server/data/exporter/index.test.js`: - Around line 54-55: The test has inconsistent assertion styles: one assertion uses assert.equal while adjacent assertions still use the older should.have.eql form (e.g., queryMock.select.callCount.should.have.eql(expectedCallCount) and the occurrences around the second block flagged). Replace those should.have.eql usages with the same assert.equal form (or assert.strictEqual if preferred) to match the converted assertion style; specifically update assertions that reference queryMock.select.callCount and any other callCount checks in the same test to use assert.equal(queryMock.select.callCount, expectedCallCount) (or assert.strictEqual) so all assertions are consistent. In `@ghost/core/test/unit/server/services/verification-trigger.test.js`: - Line 5: Remove the now-unused require('should') import from the top of the verification-trigger.test.js file: delete the standalone require('should') statement so no dead/unused assertion library is imported; ensure no other code relies on "should" style assertions in this test (e.g., any should.* calls) before removing. In `@ghost/core/test/unit/shared/config/loader.test.js`: - Line 2: Remove the unused "should" import from the test file: delete the line that requires 'should' (the const should = require('should'); at the top of ghost/core/test/unit/shared/config/loader.test.js) since there are no should-style assertions remaining; run tests to ensure nothing else relied on that import and commit the cleaned file.ghost/core/test/unit/shared/config/loader.test.js (1)
2-2: Unusedshouldimport can be removed.No remaining
should-style assertions in 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/shared/config/loader.test.js` at line 2, Remove the unused "should" import from the test file: delete the line that requires 'should' (the const should = require('should'); at the top of ghost/core/test/unit/shared/config/loader.test.js) since there are no should-style assertions remaining; run tests to ensure nothing else relied on that import and commit the cleaned file.ghost/core/test/legacy/api/content/posts.test.js (1)
3-3: Unusedshouldimport can be removed.No remaining
should-style assertions in 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/legacy/api/content/posts.test.js` at line 3, The test file imports the legacy assertion library via the const should = require('should'); statement but no `should`-style assertions remain, so remove the unused import to clean up the file; locate the top-level `should` require statement and delete it (or replace with a needed assertion import if tests require one), then run the test suite to verify no references to the `should` variable remain.ghost/core/test/unit/server/services/verification-trigger.test.js (1)
5-5: Unusedshouldimport can be removed.No remaining
should-style assertions in this file. Therequire('should')on line 5 is now dead code.Proposed fix
-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/verification-trigger.test.js` at line 5, Remove the now-unused require('should') import from the top of the verification-trigger.test.js file: delete the standalone require('should') statement so no dead/unused assertion library is imported; ensure no other code relies on "should" style assertions in this test (e.g., any should.* calls) before removing.ghost/core/test/unit/server/adapters/scheduling/scheduling-default.test.js (1)
2-2: Unusedshouldimport can be removed.No remaining
should-style assertions in 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/adapters/scheduling/scheduling-default.test.js` at line 2, Remove the unused CommonJS import "should" (the require('should') statement) from the test file since there are no should-style assertions left; locate the top-level require('should') expression in scheduling-default.test.js and delete it, then run the test suite to confirm there are no remaining references to the symbol.ghost/core/test/unit/server/data/exporter/index.test.js (1)
54-55: Inconsistent migration: adjacent line still usesshould.have.eql.Line 54 was converted to
assert.equal, but line 55 (queryMock.select.callCount.should.have.eql(expectedCallCount)) uses the same pattern and was left unconverted. Same applies to lines 101–102.Proposed fix
- queryMock.select.callCount.should.have.eql(expectedCallCount); + assert.equal(queryMock.select.callCount, expectedCallCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/data/exporter/index.test.js` around lines 54 - 55, The test has inconsistent assertion styles: one assertion uses assert.equal while adjacent assertions still use the older should.have.eql form (e.g., queryMock.select.callCount.should.have.eql(expectedCallCount) and the occurrences around the second block flagged). Replace those should.have.eql usages with the same assert.equal form (or assert.strictEqual if preferred) to match the converted assertion style; specifically update assertions that reference queryMock.select.callCount and any other callCount checks in the same test to use assert.equal(queryMock.select.callCount, expectedCallCount) (or assert.strictEqual) so all assertions are consistent.ghost/core/test/legacy/api/admin/redirects.test.js (1)
2-2:shouldimport appears unused.No remaining
should-style assertions are visible in this file. Consider removing this import.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/legacy/api/admin/redirects.test.js` at line 2, The file imports the legacy assertion library via "const should = require('should');" but there are no should-style assertions used in redirects.test.js; remove this unused import to clean up the test file (delete the require('should') line where "should" is declared) and run tests to ensure no code path depends on the "should" symbol.ghost/core/test/legacy/api/admin/schedules.test.js (1)
4-4:shouldimport appears unused.No remaining
should-style assertions are visible in this file. Consider removing this import.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/legacy/api/admin/schedules.test.js` at line 4, Remove the unused `should` import in the test file by deleting the `const should = require('should');` statement in ghost/core/test/legacy/api/admin/schedules.test.js (the `should` identifier and its require call are not used anywhere in this file); after removal run the test suite to ensure no implicit dependency on `should` remains.
Comment on lines
+688
to
+689
| assert.equal(users[1].slug, exportData.data.users[0].slug); | ||
| assert.equal(users[1].slug, exportData.data.users[0].slug); |
Contributor
There was a problem hiding this comment.
Duplicate assertion: lines 688 and 689 are identical.
Both lines assert the same thing (users[1].slug === exportData.data.users[0].slug). This is likely a pre-existing copy-paste issue, but one of these should probably verify a different property (e.g., email or name).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/integration/importer/v2.test.js` around lines 688 - 689, The
two identical assertions comparing users[1].slug to
exportData.data.users[0].slug are duplicated; update one of them in the test
file (v2.test.js) so it validates a different user property (for example replace
the second duplicate assertion with an assertion that users[1].email equals
exportData.data.users[0].email or users[1].name equals
exportData.data.users[0].name) to ensure the test checks both slug and another
distinct field; locate the assertions by the symbols users and
exportData.data.users in the test and change the second assert.equal
accordingly.
ghost/core/test/unit/api/canary/utils/serializers/output/mapper.test.js
Outdated
Show resolved
Hide resolved
…r.test.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
should.eqlwithassert.equalandassert.deepEqual.