test: close spill/cancellation coverage gaps + rstest-ify reject loop (audit E1/E2/E3) - #543
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughIntegration coverage tests clean failure for truncated BAM input, distinguishes sort spill and in-memory paths while checking output correctness, and parameterizes single-threaded consensus SAM-input checks across codec, simplex, and duplex commands. ChangesIntegration test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-runall #543 +/- ##
==============================================
Coverage ? 94.24%
==============================================
Files ? 111
Lines ? 51055
Branches ? 0
==============================================
Hits ? 48116
Misses ? 2939
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a6b4ecf to
0c8f6bd
Compare
0c8f6bd to
5d988b1
Compare
5d988b1 to
38f4b0f
Compare
38f4b0f to
e20b112
Compare
e20b112 to
f5ed73f
Compare
f5ed73f to
7c209a0
Compare
7c209a0 to
634cbce
Compare
634cbce to
ce1ffa6
Compare
ce1ffa6 to
cbf969e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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_streaming_input.rs`:
- Around line 378-380: Update the argument construction in the streaming input
test to explicitly include the “--threads 1” option alongside the existing
compression setting, ensuring the rejection assertion exercises the stated
single-threaded configuration rather than relying on the default.
🪄 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: ba42972c-d562-4149-8dab-055f785ad299
📒 Files selected for processing (3)
tests/integration/test_runall_parity.rstests/integration/test_sort_correctness.rstests/integration/test_streaming_input.rs
cbf969e to
ee00a3b
Compare
…loop (E1, E2, E3)
Three test-suite gaps from the feat-runall audit (findings/13).
E1 (CG-3) — the sort spill matrix drives `-m` down to force spills but never
asserts one happened; if memory accounting regressed so the engine ignored `-m`,
every matrix case would still pass while silently deleting all spill/merge
coverage. Add `sort_spill_actually_occurs_under_small_memory`: a subprocess run
with `RUST_LOG=info` + `--threads 1` observes the arena `SortMerge` step's log —
a tiny `-m` must take the disk-spill merge path ("Sort merge complete"), a large
`-m` the single-source in-memory fast path ("in-memory fast path complete") —
pinning the memory-bound lever in both directions.
E2 (CG-5) — every runall failure test rejects before the pipeline runs. Add
`runall_group_simplex_fails_cleanly_on_truncated_bam`: feed a BAM truncated
mid-stream to `runall --start-from group --stop-after simplex` and assert a clean
non-zero exit with no `panicked`/`unreachable` in stderr, under a wall-clock
watchdog so a hang fails fast instead of wedging CI.
E3 (TQ-4) — convert the `test_single_threaded_consensus_rejects_sam_with_threads_hint`
sequential-assert loop (codec/simplex/duplex) into an `#[rstest]` case table so a
regression names the command instead of dying on the first bad assert (repo
convention). The two `for (input, output, …)` loops at :612/:700 are left as-is:
they produce both BAM and SAM outputs and cross-compare them, so they are
setup-then-compare, not independent per-scenario asserts.
ee00a3b to
604df09
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
What
Three test-suite gaps from the feat-runall audit (
findings/13).E1 (CG-3) — the sort spill matrix drives
-mdown to force spills but never asserts one happened. If memory accounting regressed so the engine ignored-mand held everything in RAM, every matrix case would still pass (output is still correct) — silently deleting all spill/merge coverage and the memory-bound guarantee. Addedsort_spill_actually_occurs_under_small_memory: a subprocess run withRUST_LOG=info+--threads 1observes the arenaSortMergestep's INFO log — a tiny-mmust take the disk-spill merge path ("Sort merge complete"), a large-mthe single-source in-memory fast path ("in-memory fast path complete") — pinning the lever in both directions.E2 (CG-5) — every runall failure test rejects before the pipeline runs (bad stage pair, missing flag). Added
runall_group_simplex_fails_cleanly_on_truncated_bam: feed a BAM truncated mid-stream torunall --start-from group --stop-after simplexand assert a clean non-zero exit with nopanicked/unreachablein stderr, under a wall-clock watchdog so a hang fails fast instead of wedging CI.E3 (TQ-4) — convert the
test_single_threaded_consensus_rejects_sam_with_threads_hintsequential-assert loop (codec/simplex/duplex) into an#[rstest]case table so a regression names the command instead of dying on the first bad assert (repo convention). The twofor (input, output, …)loops at :612/:700 are intentionally left as-is: they produce both BAM and SAM outputs and cross-compare them, so they are setup-then-compare, not independent per-scenario asserts (rstest-ifying would break the comparison).Target rationale (main vs feat-runall)
test_runall_parity.rsis feat-runall-only;test_sort_correctness.rs/test_streaming_input.rsexist on main but the specific spill-matrix / new-pipeline SAM tests these touch are feat-runall test surface. Feat-runall-only, so this targets the audit stack (nh/audit-3-parity).Tests
All three new/converted tests pass;
cargo ci-test2338 passed / 8 skipped;cargo ci-lintclean.Summary by CodeRabbit
Bug Fixes
runallfails cleanly (no hang, no panics) when data is cut mid-record.Tests
sortcorrectness across both in-memory and merge/spill paths, including expected stderr log behavior.--threadsguidance.