test: add integration canaries for at, diff, review --batch, merge#221
test: add integration canaries for at, diff, review --batch, merge#221flyingrobots merged 4 commits intomainfrom
Conversation
Address round-3 CodeRabbit feedback requesting canary tests for remaining CLI commands. Adds 4 new integration tests: - at HEAD --json → at.schema.json - diff HEAD~1..HEAD --json → diff.schema.json - review --batch accept --json → review-batch.schema.json - merge --from <repo> --dry-run --json → merge.schema.json suggest --json is intentionally skipped (requires GITMIND_AGENT). 371 tests across 22 files.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRevises test/contracts.integration.test.js to replace direct git child-process calls with execFileSync, add a gitCommit(cwd, filename, message) helper, create two temporary repositories with deterministic commit history, and extend CLI JSON-schema validation tests for at, diff, review (batch), and merge (dry-run). Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 3: The package.json "version" field was bumped to 3.0.1 for a tests-only
change; either revert that change (restore the previous "version" value in
package.json) so the PR contains no consumer-facing version update, or if the
bump is intentional, add an explicit note in the PR description and changelog
explaining why tests require a version bump and keep the "version" change;
locate and update the "version" field in package.json accordingly and remove any
commit that only exists to bump the version if it isn't warranted.
In `@test/contracts.integration.test.js`:
- Around line 39-44: The gitCommit helper uses execSync with a
shell-interpolated string which is fragile; change it to use execFileSync with
argument arrays to avoid shell interpretation: replace execSync(`git add
${filename}` ...) with execFileSync('git', ['add', filename], {cwd, stdio:
'ignore'}), replace execSync(`git commit -m "${message}"` ...) with
execFileSync('git', ['commit', '-m', message], {cwd, stdio: 'ignore'}), and
replace the rev-parse call with execFileSync('git', ['rev-parse', 'HEAD'], {cwd,
encoding: 'utf-8'}). Keep the same option keys (cwd, stdio/encoding) as used
elsewhere.
- Use execFileSync with argument arrays in gitCommit helper to avoid shell interpolation (CodeRabbit) - Revert version bump — tests-only change doesn't warrant a version change (CodeRabbit)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/contracts.integration.test.js`:
- Around line 82-88: Replace the execSync shell-invocation calls used when
setting up mergeDir with execFileSync-style calls that pass the command and
arguments separately to avoid shell interpolation: change execSync('git init', {
cwd: mergeDir, ... }) to execFileSync('git', ['init'], { cwd: mergeDir, stdio:
'ignore' }), and change the two execSync('git config ...', { cwd: mergeDir, ...
}) lines to execFileSync('git', ['config', 'user.email', 'test@test.com'], {
cwd: mergeDir, stdio: 'ignore' }) and execFileSync('git', ['config',
'user.name', 'Test'], { cwd: mergeDir, stdio: 'ignore' }); keep the existing
execFileSync calls that run the BIN commands (execFileSync(process.execPath,
[BIN, ...], { cwd: mergeDir, stdio: 'ignore' })) unchanged; apply the same
execSync→execFileSync pattern to the tempDir setup lines if you want full
consistency with gitCommit and other helpers.
- Convert all git init/config calls to execFileSync with argument arrays (both tempDir and mergeDir setup) - Remove unused execSync import - Add 30s timeout to beforeAll (expanded setup with two repos + epochs exceeds vitest's default 10s hook timeout)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/contracts.integration.test.js`:
- Around line 221-232: The test "review --batch accept --json validates against
review-batch.schema.json" mutates shared review state by accepting pending
items; add a concise inline comment above this it(...) noting the side-effect
and ordering constraint (e.g., "// NOTE: mutates review state — keep after
review --json test") so future contributors know this test must remain after
other tests that rely on pending reviews; reference the test block name,
tempDir, and runCli to locate where to add the comment.
Document the test-ordering constraint: review --batch accept mutates shared state, so it must run after any test that expects pending items.
Summary
at,diff,review --batch, andmergecommands against their JSON Schema contractssuggestis intentionally skipped (requiresGITMIND_AGENTenv var / LLM agent)process-commit) and a second temp repo for merge testingCloses #205
Test plan
npx vitest run)at HEAD --jsonvalidates againstat.schema.jsondiff HEAD~1..HEAD --jsonvalidates againstdiff.schema.jsonreview --batch accept --jsonvalidates againstreview-batch.schema.jsonmerge --from <repo> --dry-run --jsonvalidates againstmerge.schema.jsonSummary by CodeRabbit