Repository navigation
test(input-matrix): make the SAM/stdin comparisons non-vacuous - #663
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe integration matrix now declares whether command output must depend on input, generates populated and drained fixtures across input formats, and compares their outputs. New RX/MI-tagged duplex and single-strand BAM builders support expanded command-shape coverage. ChangesInput dependence validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InputSourceMatrix
participant FixtureBuilder
participant Command
participant OutputComparator
InputSourceMatrix->>FixtureBuilder: create populated fixture
InputSourceMatrix->>Command: run with populated input
InputSourceMatrix->>FixtureBuilder: create drained fixture
InputSourceMatrix->>Command: run with drained input
Command-->>OutputComparator: populated and drained outputs
OutputComparator-->>InputSourceMatrix: detect identical output
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
==========================================
+ Coverage 93.80% 93.91% +0.10%
==========================================
Files 177 177
Lines 107658 107726 +68
==========================================
+ Hits 100990 101172 +182
+ Misses 6668 6554 -114 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5650d82 to
3d49b1b
Compare
95ea088 to
2597997
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/integration/test_input_source_matrix.rs`:
- Around line 996-1000: Restrict the early return in the drained-run handling to
the specific expected empty-input rejection, rather than accepting every
non-zero exit from drained. Use the command’s existing status, stderr, or error
classification to identify and exempt only that known refusal; allow unrelated
failures such as invalid flags, missing companions, rejected headers, or panics
to fail the test and reach output comparison as appropriate.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 276b3f8c-fa5b-4574-b7d6-51a63184cfab
📒 Files selected for processing (2)
tests/integration/helpers/bam_generator.rstests/integration/test_input_source_matrix.rs
2597997 to
3d78ecb
Compare
The SAM and stdin matrices compare a command's output against the same command reading a named path. That is only evidence when the output depends on the records that went in, and for five commands it did not: `simplex`, `duplex` and `codec` were handed ungrouped reads and wrote a header-only BAM from any input, and `simplex-metrics`/`duplex-metrics` emitted the same all-zero tables either way. A reader that dropped the entire stream still matched the oracle for all five. Give them fixtures they emit records from. `duplex` and `duplex-metrics` get both strands of one molecule (`RX` plus strand-suffixed `MI`), and `simplex`/`simplex-metrics` get single-strand molecules — `simplex-metrics` rejects a base UMI seen on both strands outright, so it cannot share the duplex shape. `codec` reuses the CODEC command's own pair builder rather than a second copy of the MI/MC/overlap invariants, and needs the same `--min-reads`/`--min-duplex-length` its own tests pass. Rather than trust that this stays true, put it under test. Every command now declares `output_depends_on_input`, and a new matrix drives each fixture through its command twice — once populated, once drained — and fails if the two agree. That converts an enumerated caveat in a doc comment into an assertion: a future fixture that stops producing records breaks at the cause instead of quietly hollowing out the other matrices. `zipper` declares the exemption, verified to be load-bearing: it emits one template per uBAM template and the uBAM arrives on `-u`, so an empty `-i` legitimately yields the same templates unmapped. Verified by fault injection in both directions — restoring the old shapes fails the new test for exactly those five commands and no others, and declaring `zipper` Required fails it for `zipper` alone.
3d78ecb to
ed4fde7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
Stacked on #644 — please merge that one first. The base is
nh/feat-accept-sam-input, so the diff here is only this change.The problem
#644 added
test_input_source_matrix.rs, which holds every command to two contracts: uncompressed SAM must be accepted anywhere BAM is, and-i -must be accepted by anything that streams. Both are checked by running the command two ways and comparing what it wrote against a named-path oracle.That comparison is only evidence when the output depends on the records that went in. For five commands it did not:
simplex,duplexandcodecwere handed ungrouped reads and wrote a header-only BAM from any input, including no input at all.simplex-metricsandduplex-metricsemitted identical all-zero tables either way, because the fixture carried nothing for them to count.So for those five, a reader that silently dropped the entire stream still matched the oracle and the matrices passed. This was found by fault-injecting a header-only BAM while reviewing #644 — it predates that PR's stdin work and affected the SAM axis identically.
What this does
Fixtures they actually emit records from.
duplexandduplex-metricsget both strands of one molecule, taggedRXplus a strand-suffixedMI.simplexandsimplex-metricsget single-strand molecules instead —simplex-metricsrejects a base UMI seen on both strands outright ("received duplex-UMI data … run duplex-metrics"), so it cannot share the duplex shape.codecreuses the CODEC command's own pair builder rather than a second copy of the MI/MC/overlap invariants, and needs the same--min-reads/--min-duplex-lengthits own tests pass. Record counts for the five go 0/0/0 and all-zero → 4, 2, 1, 28 and 29.An assertion instead of a caveat. #644 documented this limitation in a comment listing the affected commands, which is exactly the kind of thing that goes stale. Every command now declares a third contract axis,
output_depends_on_input, andevery_command_output_depends_on_its_inputdrives each fixture through its command twice — once populated, once drained of records — and fails if the two agree. A future fixture that stops producing records for a command breaks there, at the cause, rather than quietly hollowing out the other two matrices. The comment is now a pointer to that test.zipperis the one declared exemption: it emits one output template per uBAM template and the uBAM arrives on-u, so an empty-ilegitimately still yields every template, unmapped. Refusing an empty input outright also counts as depending on it, which is howextractpasses.Verification
Fault-injected in both directions rather than trusting a green run:
zipperRequiredfails it forzipperalone, so the exemption is load-bearing rather than an escape hatch.cargo ci-test6023 passed / 0 failed;ci-fmt,ci-lint, and both--no-default-features --all-targetsand--all-features --all-targetschecks clean. The cross-module reference to the CODEC pair builder follows the existing precedent intest_streaming_input.rsand was checked against the no-default-features build, where the self dev-dependency keeps those modules compiled.Summary by CodeRabbit