Repository navigation
fix: use fake timers in ReminderService.spec to prevent race condition - #1745
Conversation
The getDueReminders test captures the current time once with Date.now(), then calls the service which internally calls Date.now() again. If the system clock advances 1ms between these calls, the assertion fails intermittently. Using jest.useFakeTimers() freezes time, ensuring both calls see the identical timestamp. Closes #1713
📝 WalkthroughWalkthroughThe reminder service test imports ChangesReminder test timing
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/src/services/ReminderService.spec.ts">
<violation number="1" location="packages/shared/src/services/ReminderService.spec.ts:177">
P2: The fake-timer cleanup via `jest.useRealTimers()` will be skipped if any preceding assertion fails, leaking fake timers to subsequent tests. Consider wrapping the test body in a `try/finally` block, or better, use a scoped `afterEach` inside the `describe('getDueReminders')` block to guarantee cleanup regardless of assertion failures.
This is a low-risk concern today (the downstream tests use fixed-date strings), but it's a fragile pattern that can cause hard-to-debug flaky failures when the test suite evolves.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ach hook
The fake-timer cleanup via jest.useRealTimers() was previously placed at the end
of the test body, so if any assertion failed, the cleanup would be skipped and
fake timers would leak to subsequent tests. This fix adds a scoped afterEach
hook inside the describe('getDueReminders') block to guarantee cleanup regardless
of assertion failures, following Jest conventions.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Test-only change: uses fake timers to fix a flaky race condition in ReminderService tests. No production code or logic impacted.
Re-trigger cubic
|
🤖 I have created a release *beep* *boop* --- <details><summary>2.34.0</summary> ## [2.34.0](v2.33.1...v2.34.0) (2026-07-10) ### Features * **twitch:** use Promise.allSettled for per-event subscription error logging ([#1749](#1749)) ([6691305](6691305)) ### Bug Fixes * [#1699](#1699) ([eef5aee](eef5aee)) * **backend:** migrate webhooks to use canonical timingsafekey comparison ([#1747](#1747)) ([eef5aee](eef5aee)) * **backend:** wrap lastfm routes with asynchandler ([#1726](#1726)) ([ce51d86](ce51d86)) * **batch-move:** graceful attachment-fetch degradation + mid-loop client re-check ([#1750](#1750)) ([f21a0ce](f21a0ce)) * **bot:** approve @discordjs/opus install script — P0 music playback outage ([#1757](#1757)) ([9d894e4](9d894e4)) * **ci:** add missing packages field to pnpm-workspace.yaml ([#1760](#1760)) ([a4c585d](a4c585d)) * **ci:** remove pnpm shim from bundle-size workflow ([#1759](#1759)) ([eaf676f](eaf676f)) * **deploy:** increase validation timeout to 10min ([#1743](#1743)) ([07891ec](07891ec)) * **docker:** copy+chown [@prisma](https://github.com/prisma) engines in production-backend — P0 deploy pipeline blocker ([#1758](#1758)) ([a70d0e8](a70d0e8)) * eliminate mock state pollution in bot tests and remove resetMocks config ([#1741](#1741)) ([2e5fd94](2e5fd94)) * **frontend:** prevent state updates after unmount ([#1748](#1748)) ([f4e7c45](f4e7c45)) * pin file-type to resolve CI flake [#1740](#1740) ([#1753](#1753)) ([6b8e527](6b8e527)) * reduce Jest maxWorkers and add DB pool config for test stability ([#1751](#1751)) ([cfead33](cfead33)) * use fake timers in ReminderService.spec to prevent race condition ([#1745](#1745)) ([ba2908c](ba2908c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).



Summary
Fixes the flaky getDueReminders test that failed intermittently due to a race condition between two Date.now() calls ~1ms apart.
Root Cause
The test captured the current time with
const now = new Date()at line 150, then called the service method which internally calledDate.now()during execution. If the system clock advanced 1ms between these calls, the assertion.toBeLessThanOrEqual(now.getTime())would fail intermittently.Solution
Use
jest.useFakeTimers()to freeze time during the test, ensuring both timestamp calls see identical values. This is the standard pattern for Date-based tests and prevents any clock drift during execution.Verification
Ran the test suite 5 times in a row - all passed:
Closes #1713
Summary by cubic
Stabilizes the
getDueReminderstest inReminderServiceby freezing time withjest.useFakeTimers()and guaranteeing cleanup via anafterEachjest.useRealTimers()inside that suite. Eliminates the 1 msDate.now()race and prevents fake-timer leaks; closes #1713.Written for commit 97a8646. Summary will update on new commits.
Summary by CodeRabbit