fix(reftest): honour compare subcommand flags#684
Conversation
The compare subcommand's --threshold and --json flags were silently ignored — values stayed at subcommand defaults regardless of CLI input. Root cause: Commander v12 by-design behavior when long option names collide between a root command (with an .action() handler) and a subcommand. CLI values bind to the root's option store; the subcommand action receives only its local defaults. Upgrading commander doesn't help — confirmed unchanged through v14 (tj/commander.js#2356). Fix: read this.optsWithGlobals() inside the compare action, the pattern the Commander maintainer recommends for this scenario. Add a regression test that spawns the built CLI to verify --threshold, -t, and --json all wire through.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
WalkthroughA new Vitest regression test suite for the CLI Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 260f4e6b8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!fs.existsSync(CLI)) { | ||
| throw new Error( | ||
| `Built CLI not found at ${CLI}. Run \`pnpm --filter @grida/reftest build\` first.` | ||
| ); |
There was a problem hiding this comment.
The new CLI regression suite hard-fails in beforeAll when ../dist/cli.js is absent, which makes pnpm --filter @grida/reftest test fail on a clean checkout even though the package test script only runs Vitest and does not build first. This introduces a brittle ordering dependency (build → test) into a unit-test target that previously ran standalone, so local and CI test runs that execute tests directly will now fail before assertions run.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/grida-reftest/__tests__/cli.test.ts (1)
1-1: Clean up the temporary fixture directory after the suite.The test writes PNGs into an OS temp directory but never removes it, so repeated local runs will accumulate
reftest-cli-*directories.Proposed cleanup
-import { describe, expect, it, beforeAll } from "vitest"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; @@ describe("reftest CLI — compare subcommand", () => { let aPath: string; let bPath: string; + let dir: string | undefined; beforeAll(() => { @@ - const dir = fs.mkdtempSync(path.join(os.tmpdir(), "reftest-cli-")); + dir = fs.mkdtempSync(path.join(os.tmpdir(), "reftest-cli-")); aPath = path.join(dir, "a.png"); bPath = path.join(dir, "b.png"); @@ fs.writeFileSync(bPath, makeSolidPng(10, 10, [120, 120, 120, 255])); }); + + afterAll(() => { + if (dir) fs.rmSync(dir, { recursive: true, force: true }); + });Also applies to: 32-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-reftest/__tests__/cli.test.ts` at line 1, Add a cleanup hook to remove the OS temp fixture directory after the test suite: import afterAll from vitest (consistent with the existing beforeAll), capture the temp fixture directory variable used by the tests (e.g., tmpDir / fixtureDir) and in an afterAll call remove it recursively (fs.rm or fs.promises.rm with { recursive: true, force: true }) and handle any errors silently; place this afterAll in the same test suite in packages/grida-reftest/__tests__/cli.test.ts so repeated runs don't leave reftest-cli-* folders behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/grida-reftest/__tests__/cli.test.ts`:
- Line 1: Add a cleanup hook to remove the OS temp fixture directory after the
test suite: import afterAll from vitest (consistent with the existing
beforeAll), capture the temp fixture directory variable used by the tests (e.g.,
tmpDir / fixtureDir) and in an afterAll call remove it recursively (fs.rm or
fs.promises.rm with { recursive: true, force: true }) and handle any errors
silently; place this afterAll in the same test suite in
packages/grida-reftest/__tests__/cli.test.ts so repeated runs don't leave
reftest-cli-* folders behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af0bae82-f9a1-4262-80ec-4595f71ecb89
📒 Files selected for processing (2)
packages/grida-reftest/__tests__/cli.test.tspackages/grida-reftest/src/cli.ts
1 participant
Summary
reftest compare <a> <b> --threshold 0 --jsonsilently ignored both flags — the subcommand action received its declared defaults ("0.1"/false) regardless of what the user typed on the CLI..action()handler) and a subcommand, CLI values bind to the root's option store; the subcommand'soptsarg only exposes local defaults. Confirmed unchanged through v14 (tj/commander.js#2356, #2335) — upgrading isn't a fix.this.optsWithGlobals()inside the compare action (maintainer-recommended pattern). Added a comment explaining the Commander quirk.__tests__/cli.test.tsspawns the built CLI vianode dist/cli.js compare …and asserts--threshold,-t, and--jsonall wire through end-to-end.Test plan
pnpm --filter @grida/reftest buildpnpm --filter @grida/reftest test— 49 pass (5 new + 44 pre-existing)pnpm --filter @grida/reftest typecheck— cleannode dist/cli.js compare /tmp/a.png /tmp/b.png --threshold 0 --jsonnow returnsdiff_pixels: 100(was0), and prints JSON (was plain text).Summary by CodeRabbit
Tests
Bug Fixes