Skip to content

test(desktop): pin the clock in the session-row timestamp test - #88876

Open
jackulau wants to merge 1 commit into
NousResearch:mainfrom
jackulau:fix/desktop-session-row-timestamp-midnight-flake
Open

test(desktop): pin the clock in the session-row timestamp test#88876
jackulau wants to merge 1 commit into
NousResearch:mainfrom
jackulau:fix/desktop-session-row-timestamp-midnight-flake

Conversation

@jackulau

Copy link
Copy Markdown
Contributor

What does this PR do?

Removes a time-of-day flake from session-row.test.tsx that fails CI for any PR whose
check:test:ui shard happens to run within five minutes of local midnight.

The test derives its fixture from the wall clock:

const startedAt = Math.floor(Date.now() / 1000) - 5 * 60
...
expect(age.getAttribute('aria-label')).toMatch(/^5m, Today at /)

"Five minutes ago" is only today when the run does not straddle a day boundary. Between
00:00 and 00:05 local, startedAt lands in the previous day, formatMessageTimestamp
correctly returns the yesterday label, and the assertion fails:

AssertionError: expected '5m, Yesterday at 11:56 PM' to match /^5m, Today at /

This is not theoretical. It took down JS & TS checks / apps/desktop / check:test:ui:shard-2of3
at 00:01 UTC on an unrelated desktop PR of mine
(run 32082698353),
in a shard where the other 1401 tests passed. Every PR that lands in that window pays for it,
and because the window is five minutes wide the failure looks random rather than reproducible.

The production formatter is not changed, deliberately. Rendering "Yesterday at 11:56 PM"
for a session started five minutes before midnight is the correct thing to do, and the
assertion is about the composition of the label (relative age plus absolute time), not about
which day it names. The bug is that the test hard-codes one side of a boundary it never meant
to test.

The fix pins the clock to local noon before deriving the timestamp. Only Date is faked
(toFake: ['Date']) so the component's own timers, the running arc and the tooltip open
delay, keep running for real; the neighbouring tooltip tests that call vi.advanceTimersByTime
are untouched. The describe block gains the useRealTimers teardown its sibling block
already had, so a pinned clock cannot leak into a later test.

Local noon rather than a UTC instant is deliberate: the offset is applied by the runner's own
timezone, so new Date(2026, 2, 5, 12, 0, 0) is midday in whatever zone CI runs in, and five
minutes earlier is the same day everywhere.

Related Issue

No issue: found by reading the CI failure on one of my own PRs. Happy to file one first if
maintainers prefer that for flakes.

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/chat/sidebar/session-row.test.tsx: pin the clock with
    vi.useFakeTimers({ toFake: ['Date'] }) plus vi.setSystemTime(...) before deriving
    startedAt in exposes the exact session time through a focusable Tip trigger, and add a
    vi.useRealTimers() teardown to the enclosing describe. A comment records why the
    boundary matters, so the pin is not mistaken for ceremony and removed later.

How to Test

  1. The test passes as written:

    cd apps/desktop
    npx vitest run src/app/chat/sidebar/session-row.test.tsx
    

    Test Files 1 passed · Tests 11 passed (11)

  2. Proof the pin is what makes it deterministic. Move the pinned instant into the failing
    window, changing new Date(2026, 2, 5, 12, 0, 0) to new Date(2026, 2, 5, 0, 2, 0), and
    re-run:

    × exposes the exact session time through a focusable Tip trigger
    AssertionError: expected '5m, Yesterday at 11:57 PM' to match /^5m, Today at /
    Tests  1 failed | 10 passed (11)
    

    That is the CI failure reproduced exactly, on demand, from a clock offset alone: same
    assertion, same message shape, only the minute differs. Restore noon and all 11 pass again.
    This also demonstrates the test still genuinely exercises the label: it is pinned, not
    loosened.

  3. npx eslint src/app/chat/sidebar/session-row.test.tsx is clean.
    npx tsc -p . --noEmit exits 0.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate. Nothing open touches this test's timestamp assertion; the only other open PR on this file, fix(desktop): collapse sidebar session-row actions track (#75331) #75487, changes the actions track and does not contain this test
  • My PR contains only changes related to this fix/feature (no unrelated commits): one commit, one file, 15 added lines
  • I've run pytest tests/ -q and all tests pass. N/A here, this is a TypeScript-only change; the affected vitest file is run above and the whole shard is exercised by CI
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features). The change is to a test; step 2 above is its mutation proof
  • I've tested on my platform: Windows 11 Pro 26200, Node via the repo's desktop workspace

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings). The comment in the test explains the boundary; no user-facing docs affected
  • N/A, no config keys added or changed
  • N/A, no architecture or workflow change
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide. The pin uses a local-time Date constructor rather than a UTC instant precisely so it is timezone-independent, which is what makes it correct on every runner
  • N/A, no tool descriptions or schemas changed

Screenshots / Logs

The CI failure this removes, from
run 32082698353
at 00:02 UTC:

 FAIL  |ui| src/app/chat/sidebar/session-row.test.tsx > SidebarSessionRow > exposes the exact session time through a focusable Tip trigger
AssertionError: expected '5m, Yesterday at 11:56 PM' to match /^5m, Today at /
 Test Files  1 failed | 167 passed (168)
      Tests  1 failed | 1401 passed (1402)

The same failure, reproduced locally on demand by moving the pin into the window:

AssertionError: expected '5m, Yesterday at 11:57 PM' to match /^5m, Today at /
      Tests  1 failed | 10 passed (11)

The sidebar row's Tip trigger test derives its fixture from the wall clock:

    const startedAt = Math.floor(Date.now() / 1000) - 5 * 60
    ...
    expect(age.getAttribute('aria-label')).toMatch(/^5m, Today at /)

"Five minutes ago" is only today when the run does not straddle local
midnight. Between 00:00 and 00:05 the timestamp falls into the previous
day, formatMessageTimestamp correctly returns the yesterday label, and
the test fails on a day boundary it was never written to exercise:

    AssertionError: expected '5m, Yesterday at 11:56 PM'
                    to match /^5m, Today at /

This is a real CI failure, not a theoretical one - it took down a
check:test:ui shard on an unrelated desktop PR at 00:01 UTC, and it will
do so for any PR whose shard happens to land in that five-minute window.

Pin the clock to local noon before deriving the timestamp so the fixture
can never cross a day boundary. Only Date is faked (toFake: ['Date']),
so the component's own timers - the running arc and the tooltip open
delay - keep running for real; the neighbouring tooltip tests that
advance timers are unaffected. The describe block gains the
useRealTimers teardown its sibling already had.

The production formatter is not changed: rendering "Yesterday at 11:56
PM" for a session started five minutes before midnight is correct, and
the assertion is about the label's composition, not about which day it
names.
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Aug 18, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review; please use your judgment.

Nice flake fix with an unusual degree of care for a test-only change: vi.useFakeTimers({ toFake: ['Date'] }) pins the clock selectively so the timestamp assertion becomes deterministic while the component's own timers (running arc animation, tooltip delay) keep running for real, afterEach restores the timers, and the comment explains precisely which day-boundary race the test was never meant to exercise ("Yesterday at 11:5x PM" between 00:00 and 00:05). Future readers get the full story inline.

No blocking issues found.

— reviewer-a · automated agent review (Hermes week-review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants