fix(tests): fix Windows cost-tracker HOME env override#700
fix(tests): fix Windows cost-tracker HOME env override#700chris-yyau wants to merge 3 commits intoaffaan-m:mainfrom
Conversation
On Windows, Node.js os.homedir() checks USERPROFILE (not HOME), so tests that override HOME to redirect metrics to a temp directory fail because the cost-tracker script writes to the real home directory. Mirror HOME to USERPROFILE in runScript() env overrides so os.homedir() resolves to the expected temp directory on all platforms. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
📝 WalkthroughWalkthroughRefactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes cross-platform test failures in Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["runScript(input, envOverrides)"] --> B{"'HOME' in envOverrides?"}
B -- No --> D["env = { ...process.env, ...envOverrides }"]
B -- Yes --> C{"'USERPROFILE' in envOverrides?"}
C -- Yes --> D
C -- No --> E["env = { ...process.env, ...envOverrides, USERPROFILE: envOverrides.HOME }"]
D --> F["spawnSync('node', [cost-tracker.js], { env })"]
E --> F
F --> G["cost-tracker.js reads stdin"]
G --> H["os.homedir() → resolves HOME (POSIX) or USERPROFILE (Windows)"]
H --> I["Write metrics to tmpHome/.claude/metrics/costs.jsonl"]
I --> J["stdout.write(raw) — pass-through"]
J --> K["Test assertions"]
K --> L{"Pass?"}
L -- Yes --> M["passed++"]
L -- No --> N["failed++"]
M & N --> O["finally: fs.rmSync(tmpHome)"]
Last reviewed commit: "fix(tests): immutabl..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows-specific failures in the cost-tracker hook tests by ensuring that when the tests override HOME (to redirect metrics output into a temp directory), the same override is also applied to USERPROFILE, which is what os.homedir() uses on Windows.
Changes:
- Update
runScript()in the cost-tracker test to mirrorHOME→USERPROFILEwhenHOMEis overridden. - Refactor the per-test pass/fail counter updates into a more readable multiline ternary form.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/hooks/cost-tracker.test.js
Outdated
| const inputStr = typeof input === 'string' ? input : JSON.stringify(input); | ||
| // Mirror HOME to USERPROFILE for Windows (os.homedir() checks USERPROFILE on Windows) | ||
| const env = { ...process.env, ...envOverrides }; | ||
| if (envOverrides.HOME && !envOverrides.USERPROFILE) { |
There was a problem hiding this comment.
The HOME→USERPROFILE mirroring check uses truthiness (envOverrides.HOME && !envOverrides.USERPROFILE). This will treat an explicitly-provided but falsy USERPROFILE (e.g. '') as “not provided” and overwrite it, and it won’t run if HOME is intentionally set to a falsy value. Elsewhere in this repo, similar helpers use typeof ... === 'string' and !('USERPROFILE' in envOverrides) (e.g. tests/scripts/ecc.test.js:18-24, tests/scripts/session-inspect.test.js:20-26); consider aligning to that pattern here (and optionally mirroring both directions for consistency).
| if (envOverrides.HOME && !envOverrides.USERPROFILE) { | |
| if (typeof envOverrides.HOME === 'string' && !('USERPROFILE' in envOverrides)) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/hooks/cost-tracker.test.js (1)
33-37: Good fix for Windows cross-platform compatibility.The approach correctly mirrors HOME to USERPROFILE since
os.homedir()on Windows checks USERPROFILE first. The condition properly preserves an explicit USERPROFILE if the caller provides one.Consider a fully immutable pattern to align with coding guidelines:
♻️ Optional: Single-expression immutable construction
- const env = { ...process.env, ...envOverrides }; - if (envOverrides.HOME && !envOverrides.USERPROFILE) { - env.USERPROFILE = envOverrides.HOME; - } + const env = { + ...process.env, + ...envOverrides, + ...(envOverrides.HOME && !envOverrides.USERPROFILE && { USERPROFILE: envOverrides.HOME }) + };Based on learnings: "Ensure cross-platform compatibility for Windows, macOS, and Linux via Node.js scripts". As per coding guidelines: "Always create new objects instead of mutating existing ones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/cost-tracker.test.js` around lines 33 - 37, The current code creates env then mutates it to mirror HOME to USERPROFILE; change this to a fully immutable single-expression construction that sets USERPROFILE conditionally when building env rather than assigning to env after creation—use process.env and envOverrides in the object spread and add USERPROFILE only if envOverrides.HOME exists and envOverrides.USERPROFILE does not, so there is no post-creation mutation of the env variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/hooks/cost-tracker.test.js`:
- Around line 33-37: The current code creates env then mutates it to mirror HOME
to USERPROFILE; change this to a fully immutable single-expression construction
that sets USERPROFILE conditionally when building env rather than assigning to
env after creation—use process.env and envOverrides in the object spread and add
USERPROFILE only if envOverrides.HOME exists and envOverrides.USERPROFILE does
not, so there is no post-creation mutation of the env variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f563685-d531-4b68-a21e-a90546d07efe
📒 Files selected for processing (1)
tests/hooks/cost-tracker.test.js
- Use `in` operator for env override checks instead of truthiness, matching the pattern used elsewhere in the repo (handles falsy values) - Redirect test 1 (passes through input) to a temp directory so it doesn't write metrics to the real home directory Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
Review feedback addressed (b960e3c)Copilot: Switched env override check from truthiness ( Greptile: Test 1 ( The 9 Windows test failures are pre-existing on main (same jobs fail on the latest main CI run). Our fix resolves the cost-tracker tests — all 5 pass on Windows: Remaining Windows failures are bash-on-Windows issues in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/hooks/cost-tracker.test.js (1)
33-37: Avoid mutatingenvafter creation inrunScript.Line 36 mutates
envdirectly. Buildenvimmutably in one expression so the helper remains side-effect-safe and guideline-compliant.Suggested refactor
function runScript(input, envOverrides = {}) { const inputStr = typeof input === 'string' ? input : JSON.stringify(input); // Mirror HOME to USERPROFILE for Windows (os.homedir() checks USERPROFILE on Windows) - const env = { ...process.env, ...envOverrides }; - if ('HOME' in envOverrides && !('USERPROFILE' in envOverrides)) { - env.USERPROFILE = envOverrides.HOME; - } + const shouldMirrorHome = 'HOME' in envOverrides && !('USERPROFILE' in envOverrides); + const env = { + ...process.env, + ...envOverrides, + ...(shouldMirrorHome ? { USERPROFILE: envOverrides.HOME } : {}) + }; const result = spawnSync('node', [script], {As per coding guidelines, "
Always create new objects, never mutate existing ones" and "Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/cost-tracker.test.js` around lines 33 - 37, The test helper mutates the local env object after creation; instead, build env immutably in one expression inside runScript by combining process.env and envOverrides and conditionally adding USERPROFILE when HOME is present and USERPROFILE is not (avoid any assignment to env after creation). Locate the env construction in runScript (the const env = { ... } block) and replace it with a single spread expression that includes a conditional spread for USERPROFILE (e.g., use ...(envOverrides.HOME && !envOverrides.USERPROFILE ? { USERPROFILE: envOverrides.HOME } : {})) so no mutation occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/cost-tracker.test.js`:
- Around line 54-66: Wrap each test body that creates tmpHome (e.g., the "passes
through input on stdout" test using tmpHome, runScript and fs.rmSync) in a
try/finally so fs.rmSync(tmpHome, { recursive: true, force: true }) always runs;
specifically, move the test logic into the try block and put the fs.rmSync call
in the finally block (apply the same pattern to the other affected tests at
lines 71-94, 97-107, 110-121, 124-143) to guarantee cleanup even if assertions
fail.
---
Nitpick comments:
In `@tests/hooks/cost-tracker.test.js`:
- Around line 33-37: The test helper mutates the local env object after
creation; instead, build env immutably in one expression inside runScript by
combining process.env and envOverrides and conditionally adding USERPROFILE when
HOME is present and USERPROFILE is not (avoid any assignment to env after
creation). Locate the env construction in runScript (the const env = { ... }
block) and replace it with a single spread expression that includes a
conditional spread for USERPROFILE (e.g., use ...(envOverrides.HOME &&
!envOverrides.USERPROFILE ? { USERPROFILE: envOverrides.HOME } : {})) so no
mutation occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99b6d9ec-cd06-4aa2-b30e-a7fa937dca47
📒 Files selected for processing (1)
tests/hooks/cost-tracker.test.js
Address review nitpicks on PR affaan-m#700: - Build env object immutably via conditional spread instead of post-creation mutation - Wrap all 5 test bodies in try/finally to guarantee temp dir cleanup on assertion failure Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/hooks/cost-tracker.test.js (1)
55-155: Optional: extract a shared helper for temp HOME test scaffolding.The repeated
makeTempDir+try/finallypattern is solid, but a small helper (e.g.,withTempHome) could reduce duplication and keep each test focused on assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/cost-tracker.test.js` around lines 55 - 155, Tests repeat makeTempDir() + try/finally cleanup; extract a small helper (e.g., withTempHome) that wraps makeTempDir, accepts a callback which receives the tmpHome, runs the test body, and always removes the temp dir in a finally/cleanup block so each test calls withTempHome(tmpHome => { ... }) instead of duplicating makeTempDir/try/finally; update tests that reference makeTempDir and runScript to use withTempHome while keeping assertions unchanged (refer to tests 'passes through input on stdout', 'creates metrics file...', 'handles empty input...', 'handles invalid JSON...', and 'handles missing usage fields gracefully' and the runScript helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/hooks/cost-tracker.test.js`:
- Around line 55-155: Tests repeat makeTempDir() + try/finally cleanup; extract
a small helper (e.g., withTempHome) that wraps makeTempDir, accepts a callback
which receives the tmpHome, runs the test body, and always removes the temp dir
in a finally/cleanup block so each test calls withTempHome(tmpHome => { ... })
instead of duplicating makeTempDir/try/finally; update tests that reference
makeTempDir and runScript to use withTempHome while keeping assertions unchanged
(refer to tests 'passes through input on stdout', 'creates metrics file...',
'handles empty input...', 'handles invalid JSON...', and 'handles missing usage
fields gracefully' and the runScript helper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3ee541d-543f-4668-a6d8-f1b2cef84af9
📒 Files selected for processing (1)
tests/hooks/cost-tracker.test.js
|
Superseded by #701 (Windows CI fix already merged) |
Summary
HOMEtoUSERPROFILEin env overridesos.homedir()checksUSERPROFILE(notHOME), so tests that overrideHOMEto redirect metrics to a temp directory fail because the script writes to the real home directoryChanges
tests/hooks/cost-tracker.test.js: InrunScript(), whenHOMEis set in env overrides, automatically setUSERPROFILEto the same value soos.homedir()resolves correctly on Windows.Test plan
🤖 Generated with Claude Code
Summary by cubic
Fix Windows cost-tracker test failures by mirroring
HOMEtoUSERPROFILEsoos.homedir()points to the temp home on Windows. Also harden tests by building the env immutably and using try/finally for temp dir cleanup.inoperator for env checks and redirect the pass-through test to a temp home to avoid writes to the real home.Written for commit 53d760a. Summary will update on new commits.
Summary by CodeRabbit
Tests
Chores