Cli matrix testing - #276
Merged
Merged
Conversation
Add an ExecPlan for the requested CLI option matrix test harness. Capture the intended `assert_cmd` and `insta` structure, `.dat` fixture convention, harness self-tests, developer-guide update, and validation path before implementation begins.
Require every logical option combination to run through both file-to-stdout formatting and `--in-place` formatting. Treat `--wrap` as an explicit expansion over each selected non-wrap combination so the harness guards the wrapped and unwrapped regression surface directly.
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd a deterministic CLI matrix integration test harness with self‑tests and insta snapshot envelopes, document an ExecPlan and developer guidance, add Changes
Sequence Diagram(s)sequenceDiagram
participant TestHarness as "Test Harness\n(rust tests)"
participant TempFS as "Temporary FS\n(temp dir)"
participant Binary as "mdtablefix\n(binary via assert_cmd)"
participant SnapStore as "insta snapshots\n(snap files)"
participant Asserts as "Assertions\n(coverage & equality)"
TestHarness->>TempFS: create temp dir & stage fixture file
TestHarness->>Binary: run binary with args (flags, --in-place or stdout)
Binary-->>TestHarness: return stdout/stderr and exit status
Binary->>TempFS: modify input file when --in-place
TestHarness->>TempFS: read modified input file (if in-place)
TestHarness->>SnapStore: emit envelope (case id, mode, args, status, stdout, stderr, file bytes)
SnapStore-->>TestHarness: compare or update snapshot
TestHarness->>Asserts: assert stdout == in-place content for same logical case
TestHarness->>Asserts: assert coverage rules and unique IDs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideAdds a CLI option matrix integration test harness for the mdtablefix binary using assert_cmd + insta snapshots, supported by new .dat fixtures, self-tests for matrix coverage, and a small refactor in wrap postprocessing to satisfy clippy; also documents the harness and its workflow in the developer guide and an ExecPlan. Sequence diagram for cli_matrix_snapshots test executionsequenceDiagram
participant Test as cli_matrix_snapshots
participant Runner as CliMatrixRunner
participant Fixture as Fixture
participant TempFile as TempFile
participant Cmd as assert_cmd_Command
participant Bin as mdtablefix_binary
participant Insta as insta
Test->>Runner: all_logical_cases()
loop for each LogicalMatrixCase
Runner->>Runner: expand_to_physical_cases()
loop for each PhysicalMatrixCase
Runner->>Fixture: get path()
Fixture-->>Runner: fixture_path
Runner->>TempFile: copy fixture to temp file
TempFile-->>Runner: temp_path
Runner->>Cmd: cargo_bin(mdtablefix)
Runner->>Cmd: args(cli_args including temp_path)
Cmd->>Bin: spawn process
Bin-->>Cmd: exit_code, stdout, stderr
Cmd-->>Runner: Output
Runner->>TempFile: read temp file (for InPlace only)
TempFile-->>Runner: rewritten_content or empty
Runner->>Runner: build ResultEnvelope
Runner-->>Test: ResultEnvelope
Test->>Insta: assert_snapshot!(ResultEnvelope.format_for_snapshot())
end
Test->>Runner: assert_stdout_matches_in_place(LogicalMatrixCase)
Runner->>Runner: compare stdout vs rewritten_file for pair
end
Class diagram for CLI matrix harness typesclassDiagram
class NonWrapTransformFlag {
<<enum>>
RENumber
BREAKS
ELLIPSIS
FENCES
FOOTNOTES
CODE_EMPHASIS
HEADINGS
}
class WrapVariant {
<<enum>>
Nowrap
Wrap
}
class ExecMode {
<<enum>>
Stdout
InPlace
}
class Fixture {
+String id
+String path
+bool exists()
+bool has_dat_extension()
}
class BaseMatrixCase {
+String id
+Fixture fixture
+Set~NonWrapTransformFlag~ flags
+Vec~LogicalMatrixCase~ expand_to_logical_cases()
}
class LogicalMatrixCase {
+String id
+Fixture fixture
+Set~NonWrapTransformFlag~ flags
+WrapVariant wrap
+Vec~PhysicalMatrixCase~ expand_to_physical_cases()
}
class PhysicalMatrixCase {
+String id
+Fixture fixture
+Set~NonWrapTransformFlag~ flags
+WrapVariant wrap
+ExecMode exec_mode
+Vec~String~ cli_args()
+ResultEnvelope run()
}
class ResultEnvelope {
+String case_id
+WrapVariant wrap
+ExecMode exec_mode
+Vec~String~ args
+i32 exit_code
+String stdout
+String stderr
+Option~String~ rewritten_file
+String format_for_snapshot()
}
class CliMatrixRunner {
+Vec~BaseMatrixCase~ base_cases
+Vec~LogicalMatrixCase~ all_logical_cases()
+Vec~PhysicalMatrixCase~ all_physical_cases()
+ResultEnvelope run_case(PhysicalMatrixCase case_)
+void assert_stdout_matches_in_place(LogicalMatrixCase logical)
+void assert_snapshots()
}
class CliMatrixSelfTests {
+void matrix_case_ids_are_unique(CliMatrixRunner runner)
+void matrix_cases_expand_to_stdout_and_in_place(CliMatrixRunner runner)
+void matrix_cases_expand_to_wrapped_and_unwrapped(CliMatrixRunner runner)
+void matrix_cases_cover_all_transform_pairs(CliMatrixRunner runner)
+void matrix_fixtures_are_dat_and_exist(CliMatrixRunner runner)
}
class CarryPreviousInlineCodeTailHelper {
+bool carry_previous_inline_code_tail(
Vec~Vec~InlineFragment~~ merged,
Vec~InlineFragment~ pending_whitespace
)
}
class InlineFragment {
+FragmentKind kind
+bool is_plain()
}
class FragmentKind {
<<enum>>
InlineCode
Other
}
%% Relationships
BaseMatrixCase "1" o-- "1" Fixture
BaseMatrixCase "1" *-- "*" LogicalMatrixCase
LogicalMatrixCase "1" o-- "1" Fixture
LogicalMatrixCase "1" *-- "2" PhysicalMatrixCase
PhysicalMatrixCase "1" o-- "1" Fixture
PhysicalMatrixCase "1" o-- "1" WrapVariant
PhysicalMatrixCase "1" o-- "1" ExecMode
PhysicalMatrixCase "*" o-- "*" NonWrapTransformFlag
PhysicalMatrixCase "1" --> "1" ResultEnvelope
CliMatrixRunner "1" *-- "*" BaseMatrixCase
CliMatrixRunner "1" ..> ResultEnvelope
CliMatrixSelfTests ..> CliMatrixRunner
CarryPreviousInlineCodeTailHelper ..> InlineFragment
InlineFragment ..> FragmentKind
Flow diagram for CLI matrix case expansion and self-testsflowchart TD
A[Start matrix harness] --> B[Load BASE_MATRIX_CASES]
B --> C[Expand each BaseMatrixCase to wrapped and unwrapped LogicalMatrixCase]
C --> D[Expand each LogicalMatrixCase to Stdout and InPlace PhysicalMatrixCase]
D --> E[Self-test: matrix_case_ids_are_unique]
E --> F[Self-test: matrix_fixtures_are_dat_and_exist]
F --> G[Self-test: matrix_cases_expand_to_stdout_and_in_place]
G --> H[Self-test: matrix_cases_expand_to_wrapped_and_unwrapped]
H --> I[Self-test: matrix_cases_cover_all_transform_pairs]
I --> J{All self-tests pass?}
J -- No --> K[Fail cli_matrix harness tests]
J -- Yes --> L[Run cli_matrix_snapshots]
L --> M[For each PhysicalMatrixCase: run mdtablefix via assert_cmd]
M --> N[Build ResultEnvelope and assert_snapshot via insta]
N --> O[Assert stdout output equals InPlace rewritten file per LogicalMatrixCase]
O --> P[cli_matrix harness complete]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/cli-matrix-testing.md`:
- Line 25: The phrase "required paired variant" in the wrap-coverage requirement
should be rewritten to standard grammar and en-GB-oxendict style; locate the
sentence mentioning `--wrap` (the wrap-coverage requirement) and replace
"required paired variant" with either "required pair" or "required to be paired"
(e.g., "`--wrap` is required to be paired" or "`--wrap` requires a paired
option") so the sentence is clear and grammatically correct.
- Around line 340-343: The documented Git command is invalid: replace the usage
of "git branch --show" with the correct "git branch --show-current" in the CLI
execution steps so the workflow commands (the two-line snippet containing "pwd"
and "git branch --show") run correctly; update that code block to use "git
branch --show-current" instead.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30cf4944-9b8a-42e8-9f76-9f3fd564d301
📒 Files selected for processing (1)
docs/execplans/cli-matrix-testing.md
Address review feedback on the draft matrix ExecPlan by tightening the `--wrap` coverage sentence and replacing the invalid branch inspection command with `git branch --show-current`.
Implement the approved CLI matrix harness with `assert_cmd` and `insta`. The matrix covers seven non-wrap transform flags, expands every base row through wrapped and unwrapped variants, and runs each logical case through both file-to-stdout and `--in-place` execution. Add `.dat` fixtures so formatter runs do not rewrite matrix input data, commit the initial 32 snapshots, and document the harness workflow in the developer guide. Extract the existing inline-code whitespace carry logic in `postprocess.rs` into a helper so the warning-deny lint gate remains clean.
leynos
marked this pull request as ready for review
April 26, 2026 16:49
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 34: The dependency version for insta is too broad ("insta = \"1\"");
update the Cargo.toml entry for the insta dependency to an explicit, stable
minor version (e.g., change the insta declaration to something like insta =
"1.47" or later) so builds are reproducible and consistent with other deps such
as rayon and rstest.
In `@docs/execplans/cli-matrix-testing.md`:
- Around line 3-5: The markdown list has a stray leading space before the item
"Tolerances" which breaks list formatting; edit the line containing "Tolerances"
(the list item string `Tolerances`) and remove the leading space so it aligns
with the other list items, ensuring all bullet/list items use the same
indentation and consistent formatting.
In `@tests/cli_matrix/support.rs`:
- Around line 277-282: The is_case_id function currently forbids hyphens while
the ExecPlan permits them; update is_case_id (the function named is_case_id in
tests/cli_matrix/support.rs) to accept ASCII hyphen as well by including byte ==
b'-' in the predicate (alongside byte.is_ascii_lowercase(),
byte.is_ascii_digit(), and byte == b'_'), or alternatively amend the ExecPlan
documentation to remove hyphen from the allowed characters if hyphens are
intentionally disallowed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ebf937da-523c-4f68-8107-9b97d700380a
⛔ Files ignored due to path filters (37)
Cargo.lockis excluded by!**/*.locktests/data/cli-matrix/fences-ellipsis.datis excluded by!**/*.dattests/data/cli-matrix/footnotes.datis excluded by!**/*.dattests/data/cli-matrix/frontmatter-breaks.datis excluded by!**/*.dattests/data/cli-matrix/table-prose.datis excluded by!**/*.dattests/snapshots/cli_matrix__row_000_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_000_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_000_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_000_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_001_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_001_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_001_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_001_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_010_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_010_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_010_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_010_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_011_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_011_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_011_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_011_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_100_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_100_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_100_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_100_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_101_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_101_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_101_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_101_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_110_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_110_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_110_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_110_wrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_111_nowrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_111_nowrap_stdout.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_111_wrap_in_place.snapis excluded by!**/*.snaptests/snapshots/cli_matrix__row_111_wrap_stdout.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
Cargo.tomldocs/developers-guide.mddocs/execplans/cli-matrix-testing.mdsrc/wrap/inline/postprocess.rstests/cli_matrix.rstests/cli_matrix/support.rs
This comment was marked as resolved.
This comment was marked as resolved.
Pin `insta` to the current minor line, fix the ExecPlan heading text indentation, and allow hyphens in matrix case identifiers so the harness matches the documented case-id contract.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15153 🏁 Script executed: Length of output: 4854 🏁 Script executed: Length of output: 7678
Status
AI agent promptsPrompt A — Fix
|
Preserve the original fixture filename when copying matrix inputs into temporary directories so failed runs are easier to inspect. Make the inline-code tail invariant explicit with `expect` after the preceding `last()` check, and add a focused assertion that matrix case IDs accept the documented hyphen character.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/cli-matrix-testing.md`:
- Around line 29-31: Remove the stale "This plan is draft-only until approved."
disclaimer and replace it with a short completion-state sentence that reflects
the plan is finished (e.g., "This ExecPlan is complete and approved."), ensure
the document text (the new completion statement) is consistent with the existing
`Status: COMPLETE` field and with the implemented outcomes recorded later, and
remove or update the reference to `docs/developers-guide.md` so it no longer
implies outstanding work.
In `@tests/cli_matrix/support.rs`:
- Around line 12-293: The build fails the documentation gate because many
pub(crate) items in this test harness lack Rustdoc; either add short /// doc
comments to each exposed item (e.g., TransformFlag, TransformFlag::as_arg,
BaseCase, WrapVariant, WrapVariant::id_part, ExecutionMode,
ExecutionMode::id_part, LogicalCase, PhysicalCase, RunResult, ALL_FLAGS,
BASE_MATRIX_CASES, RunResult::envelope, PhysicalCase::snapshot_name,
PhysicalCase::args, logical_cases, physical_cases, run_physical_case,
fixture_path, is_case_id, non_wrap_signature, has_flag) describing their
purpose, parameters and return where applicable, or reduce visibility from
pub(crate) to private for any item not required outside the module; update docs
for fields as well (e.g., LogicalCase.id/fixture/is_wrapped/flags) so the doc
gate passes.
- Around line 247-267: Refactor run_physical_case to stop panicking and instead
return a Result<RunResult, E>: replace all .expect() and assert!-style checks
inside run_physical_case with ?-based error propagation, split responsibilities
into small helpers (e.g., stage_fixture -> creates tempdir, copies fixture and
returns file_path; run_command -> spawns Command::cargo_bin("mdtablefix") with
args and returns Output; collect_result -> reads file_content and constructs
RunResult) and have run_physical_case call them and return Ok(RunResult). Move
the status.success() assertion out of run_physical_case into callers/tests so
the fixture layer never asserts; keep identifiers like run_physical_case,
RunResult, tempdir(), fixture_path, Command::cargo_bin("mdtablefix"), and
file_path to locate the code to change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c76454c4-d114-4383-a9fe-03a5f7757a88
📒 Files selected for processing (3)
Cargo.tomldocs/execplans/cli-matrix-testing.mdtests/cli_matrix/support.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/cli_matrix/support.rs (2)
247-267:⚠️ Potential issue | 🟠 MajorPropagate fixture errors with
Resultand remove panicking calls from shared helpers.Refactor
run_physical_caseto returnResult<RunResult>and replace.expect()/assert!with?plus context. Split staging, execution, and collection into dedicated helpers. Performstatus.success()assertions in the calling tests.Proposed refactor
use std::{ fs, path::{Path, PathBuf}, process::Output, }; +use anyhow::{Context, Result}; use assert_cmd::Command; -use tempfile::tempdir; +use tempfile::{tempdir, TempDir}; -pub(crate) fn run_physical_case(case: &PhysicalCase) -> RunResult { - let dir = tempdir().expect("create temporary directory"); - let file_path = dir.path().join(case.logical.fixture); - fs::copy(fixture_path(case.logical.fixture), &file_path).expect("copy matrix fixture"); - - let mut command = Command::cargo_bin("mdtablefix").expect("create mdtablefix command"); - command.args(case.args()).arg(&file_path); - let output = command.output().expect("run mdtablefix"); - assert!( - output.status.success(), - "{} failed with stderr:\n{}", - case.snapshot_name(), - String::from_utf8_lossy(&output.stderr), - ); - - let file_content = fs::read(&file_path).expect("read matrix output file"); - RunResult { - output, - file_content, - } +pub(crate) fn run_physical_case(case: &PhysicalCase) -> Result<RunResult> { + let (_dir, file_path) = stage_fixture(case)?; + let output = execute_case(case, &file_path)?; + let file_content = fs::read(&file_path) + .with_context(|| format!("read matrix output file {}", file_path.display()))?; + Ok(RunResult { output, file_content }) +} + +fn stage_fixture(case: &PhysicalCase) -> Result<(TempDir, PathBuf)> { + let dir = tempdir().context("create temporary directory")?; + let file_path = dir.path().join(case.logical.fixture); + fs::copy(fixture_path(case.logical.fixture), &file_path) + .with_context(|| format!("copy matrix fixture to {}", file_path.display()))?; + Ok((dir, file_path)) +} + +fn execute_case(case: &PhysicalCase, file_path: &Path) -> Result<Output> { + let mut command = Command::cargo_bin("mdtablefix").context("create mdtablefix command")?; + command.args(case.args()).arg(file_path); + command.output().context("run mdtablefix") }#!/bin/bash set -euo pipefail FILE="tests/cli_matrix/support.rs" echo "== run_physical_case signature ==" rg -nP 'pub\(crate\)\s+fn\s+run_physical_case\s*\(' "$FILE" -A20 -B2 echo echo "== panicking calls in shared helper module ==" rg -nP '\.expect\(|assert!\(' "$FILE"As per coding guidelines,
**/*.rs: "In Rust production code and shared fixtures, avoid.expect()entirely; returnResultand use?to propagate errors instead of panicking."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_matrix/support.rs` around lines 247 - 267, Change run_physical_case to return a Result<RunResult, E> (propagate a suitable error type like anyhow::Error) and stop panicking inside it: break the current body into three helpers (e.g., stage_fixture, execute_mdtablefix, collect_output) referenced from run_physical_case; in those helpers replace all .expect() calls with ? plus contextual errors and return Results, and remove the assert!(output.status.success()) from inside run_physical_case so test callers perform the success assertion themselves; keep identifiers PhysicalCase, RunResult, and the existing command building logic (Command::cargo_bin("mdtablefix")) but propagate errors via ? and return Ok(RunResult { output, file_content }) on success.
12-293:⚠️ Potential issue | 🟠 MajorDocument exposed harness APIs or reduce visibility immediately.
Add
///Rustdoc comments for allpub(crate)enums, structs, public fields, constants, methods, and free functions in this module, or make non-shared items private now.#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path path = Path("tests/cli_matrix/support.rs") lines = path.read_text().splitlines() for idx, line in enumerate(lines, start=1): s = line.strip() if "pub(crate)" in s and not s.startswith("//"): j = idx - 1 while j > 0 and lines[j-1].strip() == "": j -= 1 prev = lines[j-1].strip() if j > 0 else "" if not prev.startswith("///"): print(f"Line {idx}: missing Rustdoc -> {s}") PYAs per coding guidelines,
**/*.rs: "Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_matrix/support.rs` around lines 12 - 293, Many items in this module are exported with pub(crate) but lack Rustdoc; either add brief /// comments for each exported symbol (at minimum a one-line description) or reduce their visibility to private if they are not intended to be shared: apply this to enums and methods TransformFlag and TransformFlag::as_arg, BaseCase, WrapVariant and WrapVariant::id_part, ExecutionMode and ExecutionMode::id_part, LogicalCase, PhysicalCase and PhysicalCase::snapshot_name/PhysicalCase::args, RunResult and RunResult::envelope, constants ALL_FLAGS and BASE_MATRIX_CASES, and free functions logical_cases, physical_cases, run_physical_case, fixture_path, is_case_id, non_wrap_signature, has_flag; ensure each pub(crate) item receives a /// doc comment placed immediately above its declaration or change the visibility to remove pub(crate).tests/cli_matrix.rs (1)
127-155:⚠️ Potential issue | 🟠 MajorAdd behavioural assertions and stop using snapshots as the sole oracle.
Assert transform semantics explicitly in
cli_matrix_snapshotsbefore snapshotting. Validate concrete enabled/disabled outcomes per transform on focused cases, then keep snapshots for regression visibility only.Based on learnings, "Create or update CLI matrix snapshots only when the behaviour change is intentional; review generated
tests/snapshots/cli_matrix__*.snapfiles before committing and inspect snapshot churn to detect if a fixture is too broad or a shared transform changed behaviour."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_matrix.rs` around lines 127 - 155, In cli_matrix_snapshots, add explicit behavioral assertions before the insta snapshots: for a few focused logical cases (from logical_cases()) call run_physical_case (as done for stdout_result and in_place_result) and assert concrete transform outcomes (e.g., enabled/disabled flags, expected output fragments or presence/absence of transforms) by inspecting stdout_result.output and in_place_result.file_content or the structures returned by envelope(&stdout_case); then keep the existing insta::assert_snapshot! calls only for regression visibility. Target the same symbols used here (cli_matrix_snapshots, logical_cases, PhysicalCase, ExecutionMode::Stdout/InPlace, run_physical_case, snapshot_name, envelope) so the new assertions live adjacent to the snapshotting and validate semantics before snapshot updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wrap/inline/postprocess.rs`:
- Around line 34-57: The helper carry_previous_inline_code_tail currently uses
.expect(...) which can panic; change the pop() call on previous_line to a
non-panicking pattern (e.g., match or let Some(previous_atomic) =
previous_line.pop() else { return false; }) so the function returns false
instead of panicking when no element is present, then push previous_atomic into
pending_whitespace and keep the existing logic that pops merged if previous_line
becomes empty; ensure you reference the same symbols (merged,
pending_whitespace, previous_line, previous_atomic, InlineFragment,
FragmentKind) and preserve the function's boolean return contract.
---
Duplicate comments:
In `@tests/cli_matrix.rs`:
- Around line 127-155: In cli_matrix_snapshots, add explicit behavioral
assertions before the insta snapshots: for a few focused logical cases (from
logical_cases()) call run_physical_case (as done for stdout_result and
in_place_result) and assert concrete transform outcomes (e.g., enabled/disabled
flags, expected output fragments or presence/absence of transforms) by
inspecting stdout_result.output and in_place_result.file_content or the
structures returned by envelope(&stdout_case); then keep the existing
insta::assert_snapshot! calls only for regression visibility. Target the same
symbols used here (cli_matrix_snapshots, logical_cases, PhysicalCase,
ExecutionMode::Stdout/InPlace, run_physical_case, snapshot_name, envelope) so
the new assertions live adjacent to the snapshotting and validate semantics
before snapshot updates.
In `@tests/cli_matrix/support.rs`:
- Around line 247-267: Change run_physical_case to return a Result<RunResult, E>
(propagate a suitable error type like anyhow::Error) and stop panicking inside
it: break the current body into three helpers (e.g., stage_fixture,
execute_mdtablefix, collect_output) referenced from run_physical_case; in those
helpers replace all .expect() calls with ? plus contextual errors and return
Results, and remove the assert!(output.status.success()) from inside
run_physical_case so test callers perform the success assertion themselves; keep
identifiers PhysicalCase, RunResult, and the existing command building logic
(Command::cargo_bin("mdtablefix")) but propagate errors via ? and return
Ok(RunResult { output, file_content }) on success.
- Around line 12-293: Many items in this module are exported with pub(crate) but
lack Rustdoc; either add brief /// comments for each exported symbol (at minimum
a one-line description) or reduce their visibility to private if they are not
intended to be shared: apply this to enums and methods TransformFlag and
TransformFlag::as_arg, BaseCase, WrapVariant and WrapVariant::id_part,
ExecutionMode and ExecutionMode::id_part, LogicalCase, PhysicalCase and
PhysicalCase::snapshot_name/PhysicalCase::args, RunResult and
RunResult::envelope, constants ALL_FLAGS and BASE_MATRIX_CASES, and free
functions logical_cases, physical_cases, run_physical_case, fixture_path,
is_case_id, non_wrap_signature, has_flag; ensure each pub(crate) item receives a
/// doc comment placed immediately above its declaration or change the
visibility to remove pub(crate).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d6cd945-a299-420a-ab7f-0de0039a6579
📒 Files selected for processing (3)
src/wrap/inline/postprocess.rstests/cli_matrix.rstests/cli_matrix/support.rs
Add concise Rustdoc comments for the matrix support types, fields, constants, methods, and helper functions so the test harness support module has explicit internal documentation.
Split fixture staging and result collection out of the matrix runner so I/O and command construction failures propagate as errors. Keep process success assertions in the snapshot test body where each stdout and in-place invocation is visible beside its snapshots.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/cli_matrix/support.rs (1)
285-305:⚠️ Potential issue | 🟠 MajorRefactor
run_physical_caseto propagate errors and stop panicking.Split this helper into staging/execution/collection steps and return
Result<RunResult, _>. Remove.expect()at Line 286, Line 288, Line 290, Line 292, and Line 300. Removeassert!at Line 293-298 and assertstatus.success()in the test caller.🔧 Proposed refactor
use std::{ fs, path::{Path, PathBuf}, process::Output, }; use assert_cmd::Command; -use tempfile::tempdir; +use tempfile::{tempdir, TempDir}; +type HarnessResult<T> = Result<T, Box<dyn std::error::Error + Send + Sync>>; /// Runs a physical matrix case through the real `mdtablefix` binary. -pub(crate) fn run_physical_case(case: &PhysicalCase) -> RunResult { - let dir = tempdir().expect("create temporary directory"); - let file_path = dir.path().join(case.logical.fixture); - fs::copy(fixture_path(case.logical.fixture), &file_path).expect("copy matrix fixture"); - - let mut command = Command::cargo_bin("mdtablefix").expect("create mdtablefix command"); - command.args(case.args()).arg(&file_path); - let output = command.output().expect("run mdtablefix"); - assert!( - output.status.success(), - "{} failed with stderr:\n{}", - case.snapshot_name(), - String::from_utf8_lossy(&output.stderr), - ); - - let file_content = fs::read(&file_path).expect("read matrix output file"); - RunResult { - output, - file_content, - } +pub(crate) fn run_physical_case(case: &PhysicalCase) -> HarnessResult<RunResult> { + let (_dir, file_path) = stage_fixture(case)?; + let output = run_command(case, &file_path)?; + let file_content = collect_result(&file_path)?; + Ok(RunResult { output, file_content }) } + +fn stage_fixture(case: &PhysicalCase) -> HarnessResult<(TempDir, PathBuf)> { + let dir = tempdir()?; + let file_path = dir.path().join(case.logical.fixture); + fs::copy(fixture_path(case.logical.fixture), &file_path)?; + Ok((dir, file_path)) +} + +fn run_command(case: &PhysicalCase, file_path: &Path) -> HarnessResult<Output> { + let mut command = Command::cargo_bin("mdtablefix")?; + command.args(case.args()).arg(file_path); + Ok(command.output()?) +} + +fn collect_result(file_path: &Path) -> HarnessResult<Vec<u8>> { + Ok(fs::read(file_path)?) +}As per coding guidelines,
**/*.rs: “In Rust production code and shared fixtures, avoid.expect()entirely; returnResultand use?to propagate errors instead of panicking.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_matrix/support.rs` around lines 285 - 305, Change run_physical_case to return a Result<RunResult, E> (e.g., Result<RunResult, Box<dyn std::error::Error>> or a crate Result type) and stop panicking: replace tempdir().expect(...), fs::copy(...).expect(...), Command::cargo_bin(...).expect(...), command.output().expect(...), and fs::read(...).expect(...) with ? to propagate errors; remove the assert! block that checks output.status.success() so this helper only stages (create temp file), executes (run Command via Command::cargo_bin and command.output()), and collects (read file) and then return Ok(RunResult { output, file_content }); update the test caller to call run_physical_case(...)?.output.status.success() (or otherwise assert status.success() there). Reference: function run_physical_case, types PhysicalCase and RunResult, tempdir(), fixture_path(), Command::cargo_bin(), command.output(), and fs::read().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/cli_matrix/support.rs`:
- Around line 285-305: Change run_physical_case to return a Result<RunResult, E>
(e.g., Result<RunResult, Box<dyn std::error::Error>> or a crate Result type) and
stop panicking: replace tempdir().expect(...), fs::copy(...).expect(...),
Command::cargo_bin(...).expect(...), command.output().expect(...), and
fs::read(...).expect(...) with ? to propagate errors; remove the assert! block
that checks output.status.success() so this helper only stages (create temp
file), executes (run Command via Command::cargo_bin and command.output()), and
collects (read file) and then return Ok(RunResult { output, file_content });
update the test caller to call run_physical_case(...)?.output.status.success()
(or otherwise assert status.success() there). Reference: function
run_physical_case, types PhysicalCase and RunResult, tempdir(), fixture_path(),
Command::cargo_bin(), command.output(), and fs::read().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef586867-a052-47f4-8e77-6590db97657f
📒 Files selected for processing (1)
tests/cli_matrix/support.rs
Add explicit stdout assertions for matrix rows that exercise ellipsis, fence, and list-renumber transforms before the insta snapshots are recorded.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_matrix.rs`:
- Around line 127-170: In cli_matrix_snapshots, before the
insta::assert_snapshot! calls add explicit semantic assertions that verify
transform-specific invariants on stdout_result and in_place_result (e.g., that
fence delimiters are normalized, that expected headers/footers or exit-code
semantics are present, and that stdout_result.output.stdout equals
in_place_result.file_content) so snapshots aren’t the sole oracle; place these
checks immediately after the assert_eq! comparing stdout and file_content and
before the two insta::assert_snapshot! calls, referencing cli_matrix_snapshots,
stdout_result, in_place_result, stdout_case, and in_place_case to locate where
to insert the assertions.
In `@tests/cli_matrix/support.rs`:
- Around line 286-316: Change the three fixture helper signatures from
Result<..., Box<dyn Error>> to anyhow::Result and add contextual .context(...)
calls for each fallible op: in stage_fixture add context to fs::copy (including
the source fixture and target path) and to any fixture_path resolution; in
collect_result add context to fs::read (including the file_path and
ExecutionMode); and in run_physical_case add context to tempdir creation,
Command::cargo_bin("mdtablefix") creation, and command.output() (including the
case.args or file_path where useful). Update uses of these functions to
propagate anyhow errors as needed and import anyhow::{Result, Context}.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83257bac-b2c4-4e09-8fa5-f46567ae658c
📒 Files selected for processing (2)
tests/cli_matrix.rstests/cli_matrix/support.rs
Replace the stale draft-only note with a completion statement that matches the completed harness and developer-guide documentation.
Return false when the previous inline-code fragment cannot be popped, preserving the helper contract without panicking in the wrap postprocess path.
leynos
commented
Apr 26, 2026
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. Please address the comments from this code review: Overall Comments
Individual CommentsComment 1+ return false; + } + + let Some(previous_atomic) = previous_line.pop() else { + return false; + }; + pending_whitespace.push(previous_atomic); **suggestion (bug_risk):** The `None` branch after checking `last().is_some_and(..)` is logically unreachable and may hide invariants breaking.Since Instead, make the invariant explicit, e.g.: let previous_atomic = previous_line
.pop()
.expect("inline code tail must be present after last() check");or use a </issue_to_address> |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This was referenced May 27, 2026
This was referenced Jun 4, 2026
Closed
Closed
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a CLI option matrix integration test harness with snapshot-based verification and supporting documentation, plus a small refactor to satisfy linting in the wrapping postprocessor.
New Features:
Enhancements:
Build:
Documentation:
Tests: