test: cover benchmark dashboard renderer

[ndycode]

Summary

• add a focused smoke test for scripts/benchmark-render-dashboard.mjs
• verify the script writes HTML from a minimal benchmark summary fixture
• improve coverage for a previously untested entrypoint script

Validation

• npm run typecheck
• npm run lint -- test/benchmark-render-dashboard-script.test.ts
• npm run test -- test/benchmark-render-dashboard-script.test.ts
• manual QA: node scripts/benchmark-render-dashboard.mjs --input=<fixture> --output=<tmp>/dashboard.html

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

adds a focused smoke-test suite for scripts/benchmark-render-dashboard.mjs, covering the happy path, spaced paths (windows safety), and three error branches (missing input file, malformed json, missing output dir). the test follows project conventions correctly: removeWithRetry with EBUSY/EPERM/ENOTEMPTY backoff for windows filesystem safety, spawnSync with an explicit 10s timeout, and absolute temp paths via mkdtempSync.

• removeWithRetry implementation correctly mirrors the pattern used across repo-hygiene.test.ts, rotation-integration.test.ts, and unified-settings.test.ts
• paths-with-spaces test is a good windows coverage addition since the script arg parser uses startsWith+slice rather than shell splitting
• the afterEach registered at module level (outside describe) is idiomatic vitest and works correctly here
• one missing vitest coverage gap: the no---input branch exits via process.exitCode = 1; return; (not process.exit(1)), so stderr is empty and the .catch() handler never fires — this path has no test case

Confidence Score: 5/5

• safe to merge — test-only addition with no production code changes and correct windows safety patterns throughout
• single new test file, no production code changes, follows all project conventions (removeWithRetry, absolute paths, spawnSync), the one missing test case for the no-input usage branch is a minor gap and non-blocking
• no files require special attention

Important Files Changed

Filename	Overview
test/benchmark-render-dashboard-script.test.ts	new smoke-test suite for the benchmark render dashboard script; correctly uses removeWithRetry for windows safety, covers the main happy path, path-with-spaces, missing-file, malformed-json, and missing-output-dir cases; missing one test case for the no---input usage branch which exits via process.exitCode rather than process.exit

Sequence Diagram

sequenceDiagram
    participant T as vitest test
    participant S as spawnSync
    participant N as node (child process)
    participant FS as filesystem (tmpdir)

    T->>FS: mkdtempSync → create temp root
    T->>FS: writeFileSync → write summary.json fixture
    T->>S: spawnSync(node, [scriptPath, --input=..., --output=...])
    S->>N: spawn child process (timeout 10s)
    N->>FS: readFile(inputPath)
    N->>N: JSON.parse + renderDashboardHtml()
    N->>FS: writeFile(outputPath, html)
    N-->>S: stdout "Dashboard written:", status 0
    S-->>T: SpawnSyncReturns
    T->>FS: readFileSync(outputPath) → assert html contents
    T->>FS: afterEach removeWithRetry(root, recursive+force)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/benchmark-render-dashboard-script.test.ts
Line: 111-113

Comment:
**missing coverage for no-`--input` usage path**

the script's `main()` handles missing `--input` via `process.exitCode = 1; return;` — not `process.exit(1)` — so the `.catch()` handler never fires and `stderr` stays empty. this exit mechanism is subtly different from the error paths that are tested. worth adding a case to verify that omitting `--input` still exits with status 1 and produces expected stdout (usage text), not an error on stderr.

```ts
it("exits with status 1 when --input is not provided", () => {
    const result = runRenderDashboard([]);

    expect(result.status).toBe(1);
    expect(result.stderr).toBe("");
    expect(result.stdout).toContain("Usage:");
});
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "test: harden benchma..."}

[coderabbitai]

📝 Walkthrough

Walkthrough

new test file validates the benchmark dashboard script's cli behavior by creating temporary directories, writing test summary data, spawning the process with input/output arguments, and asserting successful execution with expected html output.

Changes

Cohort / File(s)	Summary
Benchmark Dashboard Test test/benchmark-render-dashboard-script.test.ts	91 lines added. new vitest suite that spawns the benchmark script as a subprocess, validates exit code and stdout contains "Dashboard written:", and checks generated html contains expected static and dynamic content. uses temporary directories with afterEach cleanup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

review notes

windows compatibility risk: test/benchmark-render-dashboard-script.test.ts:line uses spawnSync with node executable path. verify the path resolution works consistently across windows/unix paths. also check if summary.json fixture handles path separators correctly on windows.

missing regression test coverage: the test validates happy path (successful execution, correct html output) but doesn't cover error cases. consider adding tests for:

• malformed summary.json input
• missing required meta or rows fields
• write permission failures on output directory
• invalid --input or --output arguments

concurrency consideration: temporary directory cleanup in afterEach could fail if the dashboard script process hasn't fully released file handles on windows. might need explicit wait or delayed cleanup.

test isolation: ensure temp directories don't accumulate if tests fail. current recursive deletion pattern is solid but verify fs operations have proper error handling in cleanup hook.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format (test: scope summary), type is valid (test), summary is lowercase imperative and under 72 chars.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	PR description covers all critical sections: summary, what changed, validation steps, and manual QA. However, it omits several required template sections (Docs & Governance Checklist, Risk & Rollback).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/pr8-benchmark-render-dashboard-smoke

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch test/pr8-benchmark-render-dashboard-smoke

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/benchmark-render-dashboard-script.test.ts`:
- Around line 23-90: Add edge-case tests alongside the existing happy-path in
test/benchmark-render-dashboard-script.test.ts: add one test that spawns the
script (using process.execPath and scriptPath) with a non-existent --input and
asserts non-zero exit and stderr includes a helpful message; add one test that
writes malformed JSON to the inputPath, spawns the script, and asserts non-zero
exit and stderr contains JSON parse or summary error; add one test that points
--output to a file inside a non-existent directory and assert either the script
creates the directory and exits 0 (check stdout "Dashboard written:" and that
file exists) or it exits non-zero with a clear stderr; use the same
mkdtempSync/tempRoots pattern and spawnSync and readFileSync to implement these
checks so they run alongside the existing test.
- Around line 79-86: Update the test that runs spawnSync so failed runs surface
stderr and the process can't hang: when calling spawnSync (the call using
process.execPath and scriptPath), add the timeout option (e.g., timeout: <ms>)
and update the assertions to include or log result.stderr when result.status is
not 0; specifically, include result.stderr in the expect failure message or an
additional expect toContain/length check so error output is visible alongside
the existing expect(result.stdout).toContain("Dashboard written:").
- Line 81: Add a regression test that passes Windows-like paths containing
spaces to the benchmark-render-dashboard script to lock in correct parsing; in
the test file (test/benchmark-render-dashboard-script.test.ts) add a case that
creates a temporary input path with spaces (e.g., using tmp or fs.mkdtemp and a
filename with a space) and invokes spawnSync with the same argument array
pattern ([scriptPath, `--input=${inputPathWithSpaces}`,
`--output=${outputPath}`]) to assert the script
(scripts/benchmark-render-dashboard.mjs) still receives and parses the
`--input=PATH` form correctly (note the script's parser at lines ~18-22 slices
after '='), then verify expected behavior or output.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0f2c8cc1-de5d-43ce-a413-054da3ac93f2

📥 Commits

Reviewing files that changed from the base of the PR and between 1be5e95 and 5bd7739.

📒 Files selected for processing (1)

• test/benchmark-render-dashboard-script.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (1)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/benchmark-render-dashboard-script.test.ts

🔇 Additional comments (2)

test/benchmark-render-dashboard-script.test.ts (2)

14-21: cleanup logic looks solid.

the afterEach hook with rmSync({ recursive: true, force: true }) handles leftover temp dirs correctly, and the force flag avoids throws on already-deleted paths. no issues here.

---

1-12: imports and setup are clean.

using node: prefixed imports is good practice for clarity. tempRoots array at module scope works fine with vitest's isolation model.