Skip to content

test(tools-pack): give packaging suites a realistic timeout - #6215

Open
PerishCode wants to merge 1 commit into
mainfrom
launcher-payload-timeout-flake
Open

test(tools-pack): give packaging suites a realistic timeout#6215
PerishCode wants to merge 1 commit into
mainfrom
launcher-payload-timeout-flake

Conversation

@PerishCode

@PerishCode PerishCode commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Why

I hit this landing an unrelated docs PR (#6207): its merge-queue run failed and
the PR was dequeued, on a tree whose only change was one markdown file and two
documentation lines. The failure was tools/pack/tests/launcher-payload.test.ts
→ "creates a Windows payload 7z with bootstrap-readable contents", with
Test timed out in 5000ms at 5007ms. The same job passed on that PR's own
run, on the same tree.

The pain is not that one test is slow — measurement says the opposite. On this
branch's own CI run the whole file completed in 657ms for all 6 tests. In
the failing merge-queue run the non-7z tests in that file summed to ~720ms
while the 7z test alone hit 5007ms. So the test normally costs well under a
second and occasionally stalls past 5s on a Windows runner: roughly an 8x
outlier, not a marginal overshoot.

tools/pack has no vitest config at all, so the whole package runs on vitest's
5s default — a value nobody chose for it. That package shells out to real
packaging tools (7z archive and extract, pnpm pack, installer assembly)
against temp directories, which is exactly the workload where an occasional
subprocess or filesystem stall is expected. With no headroom, such a stall
fails a merge-queue gate and dequeues an unrelated PR.

This change raises the ceiling so a stall stops being a merge-gate failure. It
deliberately does not diagnose the stall — see Adjacent issues.

Recent history for that job: 10 success, 1 cancelled, 1 failure (this one).
Rare, but it lands on a merge-queue gate, so it costs a full re-queue cycle.

What users will see

No user-facing or behavioral change. Contributors touching tools/pack get a
merge-queue gate that is not decided by Windows runner speed.

Surface area

  • None — internal refactor, docs, tests, or translation update only

Screenshots

Not applicable.

Bug fix verification

A red spec is not available for this one: the failing test is
it.skipIf(process.platform !== "win32"), so it cannot execute on macOS or
Linux, and the symptom is a timing boundary on a Windows CI runner rather than
a falsifiable behavior. Reproducing it deterministically would mean pinning
runner speed, which the harness cannot do.

Verification done instead:

  • Confirmed the failure is a deadline, not an assertion — 5007ms against a
    5000ms limit, no assertion output.
  • Confirmed the same job passed the identical tree on docs(tools-pack): add build cache contract #6207's pull-request run.
  • Confirmed tools/pack has no vitest config, so the 5s value was vitest's
    default rather than a deliberate budget, and that apps/daemon (20_000) and
    apps/desktop (10_000) already raise it for the same reason.
  • Guarded the include-narrowing risk: adding include could silently stop
    running suites outside the pattern. All 36 tools/pack suites live in
    tests/ and are .test.ts; zero files sit outside it.

Adjacent issues

Not addressed here, deliberately:

  • The stall itself is undiagnosed. An 8x swing on the same test and the
    same tree points at the environment — 7z subprocess startup contention,
    antivirus scanning the temp archive, or cold filesystem — not at the test
    logic. Raising the timeout stops it from failing the gate; it does not
    explain it. Worth revisiting if the job's failure rate rises.
  • Whether the Windows launcher payload test should do real 7z work at all, or
    use a smaller fixture, is a test-shape question with its own tradeoffs.
  • Per docs/testing/test-efficiency.zh-CN.md, duration budgets should follow a
    representative baseline rather than lead it. This PR removes a false failure
    signal; it does not introduce a duration guard.

Validation

Baseline vs. change, both on this branch:

  • pnpm --filter @open-design/tools-pack test without the config:
    36 files, 248 passed, 8 skipped (256)
  • pnpm --filter @open-design/tools-pack test with the config:
    36 files, 248 passed, 8 skipped (256) — identical, so the include pattern
    drops nothing
  • pnpm guard — passed
  • pnpm typecheck — passed
  • pnpm --filter @open-design/tools-pack typecheck — passed
  • git diff --check — clean

Measured on this branch's CI run (Windows job): launcher-payload.test.ts
completed in 657ms for all 6 tests — comfortably inside the old 5s limit too.
This run therefore does not by itself demonstrate the fix working; it
establishes the test's normal cost, which is what makes the 5007ms failure
identifiable as an outlier rather than a marginal overshoot.

Note: the affected test is Windows-only and skips on this machine, so the
timeout headroom itself can only be exercised by the Windows CI job.

tools/pack had no vitest config, so its suites ran on vitest's 5s default.
That package shells out to real packaging tools — 7z archive and extract,
pnpm pack, installer assembly — against temp directories, so 5s is close
enough to their actual cost that runner speed alone decides pass/fail.

The Windows launcher payload archive test failed merge-queue CI at 5007ms
while passing the same tree on the pull-request run and locally.

Add a package vitest config with testTimeout 20_000, matching apps/daemon,
which is subprocess-heavy for the same reason. The include pattern matches
the 36 existing suites exactly; test counts are unchanged.
@PerishCode
PerishCode requested a review from a team as a code owner July 29, 2026 06:17
@lefarcen
lefarcen requested a review from mrcfps July 29, 2026 06:22
@lefarcen lefarcen added size/XS PR changes <20 lines risk/medium Medium risk: regular code changes type/chore CI / build / config / tooling labels Jul 29, 2026

@mrcfps mrcfps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PerishCode I reviewed the new tools/pack/vitest.config.ts against the package test inventory and the live CI invocation. The config preserves Node execution, includes all 36 existing .test.ts suites, and raises only the per-test ceiling to 20 seconds. On this head, the full tools-pack suite passed with 248 tests passing and 8 skipped, and the Windows launcher-payload lane passed with 5 tests passing and 1 skipped. Thanks for documenting the original flake evidence so carefully and keeping the fix focused—nice work. 🙌

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

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

Labels

risk/medium Medium risk: regular code changes size/XS PR changes <20 lines type/chore CI / build / config / tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants