Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR modifies CI scripts and upgrades Vitest packages in package.json, tightens Vitest coverage to include only Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/job-manager/test/job-manager.test.js (1)
467-467: Avoid swallowing allremoveJobfailures in this test path.Line 467 catches every rejection, which can hide real regressions. Please narrow this to expected failure(s) only.
Suggested tightening
- await jobManager.removeJob('failed-oneoff').catch(() => {}); + await jobManager.removeJob('failed-oneoff').catch((err) => { + assert.match(err.message, /not found|does not exist|missing/i); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-manager/test/job-manager.test.js` at line 467, The test currently swallows all rejections from jobManager.removeJob('failed-oneoff') which can hide failures; change the test to only accept the expected failure instead of catching every error—either assert the promise rejects with the specific error/message using your test framework's rejects/toThrow (targeting jobManager.removeJob) or catch and rethrow unexpected errors by matching the error type/message (e.g., only silence a known "JobNotFound" or "CannotRemoveRunningJob" error), so the test fails on real regressions while tolerating the intended failure.packages/domain-events/test/DomainEvents.test.js (1)
91-93: Consider adding a comment explaining thesleep(0)usage.When tracking is disabled,
allSettled()resolves immediately (since#processedCount >=#dispatchCount`` is0 >= 0). The `sleep(0)` before it is what actually allows the async-wrapped handler to complete. A brief comment would clarify this subtlety.📝 Suggested comment
DomainEvents.dispatch(new TestEvent('No tracking')); + // sleep(0) allows the async-wrapped handler to complete since + // allSettled() resolves immediately when tracking is disabled await sleep(0); await DomainEvents.allSettled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain-events/test/DomainEvents.test.js` around lines 91 - 93, The test relies on an async-wrapped handler completing before calling DomainEvents.allSettled(), but the sleep(0) call is subtle and deserves an explanatory comment; update the test around DomainEvents.dispatch(new TestEvent('No tracking')); await sleep(0); await DomainEvents.allSettled(); to add a brief comment stating that sleep(0) yields to the event loop so the async handler can run (since processedCount >= dispatchCount is already true when tracking is disabled), referencing DomainEvents.dispatch, sleep(0), and DomainEvents.allSettled to clarify why the no-op delay is required.packages/domain-events/lib/DomainEvents.js (2)
105-113: Add JSDoc for consistency with other public methods.These public static methods lack JSDoc documentation, unlike the other methods in this class. For consistency and discoverability, consider adding brief documentation.
📝 Suggested JSDoc
+ /** + * Enable or disable dispatch/processed tracking. Test-only. + * `@param` {boolean} enabled + */ static setTrackingEnabledForTest(enabled) { this.#trackingEnabled = enabled; } + /** + * Clear the await queue and reset dispatch/processed counts. Test-only. + */ static resetTrackingStateForTest() { this.#awaitQueue = []; this.#dispatchCount = 0; this.#processedCount = 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain-events/lib/DomainEvents.js` around lines 105 - 113, The two public static test helpers setTrackingEnabledForTest and resetTrackingStateForTest in DomainEvents.js are missing JSDoc; add brief JSDoc blocks above each method (mirroring the style used by the other public methods in this class) describing purpose, parameters (e.g., enabled: boolean for setTrackingEnabledForTest), side effects (what internal fields like `#trackingEnabled`, `#awaitQueue`, `#dispatchCount`, `#processedCount` are changed), and that they are intended for testing only to improve discoverability and consistency.
41-43: Consider removing empty else branches.These empty else branches with only comments don't add functional value and create visual noise. The
ifcondition already ensures the guarded code runs only when tracking is enabled. If the intent is documentation, a single comment above or within theifblock would suffice without introducing no-op branches.♻️ Suggested simplification (example for subscribe)
if (this.#trackingEnabled) { this.#onProcessed(); - } else { - // Tracking is disabled outside tests. }Also applies to: 66-68, 100-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain-events/lib/DomainEvents.js` around lines 41 - 43, Remove the empty else branches in DomainEvents.js (e.g., the no-op else blocks after the tracking-enabled checks found around the subscribe and other methods) — delete the else { // Tracking is disabled ... } blocks and, if documentation is needed, move a single explanatory comment above the corresponding if (isTrackingEnabled()) guard or inside the if-block; ensure function names like subscribe, publish, and any other methods with those empty else branches are updated accordingly so no no-op branches remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/domain-events/lib/DomainEvents.js`:
- Around line 105-113: The two public static test helpers
setTrackingEnabledForTest and resetTrackingStateForTest in DomainEvents.js are
missing JSDoc; add brief JSDoc blocks above each method (mirroring the style
used by the other public methods in this class) describing purpose, parameters
(e.g., enabled: boolean for setTrackingEnabledForTest), side effects (what
internal fields like `#trackingEnabled`, `#awaitQueue`, `#dispatchCount`,
`#processedCount` are changed), and that they are intended for testing only to
improve discoverability and consistency.
- Around line 41-43: Remove the empty else branches in DomainEvents.js (e.g.,
the no-op else blocks after the tracking-enabled checks found around the
subscribe and other methods) — delete the else { // Tracking is disabled ... }
blocks and, if documentation is needed, move a single explanatory comment above
the corresponding if (isTrackingEnabled()) guard or inside the if-block; ensure
function names like subscribe, publish, and any other methods with those empty
else branches are updated accordingly so no no-op branches remain.
In `@packages/domain-events/test/DomainEvents.test.js`:
- Around line 91-93: The test relies on an async-wrapped handler completing
before calling DomainEvents.allSettled(), but the sleep(0) call is subtle and
deserves an explanatory comment; update the test around
DomainEvents.dispatch(new TestEvent('No tracking')); await sleep(0); await
DomainEvents.allSettled(); to add a brief comment stating that sleep(0) yields
to the event loop so the async handler can run (since processedCount >=
dispatchCount is already true when tracking is disabled), referencing
DomainEvents.dispatch, sleep(0), and DomainEvents.allSettled to clarify why the
no-op delay is required.
In `@packages/job-manager/test/job-manager.test.js`:
- Line 467: The test currently swallows all rejections from
jobManager.removeJob('failed-oneoff') which can hide failures; change the test
to only accept the expected failure instead of catching every error—either
assert the promise rejects with the specific error/message using your test
framework's rejects/toThrow (targeting jobManager.removeJob) or catch and
rethrow unexpected errors by matching the error type/message (e.g., only silence
a known "JobNotFound" or "CannotRemoveRunningJob" error), so the test fails on
real regressions while tolerating the intended failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3775fa16-0979-4ea2-b87a-4466bebe50e5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.jsonpackages/bookshelf-pagination/test/pagination.test.jspackages/config/lib/config.jspackages/config/test/config.test.jspackages/domain-events/lib/DomainEvents.jspackages/domain-events/test/DomainEvents.test.jspackages/job-manager/test/job-manager.test.jspackages/metrics/test/metrics.test.jspackages/request/lib/request.jspackages/request/test/request.test.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/request/test/request.test.js`:
- Around line 262-264: The test restores process.env.NODE_ENV by assigning
originalNodeEnv, but when originalNodeEnv is undefined that assignment sets the
string "undefined" and pollutes other tests; update both finally blocks that
reference originalNodeEnv so they check if originalNodeEnv === undefined and
call delete process.env.NODE_ENV in that case, otherwise set
process.env.NODE_ENV = originalNodeEnv; reference the variables originalNodeEnv
and process.env.NODE_ENV and the existing finally blocks surrounding the test
promises to locate where to change the restoration logic.
- Around line 250-265: The test title says retry should not be auto-set when
NODE_ENV is unset but it only asserts success; update the test around
request(url, {}) and requestMock so it explicitly verifies no retry metadata was
sent: after deleting process.env.NODE_ENV and before asserting status, assert
that the outgoing request did not include any retry option/header (e.g. use
nock's matchHeader or inspect requestMock.interceptors[0].req.headers to ensure
headers like 'x-retry'/'x-retries' or any retry-related field are absent, or
otherwise confirm no retry option was attached to the request object), then keep
the existing isDone() and statusCode assertions. Ensure you reference
request(url, {}), requestMock and the NODE_ENV deletion in the updated
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 897d6fd7-32f5-4a33-8d4b-f8d936497d1f
📒 Files selected for processing (3)
nx.jsonpackage.jsonpackages/request/test/request.test.js
✅ Files skipped from review due to trivial changes (1)
- nx.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
+250
to
+265
| it('[success] does not auto-set retry when NODE_ENV is unset', function () { | ||
| const url = 'http://some-website.com/unset-env-endpoint/'; | ||
| const requestMock = nock('http://some-website.com') | ||
| .get('/unset-env-endpoint/') | ||
| .reply(200, 'ok'); | ||
| const originalNodeEnv = process.env.NODE_ENV; | ||
|
|
||
| delete process.env.NODE_ENV; | ||
|
|
||
| return request(url, {}).then((res) => { | ||
| assert.equal(requestMock.isDone(), true); | ||
| assert.equal(res.statusCode, 200); | ||
| }).finally(() => { | ||
| process.env.NODE_ENV = originalNodeEnv; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test title and assertions are misaligned for retry behavior
This test does not verify its claim. On Line 250, the title says retry is not auto-set when NODE_ENV is unset, but assertions at Line 259-262 only validate a successful response. That would pass even if retry were incorrectly injected.
Suggested fix
- it('[success] does not auto-set retry when NODE_ENV is unset', function () {
- const url = 'http://some-website.com/unset-env-endpoint/';
- const requestMock = nock('http://some-website.com')
- .get('/unset-env-endpoint/')
- .reply(200, 'ok');
+ it('[success] does not auto-set retry when NODE_ENV is unset', function () {
+ const requestModule = rewire('../lib/request');
+ const gotStub = sinon.stub().resolves({
+ statusCode: 200,
+ body: 'ok'
+ });
const originalNodeEnv = process.env.NODE_ENV;
+ requestModule.__set__('got', gotStub);
+ requestModule.__get__('defaultOptions').dnsLookup = () => {};
delete process.env.NODE_ENV;
- return request(url, {}).then((res) => {
- assert.equal(requestMock.isDone(), true);
+ return requestModule('http://some-website.com/unset-env-endpoint/').then((res) => {
assert.equal(res.statusCode, 200);
+ assert.equal(Object.prototype.hasOwnProperty.call(gotStub.firstCall.args[1], 'retry'), false);
}).finally(() => {
- process.env.NODE_ENV = originalNodeEnv;
+ if (originalNodeEnv === undefined) {
+ delete process.env.NODE_ENV;
+ } else {
+ process.env.NODE_ENV = originalNodeEnv;
+ }
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/request/test/request.test.js` around lines 250 - 265, The test title
says retry should not be auto-set when NODE_ENV is unset but it only asserts
success; update the test around request(url, {}) and requestMock so it
explicitly verifies no retry metadata was sent: after deleting
process.env.NODE_ENV and before asserting status, assert that the outgoing
request did not include any retry option/header (e.g. use nock's matchHeader or
inspect requestMock.interceptors[0].req.headers to ensure headers like
'x-retry'/'x-retries' or any retry-related field are absent, or otherwise
confirm no retry option was attached to the request object), then keep the
existing isDone() and statusCode assertions. Ensure you reference request(url,
{}), requestMock and the NODE_ENV deletion in the updated assertions.
Comment on lines
+262
to
+264
| }).finally(() => { | ||
| process.env.NODE_ENV = originalNodeEnv; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the test file to see the context around lines 262-264 and 322-324
cat -n packages/request/test/request.test.js | sed -n '250,270p'Repository: TryGhost/framework
Length of output: 1073
🏁 Script executed:
# Also check the context around line 322-324
cat -n packages/request/test/request.test.js | sed -n '310,330p'Repository: TryGhost/framework
Length of output: 1043
🏁 Script executed:
# Check how originalNodeEnv is initialized
cat -n packages/request/test/request.test.js | sed -n '1,50p'Repository: TryGhost/framework
Length of output: 2016
🏁 Script executed:
node <<'EOF'
// Test what happens when assigning undefined to process.env
const originalValue = process.env.TEST_VAR;
delete process.env.TEST_VAR;
// Check if it's deleted
console.log('After delete, TEST_VAR is:', process.env.TEST_VAR);
// Now assign undefined
process.env.TEST_VAR = undefined;
// Check what it becomes
console.log('After assigning undefined, TEST_VAR is:', process.env.TEST_VAR);
console.log('Type:', typeof process.env.TEST_VAR);
console.log('Value === "undefined":', process.env.TEST_VAR === 'undefined');
EOFRepository: TryGhost/framework
Length of output: 189
🏁 Script executed:
# Check for other similar patterns in the test file where process.env is restored
rg -n "process\.env\.[A-Z_]+ = original" packages/request/test/request.test.jsRepository: TryGhost/framework
Length of output: 173
🏁 Script executed:
# Also check for the delete pattern to see full scope
rg -B5 -A5 "delete process\.env" packages/request/test/request.test.jsRepository: TryGhost/framework
Length of output: 462
Restore NODE_ENV safely to avoid test pollution from string "undefined"
Assigning undefined to process.env.NODE_ENV coerces it to the string "undefined" rather than unsetting the variable. At lines 263 and 323, originalNodeEnv is undefined (captured after delete process.env.NODE_ENV), causing the restoration to set process.env.NODE_ENV = "undefined" instead of deleting it. This leaks state across tests.
Suggested fix
- }).finally(() => {
- process.env.NODE_ENV = originalNodeEnv;
- });
+ }).finally(() => {
+ if (originalNodeEnv === undefined) {
+ delete process.env.NODE_ENV;
+ } else {
+ process.env.NODE_ENV = originalNodeEnv;
+ }
+ });Also applies to: 322-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/request/test/request.test.js` around lines 262 - 264, The test
restores process.env.NODE_ENV by assigning originalNodeEnv, but when
originalNodeEnv is undefined that assignment sets the string "undefined" and
pollutes other tests; update both finally blocks that reference originalNodeEnv
so they check if originalNodeEnv === undefined and call delete
process.env.NODE_ENV in that case, otherwise set process.env.NODE_ENV =
originalNodeEnv; reference the variables originalNodeEnv and
process.env.NODE_ENV and the existing finally blocks surrounding the test
promises to locate where to change the restoration logic.
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.
Summary
vitestand@vitest/coverage-v8to4.1.0yarn.lock@tryghost/job-managertests under Vitest v4Testing
yarn testNotes