test: cover cross-process credential writes - #280
Conversation
WalkthroughAdds cross-process credential write coverage using configurable child-process coordination, validating failure handling, concurrent profile preservation, and credentials lock-file cleanup. ChangesCross-process credential writes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/credentials-cross-process.test.ts`:
- Around line 57-93: Add focused tests for the failure branches of
runCredentialWriter alongside the existing success test: omit a required
credential environment variable and assert exit code 1, and configure an
uncreated CRED_START_PATH with a shortened timeout and assert exit code 2. Reuse
the existing temporary-directory setup and child-result assertions, and cover
the writeProfile error path with exit code 3 if the helper exposes a
controllable failure trigger.
- Around line 52-55: Set an explicit timeout on the beforeAll build hook in the
“credentials cross-process writes” suite, long enough for execNpm(['run',
'build'], ...) to complete reliably. Keep the existing build command and test
behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e8ce548-81fa-42cc-abfe-dfe9b90d615d
📒 Files selected for processing (2)
test/credentials-cross-process.test.tstest/helpers/credentials-write-child.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/credentials-cross-process.test.ts (1)
1-50: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake process timing and barrier coordination injectable.
The test suite relies on hard-coded wall-clock waits: the child helper waits 5 seconds,
runCredentialWriterhas a fixed kill timeout, and the race test uses a short delay before creating the marker. This makes the suite slower/flakier under load, and the delay does not guarantee every child reached the barrier.Expose timeout/sleep controls through test dependencies and use an explicit child-readiness handshake before releasing the marker.
As per path instructions, tests must be deterministic and offline: no real network and no real timers — inject fetch/sleep via the test deps.
Also applies to: 75-96, 123-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/credentials-cross-process.test.ts` around lines 1 - 50, Update the cross-process credential tests and runCredentialWriter to inject sleep, timeout, and fetch behavior through test dependencies instead of using hard-coded delays, kill timers, or real network access. Replace the race test’s marker delay with an explicit readiness handshake from every child before releasing the marker, and make the child helper’s barrier wait use the injected timing controls. Preserve the existing credential assertions while ensuring all synchronization is deterministic and timer-free.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/credentials-cross-process.test.ts`:
- Around line 1-50: Update the cross-process credential tests and
runCredentialWriter to inject sleep, timeout, and fetch behavior through test
dependencies instead of using hard-coded delays, kill timers, or real network
access. Replace the race test’s marker delay with an explicit readiness
handshake from every child before releasing the marker, and make the child
helper’s barrier wait use the injected timing controls. Preserve the existing
credential assertions while ensuring all synchronization is deterministic and
timer-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e51b8517-0b23-40f2-8e70-ce7a9eb4f7f5
📒 Files selected for processing (1)
test/credentials-cross-process.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/credentials-cross-process.test.ts`:
- Around line 52-62: Update waitForFiles to accept injectable clock and sleep
dependencies instead of directly calling Date.now and setTimeout, while
preserving its polling and timeout behavior. Update the readiness and
timeout-path tests to provide controlled fake time and sleep advancement,
removing reliance on real timer delays and child-process scheduling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74e8ad9d-6b8c-4330-8839-8235f42961b6
📒 Files selected for processing (2)
test/credentials-cross-process.test.tstest/helpers/credentials-write-child.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/helpers/credentials-write-child.mjs
| async function waitForFiles(paths: string[], timeoutMs = 5_000): Promise<void> { | ||
| const deadline = Date.now() + timeoutMs; | ||
|
|
||
| while (true) { | ||
| const missing = paths.filter(path => !existsSync(path)); | ||
| if (missing.length === 0) return; | ||
| if (Date.now() >= deadline) { | ||
| throw new Error(`Timed out waiting for files: ${missing.join(', ')}`); | ||
| } | ||
| await new Promise(resolveDelay => setTimeout(resolveDelay, 5)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Inject time control instead of polling wall-clock time.
waitForFiles directly uses Date.now()/setTimeout, while the timeout-path test depends on real 1 ms/100 ms child timing. Inject clock/sleep dependencies so readiness and timeout cases can run under controlled time rather than CI scheduling.
As per path instructions, “Tests must be deterministic and offline: No real network and no real timers — inject fetch/sleep via the test deps.”
Also applies to: 97-99
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/credentials-cross-process.test.ts` around lines 52 - 62, Update
waitForFiles to accept injectable clock and sleep dependencies instead of
directly calling Date.now and setTimeout, while preserving its polling and
timeout behavior. Update the readiness and timeout-path tests to provide
controlled fake time and sleep advancement, removing reliance on real timer
delays and child-process scheduling.
Source: Path instructions
1 participant
Closes #279. Summary: Adds a child-process credentials writer helper and a regression test that releases eight child processes against one credentials file, asserting all profiles survive and the lock file is removed. Validation: npm test -- test/credentials-cross-process.test.ts; npm run typecheck; npx prettier --check test/credentials-cross-process.test.ts test/helpers/credentials-write-child.mjs; npm run lint -- test/credentials-cross-process.test.ts test/helpers/credentials-write-child.mjs.
Summary by CodeRabbit