Skip to content

test(cli): regression for #121 — FakeDriver mobile rect in JSON output - #137

Merged
aram-devdocs merged 1 commit into
mainfrom
test/121-fakedriver-rect-cli-regression
Apr 28, 2026
Merged

test(cli): regression for #121 — FakeDriver mobile rect in JSON output#137
aram-devdocs merged 1 commit into
mainfrom
test/121-fakedriver-rect-cli-regression

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

End-to-end CLI regression test for issue #121. PR #125 fixed the
FakeDriver so multi-viewport runs against plumb-fake://hello no
longer emit the canned desktop rect for the mobile viewport. The fix
is unit-tested at the FakeDriver::snapshot layer
(crates/plumb-cdp/tests/snapshot_all_default.rs::fake_driver_rewrites_viewport_sized_rects_to_target).
The original issue, however, was reported at the CLI layer — plumb lint plumb-fake://hello --format json showing 1280x800 rects under
a mobile viewport. This PR adds the matching CLI-level assertion so
the regression is guarded at the level the issue was reported.

Why this exists (audit follow-up)

PR #125 merged with the Claude review check failed because the
review action hit its SDK turn limit (Reached maximum number of turns (20)), an infrastructure failure rather than a review finding.
All other gates (tests, fmt, clippy, determinism, coverage, MSRV,
cargo-deny, three-OS test matrix) passed. There is therefore no
recorded approval verdict on #125, only passing real CI. This PR
closes the acceptance-criteria validation gap so the fix is covered
end-to-end.

What the test asserts

plumb lint plumb-fake://hello --viewport mobile --format json against
a [viewports.mobile] width=375 height=812 config MUST:

Test plan

  • cargo test -p plumb-cli --test cli_integration — all 22 tests pass
  • cargo fmt --all -- --check — clean
  • cargo clippy -p plumb-cli --all-targets -- -D warnings — clean
  • CI green

Refs #121.

Add an end-to-end CLI regression for #121 against the JSON
formatter: `plumb lint plumb-fake://hello --viewport mobile
--format json` against a `[viewports.mobile]` config MUST emit
`"width": 375 / "height": 812` rects, not the canned desktop
`1280x800`. PR #125's fix is at the FakeDriver layer; this guards
the symptom at the layer the issue was reported (CLI + JSON).

Audit follow-up: PR #125 merged with the Claude review check
failed (SDK max-turns infrastructure error, not a code finding)
so no recorded approval verdict exists. All other gates passed.
This PR adds the missing CLI-level regression test so the
acceptance-criteria check is durable.

Refs #121.
@github-actions

Copy link
Copy Markdown
Contributor

I have a complete picture of the change. Here is the review.


PR #137test(cli): regression for #121 — FakeDriver mobile rect in JSON output

Scope: one file, crates/plumb-cli/tests/cli_integration.rs, +28 lines, 0 deletions. No production code changed.


1. Determinism invariants — PASS

No wall-clock, no HashMap/HashSet in output paths. TempDir paths never appear in any assertion (the test binds to .current_dir() but asserts only on stdout content). ✅

2. Workspace layering — PASS

Change is confined to plumb-cli/tests/. Imports are: assert_cmd, predicates, tempfile — all test-side deps already present. No cross-layer dependency introduced. ✅

3. Error handling — PASS (N/A for test code)

Integration test returns Box<dyn std::error::Error>, matching the established pattern in this file. No unwrap/expect added anywhere. ✅

4. Test coverage — PASS

The new test closes the acceptance gap described in the PR body:

  • code(3) correct per CLAUDE.md exit-code table (warning-only).
  • "viewport": "mobile" verifies the JSON label.
  • "width": 375 / "height": 812 verify the mobile rect dimensions.
  • .stdout(contains("\"width\": 1280").not()) / .stdout(contains("\"height\": 800").not()) are the load-bearing regression guards against the pre-fix(cdp): scale FakeDriver viewport rects to configured target #125 canned desktop dimensions leaking into mobile output.

The helper workspace_with_two_viewports() is the same fixture used by lint_runs_every_configured_viewport_when_flag_absent and lint_with_unknown_viewport_exits_input_error — no new fixture code needed. ✅

5. Documentation — PASS

The /// doc comment explains the regression context with issue numbers and uses RFC 2119 MUST correctly. Consistent with adjacent tests. ✅


Punch list

No blockers, no warnings. Single minor observation (no action required):

  • crates/plumb-cli/tests/cli_integration.rs:386.stdout(contains("\"width\": 375")) is a substring match that would technically also pass if the JSON output ever echoed the viewport config (not just the rect). Today the lint JSON output does not echo config, so this is benign — but if the format ever grows a config-echo field, the positive assertion becomes weaker than the .not() guards. The .not() assertions remain the authoritative regression guards regardless.

Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit 857d70d into main Apr 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant