Skip to content

Stabilize notification dispatch tests with fixed timestamps - #33698

Merged
AmethystLiang merged 1 commit into
mainfrom
AmethystLiang/fix-ci
Oct 26, 2026
Merged

AmethystLiang merged 1 commit into
mainfrom
AmethystLiang/fix-ci

Conversation

@AmethystLiang

@AmethystLiang AmethystLiang commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​28 $\color{#cf222e}{\Huge{\mathbf{−}}}$​20 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​8
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1

Problem

Notification dispatch tests read the real clock, making their timing-dependent expectations flaky.

Solution

Freeze the test clock to one known time so every test uses predictable timestamps. Add a precise mock return type so CI type checking stays reliable.

ELI5

The tests used to look at the current time, which keeps changing. Now they use the same pretend time every run.

What Changed

  • Mock Date.now() with a fixed timestamp in notification dispatch tests and restore mocks afterward.
  • Replace wall-clock timestamp calls with values based on that fixed time.
  • Remove the resolved flaky-test baseline entry.
  • Add an explicit return type to the Mimo hook service mock.

Why

Using a fixed clock makes time comparisons repeatable in CI without changing notification behavior.

Linked Issue

Fixes #

Visual Proof

N/A — test-only and mock typing changes have no visual or interaction change.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No production behavior changes. Cross-platform, remote SSH, mobile, backwards compatibility, security, and performance impact are N/A.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 900cbce9-25ab-4af2-b2de-0085d720713c
Commits

Reviewing files that changed from the base of the PR and between a5f7da4 and e9d65f1.

Files selected for processing: 3
  • config/flaky-test-pattern-baseline.txt
  • src/main/ipc/pty-ipc-mock-registry.ts
  • src/renderer/src/components/terminal-pane/use-notification-dispatch.test.ts
Files with no reviewable changes: 1
  • config/flaky-test-pattern-baseline.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

The notification-dispatch tests now use a fixed timestamp for clock-dependent setup and fixtures. The flaky-test baseline entry for the test file is removed. The mimoHookServiceModuleMock factory now declares its return type; its returned mock object is unchanged.

Priority: Low

Merge Risk: Minimal · up to e9d65

The changes are limited to deterministic test timing and mock typing, with no established production risk.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description explains the problem, solution, scope, and lack of visual changes. However, the required linked issue is missing, testing verification is not reported, and required self-review and CI … Add the actual issue reference after “Fixes #”. Report the automated or manual test results, complete the self-review, and state whether lint, typecheck, test, and build checks passed or will be covered by CI.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: stabilizing notification dispatch tests with fixed timestamps.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

Full details: Description check

Explanation

The description explains the problem, solution, scope, and lack of visual changes. However, the required linked issue is missing, testing verification is not reported, and required self-review and CI checks remain incomplete.

Resolution

Add the actual issue reference after “Fixes #”. Report the automated or manual test results, complete the self-review, and state whether lint, typecheck, test, and build checks passed or will be covered by CI.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@AmethystLiang
AmethystLiang merged commit 405ce9b into main Oct 26, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant