test: cover cross-process credential writes#280
Conversation
WalkthroughAdds cross-process credential write coverage using concurrent child processes, validating that all profiles persist correctly and that the credentials lock file is removed afterward. 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
| describe('credentials cross-process writes', () => { | ||
| beforeAll(() => { | ||
| execNpm(['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,90p' test/credentials-cross-process.test.ts | cat -n
echo
echo "== package/test scripts and deps =="
if [ -f package.json ]; then
jq '{scripts, devDependencies, dependencies, packageManager}' package.json
fi
echo
echo "== relevant lock/vitest config if present =="
for f in vitest.config.* vite.config.* package-lock.json pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
if [ "$f" = package-lock.json ]; then
jq -r '.lockfileVersion, .packages[""].devDependencies.vitest // empty, .packages["node_modules/vitest"].version // empty' "$f"
else
rg -n "vitest|hookTimeout|testTimeout|timeout" "$f" | head -40 || true
fi
fi
done
echo
echo "== all vitest hookTimeout / beforeAll usages =="
rg -n "hookTimeout|beforeAll\\(" test package.json vitest.config.* vite.config.* 2>/dev/null || true
echo
echo "== repo build script excerpt =="
if [ -f package.json ]; then
jq -e '.scripts.build // empty' package.json
fiRepository: TestSprite/testsprite-cli
Length of output: 5634
🌐 Web query:
vitest v2 default hookTimeout beforeAll timeout documentation
💡 Result:
In Vitest, including v2, the default timeout for hooks like beforeAll is 10,000 milliseconds (10 seconds) when running in a Node.js environment [1][2][3]. If browser mode is enabled, the default timeout increases to 30,000 milliseconds (30 seconds) [1][3][4]. You can override this global setting using the hookTimeout configuration option [1][2]: // vitest.config.ts export default { test: { hookTimeout: 20000, // Sets hook timeout to 20 seconds }, } Additionally, you can specify a custom timeout for an individual beforeAll hook by passing the timeout as the second argument [2][5]: beforeAll(async => { // your setup logic }, 20000); // Individual timeout of 20 seconds Setting the timeout to 0 disables it completely [1][3][6].
Citations:
- 1: https://vitest.dev/config/hooktimeout
- 2: https://vitest.dev/api/hooks.html
- 3: https://docs.w3cub.com/vitest/config/hooktimeout
- 4: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/packages/vitest/src/node/config/resolveConfig.ts
- 5: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/hooks.md
- 6: https://vitest.dev/guide/cli
Set an explicit timeout for the beforeAll build hook.
This beforeAll runs npm run build via tsc and a postbuild script, but the hook uses Vitest’s default 10s timeout. That can fail the test run with a generic hook-timeout error instead of a clear build failure.
🔧 Suggested fix
- beforeAll(() => {
+ beforeAll(() => {
execNpm(['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' });
- });
+ }, 60_000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('credentials cross-process writes', () => { | |
| beforeAll(() => { | |
| execNpm(['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' }); | |
| }); | |
| describe('credentials cross-process writes', () => { | |
| beforeAll(() => { | |
| execNpm(['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' }); | |
| }, 60_000); |
🧰 Tools
🪛 ast-grep (0.44.1)
[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 - 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.
| it('preserves every profile written by concurrent child processes', async () => { | ||
| const tmpRoot = mkdtempSync(join(tmpdir(), 'testsprite-creds-race-')); | ||
| const credentialsPath = join(tmpRoot, 'credentials'); | ||
| const startPath = join(tmpRoot, 'start'); | ||
|
|
||
| try { | ||
| const profiles = Array.from({ length: 8 }, (_, index) => ({ | ||
| apiKey: `sk-child-${index}`, | ||
| profile: `child-${index}`, | ||
| })); | ||
| const children = profiles.map(({ apiKey, profile }) => | ||
| runCredentialWriter({ | ||
| CRED_API_KEY: apiKey, | ||
| CRED_PATH: credentialsPath, | ||
| CRED_PROFILE: profile, | ||
| CRED_START_PATH: startPath, | ||
| }), | ||
| ); | ||
|
|
||
| await new Promise(resolveDelay => setTimeout(resolveDelay, 50)); | ||
| writeFileSync(startPath, 'go'); | ||
|
|
||
| const results = await Promise.all(children); | ||
| expect(results).toEqual( | ||
| profiles.map(() => ({ | ||
| code: 0, | ||
| signal: null, | ||
| stderr: '', | ||
| stdout: '', | ||
| })), | ||
| ); | ||
|
|
||
| const credentials = readCredentialsFile({ path: credentialsPath }); | ||
| for (const { apiKey, profile } of profiles) { | ||
| expect(credentials[profile]).toEqual({ apiKey }); | ||
| } | ||
| expect(existsSync(`${credentialsPath}.lock`)).toBe(false); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Only the success path is covered; the helper's error/exit-code paths are untested.
The child helper documents three distinct failure exit codes (1: missing env vars, 2: start-marker timeout, 3: writeProfile error), but this suite only ever exercises the code: 0 success path across all 8 children. Per path instructions, new behavior — including error and exit-code paths — should be covered.
As per path instructions, "Cover new behavior, including error and exit-code paths." Consider adding focused cases, e.g. omitting a required env var (expect code 1) and pointing CRED_START_PATH at a marker that's never created with a shortened wait (expect code 2).
🧰 Tools
🪛 ast-grep (0.44.1)
[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 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.
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