Updated sinon to v19 #23866
Updated sinon to v19 #23866
Conversation
ErisDS
commented
Jun 16, 2025
ErisDS
commented
- Stepping through the major versions one-by-one
- Stepping through the major versions one-by-one
WalkthroughThis change updates the version of the Possibly related PRs
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- Sinon v19 now fakes ALL timers by default including nextTick/queueMicroTask - This breaks async operations in tests as they depend on nextTick to resolve - Used explicit toFake array to exclude nextTick while keeping timer control - Alternative shouldAdvanceTime:true considered but rejected for determinism - Affects tests that mix timer mocking with HTTP requests/async operations
![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
🧹 Nitpick comments (3)
ghost/core/test/e2e-api/admin/actions.test.js (2)
25-27: Fake-timer scope could be broader & deterministicYou’re now explicitly listing the functions to fake, but you dropped
setImmediate,clearImmediateandnextTick(faked by default in Sinon ≤18).
If any code under test schedules work with these, the ticks you perform won’t advance it, resulting in flakiness that only appears on CI.While touching this line, you could also pin the start time for full repeatability.
-const clock = sinon.useFakeTimers({ - toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] -}); +const clock = sinon.useFakeTimers({ + now: Date.now(), // deterministic baseline + toFake: [ + 'setTimeout', 'clearTimeout', + 'setInterval', 'clearInterval', + 'setImmediate', 'clearImmediate', + 'nextTick', + 'Date' + ] +});
141-142: Duplicate restore – move tofinallyor rely on theafterhook
clock.restore()is already implicitly executed bysinon.restore()in theafterhook.
Calling it here is harmless but redundant; if an assertion throws before this line, the call is skipped anyway.Consider either:
- Removing these two lines and letting the
afterhook handle cleanup, or- Wrapping the test body in
try … finally { clock.restore(); }to guarantee restoration on failure.Leaving as-is is safe but slightly noisy.
ghost/core/test/e2e-api/members/signin.test.js (1)
321-324: IncludesetImmediate/nextTickin fake timers for full coverageSame remark as earlier: with Sinon 19 you need to list everything you expect to be faked.
Adding the two omissions avoids surprises if the rate-limit implementation ever switches tosetImmediate/process.nextTick.-clock = sinon.useFakeTimers({ - now: new Date(), - toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] -}); +clock = sinon.useFakeTimers({ + now: Date.now(), + toFake: [ + 'setTimeout', 'clearTimeout', + 'setInterval', 'clearInterval', + 'setImmediate', 'clearImmediate', + 'nextTick', + 'Date' + ] +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/test/e2e-api/admin/actions.test.js(2 hunks)ghost/core/test/e2e-api/admin/links.test.js(1 hunks)ghost/core/test/e2e-api/members/feedback.test.js(1 hunks)ghost/core/test/e2e-api/members/signin.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- ghost/core/test/e2e-api/members/feedback.test.js
- ghost/core/test/e2e-api/admin/links.test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-api/members/signin.test.js (3)
ghost/core/test/e2e-api/admin/links.test.js (2)
clock(20-20)sinon(3-3)ghost/core/test/e2e-api/admin/actions.test.js (2)
clock(25-27)sinon(2-2)ghost/core/test/e2e-api/members/feedback.test.js (2)
clock(13-13)sinon(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
| clock = sinon.useFakeTimers({ | ||
| now: startDate, | ||
| toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
startDate is shared between tests – take a fresh copy per run
startDate is declared once outside the beforeEach; after the first test it no longer reflects the real start of subsequent tests.
Although you pass the timestamp into useFakeTimers, re-using the mutated object risks hidden coupling between tests.
-beforeEach(async function () {
- // Remove ms precision (not supported by MySQL)
- startDate.setMilliseconds(0);
- clock = sinon.useFakeTimers({
- now: startDate,
+beforeEach(async function () {
+ // Fresh baseline, no ms precision (not supported by MySQL)
+ const now = new Date();
+ now.setMilliseconds(0);
+ clock = sinon.useFakeTimers({
+ now,
toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date']
});
});This keeps each test isolated and easier to reason about.
📝 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.
| clock = sinon.useFakeTimers({ | |
| now: startDate, | |
| toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] | |
| }); | |
| beforeEach(async function () { | |
| // Fresh baseline, no ms precision (not supported by MySQL) | |
| const now = new Date(); | |
| now.setMilliseconds(0); | |
| clock = sinon.useFakeTimers({ | |
| now, | |
| toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In ghost/core/test/e2e-api/members/signin.test.js around lines 188 to 191, the
startDate object is declared once outside the beforeEach hook and reused across
tests, causing shared mutable state that can lead to coupling between tests. To
fix this, move the creation or cloning of startDate inside the beforeEach so
that each test gets a fresh, independent copy before calling
sinon.useFakeTimers. This ensures test isolation and prevents side effects from
previous tests.
• Sinon v19 fake timers create different Date object instances • Test was comparing Date objects created before/after fake timer setup • Fixed by creating expected date using same faked Date constructor • Ensures both dates are created with same timer context
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/test/unit/server/adapters/scheduling/SchedulingDefault.test.js (2)
254-256: Store the fake-clock instance for clearer teardown & controlInside this test you create a second fake clock but don’t keep a reference:
sinon.restore(); sinon.useFakeTimers({ shouldAdvanceTime: true });Because the instance isn’t assigned, you lose programmatic control (
runAll(),tick(), etc.) and rely on real passage of time. Capturing the handle also makes the finalsinon.restore()unnecessary and avoids accidental leaks if another stub is added later.-sinon.restore(); -sinon.useFakeTimers({ - shouldAdvanceTime: true -}); +sinon.restore(); +const pingClock = sinon.useFakeTimers({ shouldAdvanceTime: true }); +// … test body … +pingClock.restore(); // explicit & local – avoids double restore confusion
337-339: Duplicate pattern – apply same explicit clock handleSame comment as above: keep the returned fake-timer so the test remains deterministic and self-contained.
-sinon.useFakeTimers({ - shouldAdvanceTime: true -}); +const retryClock = sinon.useFakeTimers({ shouldAdvanceTime: true }); +// … test … +retryClock.restore();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/test/unit/server/adapters/scheduling/SchedulingDefault.test.js(1 hunks)ghost/core/test/unit/server/models/user.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/unit/server/models/user.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (1)
ghost/core/test/unit/server/adapters/scheduling/SchedulingDefault.test.js (1)
16-18: Nice upgrade – automatic time-advancing clock simplifies testsSwitching to
shouldAdvanceTime: truemeans all thesetTimeout/setIntervalcalls progress in real time without manualclock.tick(...). This fits the test suite which already uses real-time polling in several places.No further concerns here.
