Skip to content

test: load ICU data once before Vitest workers start - #2226

Merged
chhoumann merged 1 commit into
masterfrom
fix/windows-filesuggester-timeout
Oct 12, 2026
Merged

chhoumann merged 1 commit into
masterfrom
fix/windows-filesuggester-timeout

Conversation

@chhoumann

@chhoumann chhoumann commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Summary

Platform Test (windows-latest) failed on the last two master pushes. Both times, renders tooltip metadata as text in fileSuggester.test.ts timed out at 5 s (13,446 ms after #2224 and 5,934 ms after #2225). #2224 did not cause it. In the 20 Windows runs before #2224, the same test took 4,308 ms once and 609 to 1,994 ms in five others.

The test made the run's first date format, through toLocaleDateString() in createTooltip. Node on Windows embeds its ICU data in node.exe, and the first date format on a runner reads those pages from disk. Vitest runs large test files first, so this file starts while every worker loads jsdom, and the read competes for the disk. A Windows probe measured that first call at 689 ms of wall time for 62 ms of CPU. Later processes share the pages, so only the first one waits.

The fix formats one date in a Vitest globalSetup. It runs in the main process before any worker starts.

Changes

  • Add tests/vitest-global-setup.ts, which formats one date.
  • Register it as globalSetup in vitest.config.mts.

Production code is unchanged, because ICU is already loaded in Obsidian. A beforeAll in this one test file was rejected: the wait falls on whichever file formats a date first, and two other test files format dates. A larger test timeout was rejected because it hides the stall.

Testing / validation

  • Windows probe with this fix on a cold runner (run 38285030654): the warm-up waited 509 ms (31 ms CPU) before workers started. The tooltip test's first date format then took 21.6 ms and was CPU-bound. On a warm runner (run 38284834255), the warm-up took 6.4 ms.
  • CI dispatch on this branch (run 38286936693): all five jobs pass. On Windows, fileSuggester.test.ts takes 817 ms for 24 tests, and the tooltip test stays under the 300 ms slow-test threshold.
  • pnpm run test passes (7,019 passed, 28 skipped). Oxlint and tsc pass.

Release impact: none. The change is test setup only, and test: requests no release.

Checklist

  • PR title follows Conventional Commits.
  • No linked issue. Found as red master CI.
  • Release and migration impact noted.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5beb5b25-c411-4fbf-b4c4-68a9e782f752

📥 Commits

Reviewing files that changed from the base of the PR and between d57eb15 and e44bee7.


📒 Files selected for processing (2)
  • tests/vitest-global-setup.ts
  • vitest.config.mts

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



📝 Walkthrough
📝 Walkthrough

Walkthrough

Vitest now runs a global setup function that formats the Unix epoch date with toLocaleDateString() before tests run.

Changes

Vitest ICU setup

Layer / File(s) Summary
Add and register global setup
tests/vitest-global-setup.ts, vitest.config.mts
The setup function formats the Unix epoch date with toLocaleDateString(). Vitest registers the setup file as a global setup file.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Other


Merge Risk: ⚪ Minimal · up to e44be

This change affects test startup, not production behavior; no actionable merge-blocking issue is identified.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
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.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: loading ICU data before Vitest workers start. It matches the pull request objective and changed files.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • 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

A rabbit checks the date at dawn
The epoch shines before tests spawn
Locale leaves its data warm
Vitest starts without alarm
Hops along, the setup’s done

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

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying quickadd with  Cloudflare Pages  Cloudflare Pages

Latest commit: e44bee7
Status: ✅  Deploy successful!
Preview URL: https://c4ee98f6.quickadd.pages.dev
Branch Preview URL: https://fix-windows-filesuggester-ti.quickadd.pages.dev

View logs

@chhoumann

Copy link
Copy Markdown
Owner Author

Ship verification: PASS+NOTES

Independent verification of this PR from fresh detached worktrees, not the author's checkout.

  • Head: e44bee7 (fix/windows-filesuggester-timeout)
  • Base: d57eb15 (master)
  • Patch-id: b0bfb60ed0b25e486f8f9768d4ef837f81ea9205 (recomputed locally, matches)

Checks run and results:

  1. Diff scope: 2 files, 8 insertions, 0 deletions. Only tests/vitest-global-setup.ts (new) and the globalSetup line in vitest.config.mts. No src/ change. The new file is outside the Vitest include globs, so it is not collected as a test.
  2. Artifacts: pnpm run build at base and at head (Node v24.19.0, Linux) produce byte-identical bundles. main.js sha256 d83aa6eeaa77bbcfe0e3574f3ab1e67c57469506074c43f5e67087bf64ae6127, styles.css sha256 9467ef3622cd1137ce5a0148e16de23b9f5be87bc8c204fb6050c48f8f29994d, same hashes at both commits and again after build-with-lint.
  3. pnpm run test at head: exit 0. 501 files passed, 6 skipped; 7019 tests passed, 28 skipped. Matches the counts in the PR body.
  4. globalSetup actually runs: a temporary NODE_OPTIONS preload (outside the checkout, nothing committed) recorded the first toLocaleDateString call per process. The first record is from the Vitest main process (argv vitest.mjs) at warmIcuData [as setup] in tests/vitest-global-setup.ts:6, called by TestProject._initializeGlobalSetup. The next records are forked workers (ppid is the main process) about 30 s later, first from ImportPackageModal.svelte:466, then from createTooltip in fileSuggester. So the warm-up runs once, in the main process, before any worker formats a date.
  5. pnpm run build-with-lint at head: exit 0 (tsc, oxlint over src and tests, svelte policy, svelte-check, production bundle).
  6. Windows CI evidence (read with gh):
    • Run 38286936693 (workflow_dispatch at e44bee7): all five jobs succeeded. Platform Test (windows-latest) job 114937447878: 500 files passed, 7 skipped, 179.53 s, Node 24.21.0. fileSuggester.test.ts 24 tests in 817 ms. The reporter in that log lists tests over 300 ms (several appear at 324 to 576 ms) and the tooltip test is not among them, so it ran under the slow threshold.
    • Run 38261351461 (master at e4133fa): Windows job 114852359980 failed. fileSuggester.test.ts 14162 ms, renders tooltip metadata as text 13446 ms, "Test timed out in 5000ms". 1 failed, 498 passed, 7 skipped.
    • Run 38277500194 (master at d57eb15): Windows job 114906447939 failed. fileSuggester.test.ts 6490 ms, tooltip test 5934 ms, same timeout. 1 failed, 499 passed, 7 skipped.
    • The PR's own pull_request run 38286965305 has Test, Build + Lint and Docs green; the Platform Test matrix is skipped there by design (if: push || workflow_dispatch in ci.yml), so the Windows pass at this SHA comes from the manual dispatch run above.
  7. History before build(deps): adopt native tooling and replace ESLint #2224: in the 14 master Windows runs from 37703697571 to 38216261806 the tooltip test was listed as slow five times (609, 678, 791, 1994, 4308 ms) and the file ranged 678 to 4952 ms. The stall predates build(deps): adopt native tooling and replace ESLint #2224, as the PR body says.
  8. Probe runs cited in the PR body: run 38285030654 (cold runner) logs the parent warm-up at 509.4 ms wall for 31.0 ms CPU, a child process right after at 17.3 ms wall and 0 CPU, and the worker's cold toLocaleDateString at 21.6 ms wall (CPU-bound). The unwarmed Intl.NumberFormat control took 24.6 ms wall for 0 CPU on the cold runner and 3.6 ms on the warm runner 38284834255, where the parent warm-up took 6.4 ms. Wall time far above CPU time on the first call, and a cheap call in every later process, is what the disk page-in story predicts.

Root-cause judgment: supported. The failing test is synchronous, and Vitest cannot interrupt a synchronous body, so the 13.4 s and 5.9 s are real wall time inside a body that only builds a few DOM nodes and formats one date. The probes show that first format is I/O-bound on a cold Windows runner and that every later process gets it for about 20 ms once the parent has touched the pages. The fix pays that cost once in the main process before workers start, which is the right place given that two other test files also format dates.

Notes, none blocking:

  • The 13 s magnitude itself was not reproduced; the cold probe measured about 0.5 s for the first call while the runner was otherwise idle. The multi-second stalls happen only while the first wave of workers loads jsdom, which the probe does not recreate. The mechanism is demonstrated, the worst case is inferred.
  • The PR body's "689 ms of wall time for 62 ms of CPU" figure does not appear in either cited probe run (those show 509.4/31.0 and 6.4/0.0). Presumably from an earlier probe run. Cosmetic.
  • Why the stall crossed 5 s on two consecutive master pushes right after build(deps): adopt native tooling and replace ESLint #2224, after sitting at or below 4.3 s in the prior 14 runs, is not explained. The fix removes the first-call I/O from the test path either way.
  • Local only: on this heavily loaded Linux box (load average above 60 on 12 cores) one unrelated test, completeFormatter-2197-escaped-pipe.test.ts > writes an inline script's escaped pipe verbatim, timed out at 5 s during the probe-instrumented full run. It passes in the plain full run and when rerun alone both with and without the preload (13 of 13, 2.6 to 3.8 s). Not attributable to this PR, flagged for awareness.

Release impact: none, test infrastructure only, bundles unchanged.

@chhoumann

Copy link
Copy Markdown
Owner Author

Independent verification: PASS+NOTES

Head e44bee708d97a4e4eea4c73e53c2ece9fa58701b, base d57eb156, patch-id b0bfb60ed0b25e486f8f9768d4ef837f81ea9205 (matches).

Claim What I ran Parent / without fix Head / with fix
Windows Platform Test failed on the last two master pushes, same test, 5 s timeout gh run view --log-failed on 38261351461 (#2224) and 38277500194 (#2225) renders tooltip metadata as text took 13,446 ms and 5,934 ms: "Test timed out in 5000ms" at fileSuggester.test.ts:483. No other test failed. -
#2224 did not cause it Windows job logs of the 30 master runs before #2224 (Oct 4-12) 0 failures. In the 20 runs right before #2224, the test took 4,308 ms once and 609-1,994 ms in five others, as the PR says. Over 300 ms in 7 of 29 logs. -
The failure is flaky, not consistent Windows logs of 4 dispatches on post-#2224 code without the fix (38280951885, 38290724108, 38291748711, 38292530026) The tooltip test ran in under 300 ms in all 4. Without the fix after #2224, 2 of 6 Windows runs failed, both on master pushes. -
The stall is the first ICU page-in (wall time much larger than CPU time) Author's probe logs: 38280955067 (no fix), 38285030654 (fix + probe) First ICU call in the worker: 688.7 ms wall, 62 ms CPU Warm-up in the main process: 509 ms wall, 31 ms CPU. The worker's first date format then took 21.6 ms.
globalSetup runs once in the main process before any worker; workers are separate processes Temporary probe config and test, removed afterwards - globalSetup pid = Vitest main pid, and its timestamp precedes the worker's. Worker ppid = main pid, isMainThread true, threadId 0, so the pool is forks. No ICU state crosses processes, so the gain comes from the OS file cache (Node embeds icudt78l.dat in its binary).
No timezone or locale change Same probe with TZ unset and with TZ=America/Los_Angeles, formatting 2026-03-08T10:30Z (US DST start) Copenhagen: 3/8/2026 11:30:00 AM, offset -60. LA: 3:30:00 AM, offset 420. Locale en-US. Identical
The fix holds on the real surface 5 workflow_dispatch runs at e44bee7: 38286936693 (author), 38295200027, 38295923051, 38296435883, 38296885308 - 5/5 green on Windows and macOS. Windows fileSuggester.test.ts: 817, 842, 454, 562, 566 ms. Tooltip test under the 300 ms slow threshold every time.
pnpm run test passes Local Linux 7,019 passed, 28 skipped, exit 0 7,019 passed, 28 skipped, exit 0. With TZ=America/Los_Angeles: same.
Gates pnpm install --frozen-lockfile, pnpm run build-with-lint exit 0, exit 0 exit 0, exit 0
Production code unchanged sha256sum main.js styles.css after build-with-lint main.js d83aa6eeaa77bbcfe0e3574f3ab1e67c57469506074c43f5e67087bf64ae6127, styles.css 9467ef3622cd1137ce5a0148e16de23b9f5be87bc8c204fb6050c48f8f29994d Identical

Notes

  1. Green dispatches alone do not prove the fix. Without the fix, post-build(deps): adopt native tooling and replace ESLint #2224 dispatches were green 4 of 4. The fix is supported by three things together: the paired probe (the cold read moves to the main process, and the worker's first date format becomes CPU-bound), the mechanism, and the sample. Across 36 Windows runs without the fix, the tooltip test was slow (over 300 ms) in 10. With the fix it was slow in 0 of 7 (5 head runs plus 2 probe runs).
  2. The PR's "689 ms" was measured on the probe's first ICU call, which was Intl.NumberFormat, not the date format. The date format right after it took 77.7 ms. The mechanism is the same (the first ICU page-in), but the wording is loose.
  3. A cold ICU read measured 0.5-0.7 s, but the failing runs stalled for 5.9-13.4 s. The extra time is attributed to disk contention while the first wave of workers loads jsdom. That is plausible but was not measured in a failing run.
  4. The warm-up touches only the pages that a default-locale, default-zone toLocaleDateString() needs. Other ICU data, such as collation or the zones that tests set (Asia/Tokyo, Europe/Copenhagen), still loads cold on first use. That is fine for this test, and nothing else currently times out.
  5. Scope and rules: the title follows Conventional Commits, release impact is noted, no generated files changed, and the 8 lines touch only test setup. tsconfig includes **/*.ts, so tsc type-checks the new file. vitest.e2e.config.mts is separate and does not get the globalSetup.

@chhoumann
chhoumann merged commit c8bb782 into master Oct 12, 2026
42 checks passed
@chhoumann
chhoumann deleted the fix/windows-filesuggester-timeout branch October 14, 2026 06:23
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