Removed unnecessary dones from several tests#27580
Merged
Conversation
Contributor
WalkthroughThe changes replace callback- and Promise-chain test patterns across the repo with async/await. Many unit and integration tests remove 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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/models/post.test.js (1)
635-792: 🛠️ Refactor suggestion | 🟠 MajorDestroy-action tests are now grouped under
describe('Adding').Lines 736-791 cover
models.Post.permissible(..., 'destroy', ...)but live inside thedescribe('Adding')block. Looks like the previousdescribe('Destroying', ...)wrapper was removed but the innerit(...)s were kept and folded into Adding. This makes the test suite output misleading (a "destroy" failure will report under "Adding").Please either restore the dedicated
describe('Destroying', function () { ... })wrapper or move these two tests to wherever destroy-permissions belong.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/models/post.test.js` around lines 635 - 792, The destroy-permission tests that call models.Post.permissible(..., 'destroy', ...) are incorrectly nested inside describe('Adding'); restore a separate describe('Destroying', function () { ... }) block (or move the two it(...) cases named 'rejects if destroying a published post' and 'resolves if none of the above cases are true') so those tests are grouped under a dedicated "Destroying" describe; ensure the two tests retain their existing mock setup (mockPostObj, context) and assertions around models.Post.permissible, mockPostObj.get, and mockPostObj.related.
🧹 Nitpick comments (4)
ghost/core/test/legacy/site/dynamic-routing.test.js (1)
105-118: Redundant.expect(200)added.Line 107 already asserts
200; the new.expect(200)on line 111 duplicates it. Drop one of them.♻️ Proposed fix
const res = await request.get('/tag/getting-started/') - .expect(200) .expect('Content-Type', /html/) .expect('Content-Type', /html/) .expect('Cache-Control', testUtils.cacheRules.public) .expect(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/legacy/site/dynamic-routing.test.js` around lines 105 - 118, The test "should return HTML for valid route" contains a duplicated status assertion: remove the redundant .expect(200) in the request chain started by request.get('/tag/getting-started/') so the chain only asserts status 200 once (keep the first or the last .expect(200) and delete the other) in the test function to eliminate the duplicate assertion.ghost/core/test/unit/server/lib/package-json/read.test.js (1)
33-33: Remove unnecessaryawaiton synchronousremoveCallbackcalls.
tmp.dirSync({unsafeCleanup: true}).removeCallbackis a synchronous function returningvoid. Theawaitis unnecessary and misleading —await packagePath.removeCallback()simply resolves toundefinedimmediately. UsepackagePath.removeCallback();instead, or switch totmp.dir(...)(async API) if async cleanup is actually needed.Applies to lines: 33, 65, 93, 129, 157, 186, 218, 246, 275, 289, 303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/lib/package-json/read.test.js` at line 33, The tests call the synchronous tmp.dirSync().removeCallback via expressions like packagePath.removeCallback() but currently use unnecessary awaits; remove the await keywords from all calls to removeCallback (e.g., packagePath.removeCallback()) in the test file so the synchronous cleanup is invoked without awaiting, or if you intended async cleanup switch to tmp.dir(...) and await its async callback; update every occurrence listed (lines with packagePath.removeCallback and similar tempDir.removeCallback calls) accordingly.ghost/core/test/unit/server/lib/image/image-size.test.js (1)
629-649: Could useassert.rejectshere for consistency.The other failure tests in this file were converted to
assert.rejects(...). This try/catch block could follow the same pattern, with the additional benefit that an unexpected resolution is reported automatically:♻️ Proposed refactor
- let thrownError; - try { - await imageSize.getImageSizeFromStoragePath(url); - } catch (err) { - thrownError = err; - assertExists(err); - assert.equal((err instanceof errors.NotFoundError), true); - } - assert(thrownError); + await assert.rejects( + imageSize.getImageSizeFromStoragePath(url), + errors.NotFoundError + );🤖 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/image-size.test.js` around lines 629 - 649, Replace the manual try/catch in the "[failure] returns error if storage adapter errors" test with assert.rejects to mirror other failure tests: call await assert.rejects(() => imageSize.getImageSizeFromStoragePath(url), { instanceOf: errors.NotFoundError }) (or the equivalent predicate) so the test fails automatically on unexpected resolution and removes the thrownError/assertExists boilerplate; keep the createImageSize setup and the same url and storage stub, just change the assertion to use assert.rejects against imageSize.getImageSizeFromStoragePath.ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (1)
129-178: Consider simplifying to match thechannel.test.jspattern.The other refactored controller tests in this PR (e.g.
channel.test.js"page param too big") simply pass asinon.stub()asnext,awaitthe controller, and assert withsinon.assert.calledWith(next, sinon.match.instanceOf(errors.NotFoundError)). That avoids the manualPromiseplumbing here.♻️ Proposed simplification
- it('page param too big', async function () { - req.params.page = 6; - - fetchDataStub.withArgs({page: 6, slug: undefined, limit: postsPerPage}, res.routerOptions) - .resolves({ - posts: posts, - meta: { - pagination: { - pages: 5 - } - } - }); - - let called = false; - - await new Promise((resolve, reject) => { - const next = (err) => { - if (called) { - return; - } - - called = true; - - try { - assert.equal((err instanceof errors.NotFoundError), true); - sinon.assert.calledOnce(themeEngine.getActive); - sinon.assert.notCalled(security.string.safe); - sinon.assert.calledOnce(fetchDataStub); - sinon.assert.notCalled(renderStub); - sinon.assert.notCalled(ownsStub); - resolve(); - } catch (error) { - reject(error); - } - }; - - controllers.collection(req, res, next) - .then(() => { - if (!called) { - reject(new Error('Expected callback to be called with NotFoundError.')); - } - }) - .catch((error) => { - if (!called) { - reject(error); - } - }); - }); - assert.equal(called, true); - }); + it('page param too big', async function () { + req.params.page = 6; + + fetchDataStub.withArgs({page: 6, slug: undefined, limit: postsPerPage}, res.routerOptions) + .resolves({ + posts: posts, + meta: { + pagination: { + pages: 5 + } + } + }); + + const next = sinon.stub(); + await controllers.collection(req, res, next); + + sinon.assert.calledOnce(themeEngine.getActive); + sinon.assert.notCalled(security.string.safe); + sinon.assert.calledOnce(fetchDataStub); + sinon.assert.notCalled(renderStub); + sinon.assert.notCalled(ownsStub); + sinon.assert.calledWith(next, sinon.match.instanceOf(errors.NotFoundError)); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js` around lines 129 - 178, Replace the manual Promise plumbing in the "page param too big" test for controllers.collection with the simpler pattern used in channel.test.js: create a sinon.stub() for next, await controllers.collection(req, res, nextStub), then assert that nextStub was called with sinon.match.instanceOf(errors.NotFoundError) (sinon.assert.calledWith(nextStub, sinon.match.instanceOf(errors.NotFoundError))); keep the other assertions (sinon.assert.calledOnce(themeEngine.getActive), sinon.assert.notCalled(security.string.safe), sinon.assert.calledOnce(fetchDataStub), sinon.assert.notCalled(renderStub), sinon.assert.notCalled(ownsStub)) unchanged. This removes the manual called flag and Promise resolution/rejection logic and uses the next stub to verify the error was forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/unit/server/lib/image/image-size.test.js`:
- Line 464: Remove the unnecessary backslash escapes from the test titles that
wrap module names in backticks so the backticks render correctly; specifically,
update the three failing "it" descriptions that currently read like '[failure]
returns error if \`probe-image-size`\ module throws error' (and the similar ones
for `image-size`) by deleting the backslashes before the backticks in those
it(...) strings so they become '[failure] returns error if `probe-image-size`
module throws error' (and `image-size`) within the test file's describe/it
blocks.
- Around line 695-697: The test currently asserts only that
imageSize.getImageSizeFromStoragePath(url) rejects; update the assert.rejects
call to validate the error shape so the failure is from the expected source —
e.g., pass a second argument to assert.rejects that checks the error.message or
errorType (for example {message: /expected error text/} or {errorType:
'ExpectedErrorType'}) to the existing call to
imageSize.getImageSizeFromStoragePath in the test so it mirrors other failure
tests in this file.
---
Outside diff comments:
In `@ghost/core/test/unit/server/models/post.test.js`:
- Around line 635-792: The destroy-permission tests that call
models.Post.permissible(..., 'destroy', ...) are incorrectly nested inside
describe('Adding'); restore a separate describe('Destroying', function () { ...
}) block (or move the two it(...) cases named 'rejects if destroying a published
post' and 'resolves if none of the above cases are true') so those tests are
grouped under a dedicated "Destroying" describe; ensure the two tests retain
their existing mock setup (mockPostObj, context) and assertions around
models.Post.permissible, mockPostObj.get, and mockPostObj.related.
---
Nitpick comments:
In `@ghost/core/test/legacy/site/dynamic-routing.test.js`:
- Around line 105-118: The test "should return HTML for valid route" contains a
duplicated status assertion: remove the redundant .expect(200) in the request
chain started by request.get('/tag/getting-started/') so the chain only asserts
status 200 once (keep the first or the last .expect(200) and delete the other)
in the test function to eliminate the duplicate assertion.
In
`@ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js`:
- Around line 129-178: Replace the manual Promise plumbing in the "page param
too big" test for controllers.collection with the simpler pattern used in
channel.test.js: create a sinon.stub() for next, await
controllers.collection(req, res, nextStub), then assert that nextStub was called
with sinon.match.instanceOf(errors.NotFoundError)
(sinon.assert.calledWith(nextStub,
sinon.match.instanceOf(errors.NotFoundError))); keep the other assertions
(sinon.assert.calledOnce(themeEngine.getActive),
sinon.assert.notCalled(security.string.safe),
sinon.assert.calledOnce(fetchDataStub), sinon.assert.notCalled(renderStub),
sinon.assert.notCalled(ownsStub)) unchanged. This removes the manual called flag
and Promise resolution/rejection logic and uses the next stub to verify the
error was forwarded.
In `@ghost/core/test/unit/server/lib/image/image-size.test.js`:
- Around line 629-649: Replace the manual try/catch in the "[failure] returns
error if storage adapter errors" test with assert.rejects to mirror other
failure tests: call await assert.rejects(() =>
imageSize.getImageSizeFromStoragePath(url), { instanceOf: errors.NotFoundError
}) (or the equivalent predicate) so the test fails automatically on unexpected
resolution and removes the thrownError/assertExists boilerplate; keep the
createImageSize setup and the same url and storage stub, just change the
assertion to use assert.rejects against imageSize.getImageSizeFromStoragePath.
In `@ghost/core/test/unit/server/lib/package-json/read.test.js`:
- Line 33: The tests call the synchronous tmp.dirSync().removeCallback via
expressions like packagePath.removeCallback() but currently use unnecessary
awaits; remove the await keywords from all calls to removeCallback (e.g.,
packagePath.removeCallback()) in the test file so the synchronous cleanup is
invoked without awaiting, or if you intended async cleanup switch to
tmp.dir(...) and await its async callback; update every occurrence listed (lines
with packagePath.removeCallback and similar tempDir.removeCallback calls)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc531627-09e5-42d9-ab27-ac94fd415aa9
📒 Files selected for processing (27)
ghost/core/test/legacy/site/dynamic-routing.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/get.test.jsghost/core/test/unit/frontend/meta/schema.test.jsghost/core/test/unit/frontend/meta/structured-data.test.jsghost/core/test/unit/frontend/services/data/fetch-data.test.jsghost/core/test/unit/frontend/services/routing/controllers/channel.test.jsghost/core/test/unit/frontend/services/routing/controllers/collection.test.jsghost/core/test/unit/frontend/services/routing/controllers/previews.test.jsghost/core/test/unit/frontend/services/rss/cache.test.jsghost/core/test/unit/frontend/services/rss/generate-feed.test.jsghost/core/test/unit/frontend/services/rss/renderer.test.jsghost/core/test/unit/frontend/web/routers/serve-favicon.test.jsghost/core/test/unit/server/data/db/backup.test.jsghost/core/test/unit/server/data/exporter/index.test.jsghost/core/test/unit/server/data/importer/handlers/image.test.jsghost/core/test/unit/server/data/importer/index.test.jsghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.jsghost/core/test/unit/server/lib/image/image-size.test.jsghost/core/test/unit/server/lib/package-json/read.test.jsghost/core/test/unit/server/models/post.test.jsghost/core/test/unit/server/models/session.test.jsghost/core/test/unit/server/models/user.test.jsghost/core/test/unit/server/services/permissions/can-this.test.jsghost/core/test/unit/server/services/permissions/index.test.jsghost/core/test/unit/server/services/permissions/providers.test.jsghost/core/test/unit/server/services/url/url-service.test.js
EvanHahn
force-pushed
the
remove-useless-dones
branch
from
586a195 to
fecbe0b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (1)
125-128: Prefer asserting on a captured theme stub here.Calling
themeEngine.getActive()again in the assertion bumps the stub count after you've already checked it once, which makes the test a bit brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js` around lines 125 - 128, The test currently calls themeEngine.getActive() again in the assertion which increments the stub call count; capture the active theme stub once after setup and reuse it in assertions (e.g. assign themeEngine.getActive() to a variable) and then assert theme.updateTemplateOptions.withArgs({data: {config: {posts_per_page: 3}}}) was called and that security.string.safe was not called; update references to use the captured stub instead of calling themeEngine.getActive() again to avoid changing call counts and brittle behavior in controllers.collection tests.
🤖 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/unit/frontend/services/routing/controllers/collection.test.js`:
- Around line 125-128: The test currently calls themeEngine.getActive() again in
the assertion which increments the stub call count; capture the active theme
stub once after setup and reuse it in assertions (e.g. assign
themeEngine.getActive() to a variable) and then assert
theme.updateTemplateOptions.withArgs({data: {config: {posts_per_page: 3}}}) was
called and that security.string.safe was not called; update references to use
the captured stub instead of calling themeEngine.getActive() again to avoid
changing call counts and brittle behavior in controllers.collection tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73db67b7-ef9b-49f0-829d-d2d4c25cf651
📒 Files selected for processing (27)
ghost/core/test/legacy/site/dynamic-routing.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/get.test.jsghost/core/test/unit/frontend/meta/schema.test.jsghost/core/test/unit/frontend/meta/structured-data.test.jsghost/core/test/unit/frontend/services/data/fetch-data.test.jsghost/core/test/unit/frontend/services/routing/controllers/channel.test.jsghost/core/test/unit/frontend/services/routing/controllers/collection.test.jsghost/core/test/unit/frontend/services/routing/controllers/previews.test.jsghost/core/test/unit/frontend/services/rss/cache.test.jsghost/core/test/unit/frontend/services/rss/generate-feed.test.jsghost/core/test/unit/frontend/services/rss/renderer.test.jsghost/core/test/unit/frontend/web/routers/serve-favicon.test.jsghost/core/test/unit/server/data/db/backup.test.jsghost/core/test/unit/server/data/exporter/index.test.jsghost/core/test/unit/server/data/importer/handlers/image.test.jsghost/core/test/unit/server/data/importer/index.test.jsghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.jsghost/core/test/unit/server/lib/image/image-size.test.jsghost/core/test/unit/server/lib/package-json/read.test.jsghost/core/test/unit/server/models/post.test.jsghost/core/test/unit/server/models/session.test.jsghost/core/test/unit/server/models/user.test.jsghost/core/test/unit/server/services/permissions/can-this.test.jsghost/core/test/unit/server/services/permissions/index.test.jsghost/core/test/unit/server/services/permissions/providers.test.jsghost/core/test/unit/server/services/url/url-service.test.js
✅ Files skipped from review due to trivial changes (9)
- ghost/core/test/unit/server/data/db/backup.test.js
- ghost/core/test/unit/frontend/meta/schema.test.js
- ghost/core/test/unit/frontend/meta/structured-data.test.js
- ghost/core/test/unit/server/models/session.test.js
- ghost/core/test/unit/frontend/web/routers/serve-favicon.test.js
- ghost/core/test/unit/server/data/importer/handlers/image.test.js
- ghost/core/test/unit/frontend/services/routing/controllers/channel.test.js
- ghost/core/test/unit/server/services/permissions/providers.test.js
- ghost/core/test/unit/frontend/helpers/get.test.js
🚧 Files skipped from review as they are similar to previous changes (10)
- ghost/core/test/unit/frontend/helpers/comments.test.js
- ghost/core/test/unit/server/services/permissions/index.test.js
- ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
- ghost/core/test/unit/server/data/exporter/index.test.js
- ghost/core/test/unit/server/models/user.test.js
- ghost/core/test/unit/frontend/services/rss/generate-feed.test.js
- ghost/core/test/unit/server/lib/package-json/read.test.js
- ghost/core/test/unit/server/services/permissions/can-this.test.js
- ghost/core/test/unit/server/data/importer/index.test.js
- ghost/core/test/legacy/site/dynamic-routing.test.js
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.
no ref
This is a test-only change. I think it's useful on its own, but will make upcoming changes (like migrating to Vitest) a bit easier.