test(codec): make the CODEC tests that cannot fail fail - #774
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour. WalkthroughThe PR removes a test-only downsampling wrapper and expands CODEC tests to exercise production rejection, consensus transformation, downsampling, statistics, and rejected-read handling paths. ChangesCODEC production-path tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change strengthens CODEC test coverage and removes an obsolete test-only wrapper; no actionable merge-blocking risk remains at the current head after normal checks and review. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #774 +/- ##
==========================================
- Coverage 94.30% 94.28% -0.02%
==========================================
Files 186 186
Lines 112508 112550 +42
==========================================
+ Hits 106098 106116 +18
- Misses 6410 6434 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
54f5f10 to
4884585
Compare
ab217bf to
f7883c3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
4884585 to
477dbbe
Compare
Ten CODEC tests either asserted something the code cannot violate, or were satisfied before the code under test ran. Each now asserts what its name claims, and each new assertion was verified by removing the behavior it covers and observing the failure. Two needed a fixture change rather than a stronger assertion: - `test_not_emit_consensus_chimeric_pair` built its pair at starts 100 and 135 with 30M each, so the reads never overlapped and the family was rejected for insufficient overlap in phase 4 -- long before the cross-chromosome check in phase 2 was consulted. The pair now overlaps, a control asserts it consenses while on one chromosome, and the rejection reason is asserted. - `test_clip_overlap_failed_counted_and_labeled` hand-called `reject_records_count`, so its guard against the mislabeling it is named for could not fire. It now drives the production site end to end. `test_check_overlap_phase_indel_mismatch` discarded its result under a comment claiming the outcome was indeterminate. It is deterministic; it is asserted, with a control that attributes it to the indel. The rest: `codec_downsampling_retains_lowest_hashing_pairs` and `test_downsample_pairs_no_limit` both ran against a `#[cfg(test)]` `downsample_pairs` wrapper rather than production -- the first's R2 claim was a property of the wrapper, the second returned before any production code. Both now go through the production cap; the wrapper and `test_downsample_pairs`, which only exercised it, are removed. `test_clear_rejected_reads` cleared an already-empty buffer, `test_codec_statistics_tracking` fed no records and asserted `Default`, `test_mask_end_qualities` asserted an upper bound where the code assigns an exact value and never looked at the interior, `test_build_clipped_info_clip_from_start_reverse` asserted `> 100` for an exact 103, and `test_reverse_complement_ss_depths_errors_reversed` used the palindrome `ACGT`, so it passed for a no-op. Two siblings found in the same sweep are fixed alongside: `test_build_clipped_info_zero_clip_preserves_all` asserted the CIGAR had one op rather than which op, and `test_pad_consensus_shorter_target` asserted a length where the claim is "returned unchanged". No production code changes; the only non-test edit removes the unused `#[cfg(test)]` wrapper and the doc references to it. Closes #771.
f7883c3 to
06a81c6
Compare
This branch was successfully deployed
1 participant
Closes #771.
Ten CODEC tests asserted something the code cannot violate, or were satisfied before the code under test ran. Each now asserts what its name claims, and every new assertion was verified by removing the behavior it covers and observing the failure. The perturbation table is below; nothing here rests on inspection alone.
Base branch
Based on #770 (
769/nhomer/assert-codec-consensus-bases), which is itself based on #768. Several of these tests need a family that actually consenses —test_not_emit_consensus_chimeric_pair's control assertion,codec_uncapped_family_contributes_every_read's depth tags — which is only true once #768 stores the fixture's reverse read in reference orientation. This cannot stand alone.The two confirmed defects
test_not_emit_consensus_chimeric_pairneeded a fixture, not an assertion. It built its pair at starts100/135with30Meach, so R1 spanned 100-129 and R2 spanned 135-164 and the two never overlapped.duplex_lengthcame out negative and phase 4 rejected the family asInsufficientOverlap— long after phase 2 had already dropped it for the reason the test is named for. Deleting bothset_ref_idcalls left it green.The pair now overlaps (R1 at 1, R2 at 11, both
30M), a control asserts that the same family does consense while both reads are on one chromosome, and the rejection reason is asserted rather than only the absence of output.Worth knowing for anyone perturbing this: the cross-chromosome rejection is doubly guarded, and removing either guard alone is undetectable.
is_primary_fr_pair_rawcompares the two records'ref_ids, and theis_fr_pair_rawit then calls on the reverse record compares that record'sref_idagainst itsmate_ref_id. Removing only the first leaves the test passing; removing both makes the family emit a consensus and the test fail.test_check_overlap_phase_indel_mismatchdiscarded its result under// Result depends on whether the boundaries land in indels - just verify it runs. It does not depend: over the overlap 100-129, R1's30Mgives query offsets 1 and 30 while R2's15M5D15Mgives 1 and 25, so the phases are0and5and the answer is deterministicallyfalse. It is asserted, the comment is replaced with that arithmetic, and a30Mcontrol pins that thefalseis attributable to the deletion rather than to a lookup that returnedNone— which also yieldsfalse.Per test
test_not_emit_consensus_chimeric_pairNotPrimaryFrPair: 2and no other reasontest_check_overlap_phase_indel_mismatchfalse, plus a30Mcontrol that istruetest_clip_overlap_failed_counted_and_labeledClipOverlapFailed, not the indel reasonconsensus_length < ss.bases.len()branch end to endcodec_downsampling_retains_lowest_hashing_pairskeep_indices_for_infos; R2's list is reversed, so the same three names come back at different positionscodec_uncapped_family_contributes_every_read(wastest_downsample_pairs_no_limit)aD/bDof 10 for a ten-template family, against a capped control of 3test_clear_rejected_readstest_codec_statistics_trackingtest_mask_end_qualities--outer-bases-qualmasks the ends and only the endstest_build_clipped_info_clip_from_start_reverseadjusted_pos == 103test_reverse_complement_ss_depths_errors_reversedAACG→CGTTTwo of these needed more than a stronger assertion.
test_clip_overlap_failed_counted_and_labeledwas not reaching its subject at all. Its only interaction with the caller was a directreject_records_count(2, ClipOverlapFailed), so its stated regression guard — that the reject had not leaked intoIndelErrorBetweenStrands— could not fire: nothing in the test could insert the other reason.Its own doc comment claimed the production branch was unreachable from a synthetic fixture. That turns out to be wrong, and the mechanism is worth recording. The branch fires when the consensus is shorter than a single strand, and the overlap clipper measures the mate overhang in soft-clip unclipped coordinates. A query-only operation is therefore invisible to it: R2 at
20I30Mcarries twenty query bases the clipper does not see and does not clip, the consensus length comes out at 40 in reference space, and R2's own strand consensus is 50. Putting the insertion ahead of the overlap keepscheck_overlap_phase_rawsatisfied, so the earlierIndelErrorBetweenStrandsgate does not fire first. The CIGAR is degenerate rather than aligner-shaped, and deliberately so — a soft-clip-built fixture is normalised by the clipper to exactlyconsensus_length == ss.bases.len(), one short of the branch. That degeneracy is what the reject exists for.codec_downsampling_retains_lowest_hashing_pairs' R2 claim was a property of the test harness. The#[cfg(test)]downsample_pairswrapper derived one index vector from R1 and mapped it over R2, so "R2 must keep the same templates as R1" held by construction. Both strands now go throughkeep_indices_for_infos— the functioncap_infos_to_lowest_ranking, and thereforeconsensus_reads_raw, calls — each capped from its own list. R2's list is built in the reverse of R1's, so the R2 arm now carries the "lowest-hashing, not first-N" half of the claim: an order-dependent rule would keep{q0, q1, q2}from R1 and{q9, q8, q7}from R2, and the two arms would disagree.One qualification on the issue's wording there. The old assertion did not state the opposite of production for this fixture — production caps each strand on its own ranks, and mates share a read name and therefore a rank, so two independent caps over identically-named lists do land on the same templates. What was wrong was presenting that as the rule: the rule is per-strand independence, and
codec_caps_each_strand_from_its_own_filtered_setis the test where the two strands deliberately retain different templates. The old assertion was unfalsifiable and its comment was misleading; it was not asserting a falsehood.Deletions
downsample_pairs(the#[cfg(test)]wrapper) andtest_downsample_pairsare removed. Once the two tests above go through production, the only remaining caller of the wrapper wastest_downsample_pairs, whose assertion (ds_r1s.len() == 2) is strictly weaker thancodec_downsampling_retains_lowest_hashing_pairs' on the same rule — and the cap's boundary behaviour is separately pinned bytest_select_lowest_ranking'sat_cap_keeps_everything/below_cap_keeps_everythingcases. No coverage is lost.This is the only non-test edit, along with the two doc comments that referred to the wrapper.
Non-vacuity
Every strengthened assertion was verified against a perturbation of the behavior it pins. Each perturbation was also run against the pre-change tree, so the last column is measured rather than asserted.
not_emit_consensus_chimeric_pairis_primary_fr_pair_rawandis_fr_pair_raw)not_emit_consensus_chimeric_pairis_primary_fr_pair_raw's same-reference checktruecheck_overlap_phase_indel_mismatchfalsecheck_overlap_phase_indel_mismatch(control)ClipOverlapFailedsite relabeledIndelErrorBetweenStrands— the pre-fix behaviorclip_overlap_failed_counted_and_labeledcodec_uncapped_family_contributes_every_readcodec_downsampling_retains_lowest_hashing_pairsclear_rejected_readsbody emptiedclear_rejected_readsclear_rejected_readsdelegates toclear()clear_rejected_readstotal_input_readsnever accumulatedcodec_statistics_trackingconsensus_reads_generatednever incrementedcodec_statistics_trackingreads_filterednever accumulatedcodec_statistics_trackingmask_end_qualitiesmask_end_qualitiesbuild_clipped_info_clip_from_start_reversereverse_complement_ssreverses without complementingreverse_complement_ss_depths_errors_reversedreverse_complement_sscomplements without reversingreverse_complement_ss_depths_errors_reversedreverse_complement_ssleaves bases untouchedreverse_complement_ss_depths_errors_reversedbuild_clipped_info_zero_clip_preserves_allpad_consensus_shorter_targetSixteen of these were undetected by the test that owns them.
Two corrections to the issue while doing this.
ACGTdoes separate reverse-only from complement-only — both giveTGCA, which the old assertion rejected. What it could not catch is the function being a no-op, and that is the case that was untested. Relatedly, "no non-palindromic test anywhere in the crate" was true of direct tests only: the crate-wide blast radius of a no-opreverse_complement_ssis 13 tests, 12 of which come from #768's and #770's end-to-end base assertions. Every other unconfirmed claim in the issue held exactly as written.Sibling sweep
Two more of the same shape, adjacent to tests already being fixed, folded in here rather than filed:
test_build_clipped_info_zero_clip_preserves_allasserted the CIGAR had one op, not which op. It now compares against4M.test_pad_consensus_shorter_targetasserted a length where the claim in its own comment is "returned unchanged". It now compares bases, quals, depths and errors.Still weaker than the contract, not changed here
Listed rather than fixed, to keep the diff on the issue's set:
test_to_source_read_*_over_clip(four assertions across two tests) checkbases.is_empty()/quals.is_empty()after over-clipping. That is the right claim, but it is also what an unconditionalVec::new()returns; neither test pins that a partial clip keeps the right remainder.test_codec_caller_creationandtest_vanilla_to_single_strand_negative_strandecho their own constructor arguments.test_consensus_reads_typed_disagreement_count'sassert!(err.is_duplex_disagreement())is implied by thematches!on the line above it.Verification
cargo ci-fmt,cargo ci-lint,cargo ci-test(7494 passed),cargo ci-doctest— all green.Local review
CodeRabbit's automatic reviews are paused on this repo, so a local sweep was run against
.coderabbit.yaml's test instruction — "flag assertions weaker than the contract". It caught this PR committing the same defect it fixes:test_reverse_complement_ss_depths_errors_reversedhad gained twoassert_ne!s (!= "GCAA",!= "TTGC") that are implied by theassert_eq!(rc.bases, b"CGTT")above them and cannot fail. They are gone; the reasoning they encoded lives in the doc comment. The sweep also flagged thattest_downsample_pairs_no_limitno longer named anything that exists once the wrapper was removed — hence the rename — and produced the two sibling fixes above, which are the "check the file for its siblings and list them all" clause applied tobuild_clipped_infoandpad_consensus.Risk: command output changes: none;
unsafechanges: none, and noCLAUDE.mdallowlist update applies; memory bounds, queue capacities, and thread/backpressure policies: none.downsample_pairswrapper and its test.