Skip to content

test(clip): port missing fgbio SamRecordClipper and ClipBam tests - #1018

Merged
nh13 merged 3 commits into
mainfrom
nh/clip-port-fgbio-tests
Oct 5, 2026
Merged

nh13 merged 3 commits into
mainfrom
nh/clip-port-fgbio-tests

Conversation

@nh13

@nh13 nh13 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

Follows #1015 (merged).

#1015's bugs got through because none of fgbio's SamRecordClipperTest / ClipBamTest cases cover overlap clipping with a far-end soft clip, soft-with-mask overlap clipping, or empty SEQ. That prompted an audit of how completely those two suites were ported. Every fgbio case was re-run against fgumi with fgbio's exact inputs and expectations, and fgumi met all of them, so this PR is test coverage plus cleanup with no change in output.

fgbio suite cases already equivalent weaker in fgumi missing
SamRecordClipperTest 82 44 8 30
ClipBamTest 24 3 11 10

Changes

  • Port the weaker/missing cases into fgbio_sam_record_clipper_tests (clipper.rs, 69 tests) and fgbio_clip_bam_tests (clip.rs, 31 tests). Each cites its fgbio source line, with fgbio's expected values copied unchanged. Notable gaps closed:
    • all 12 clipExtendingPastMateEnds cases;
    • hard-mode overlap clipping with soft clips and deletions;
    • clip{Start,End}OfRead and clip{5,3}PrimeEndOfRead;
    • ClipBam's end-to-end metric values, mate info and NM/UQ/MD;
    • --upgrade-clipping across mode pairs.
  • Fixture-only deviations are documented on the affected tests:
    • ClipBamTest.scala:182's SEQ is resized to match its hard-clipped CIGAR, because fgbio's fixture is malformed.
    • numBasesExtendingPastMate is exercised through the MC-based num_bases_extending_past_mate_raw.
  • Stricter tag assertions: 28 existing clipper tests asserted tag values inside if let Some(Value::...), so a missing tag silently passed. They now panic instead.
  • Cleanup:

Not addressed

  • clip_start_of_alignment / clip_end_of_alignment still leave an empty-SEQ (*) record's alignment unclipped, whereas fgbio clips the CIGAR. Since fix(clip): upgrade soft clips only at the mate-facing end when clipping overlaps #1015 the read-end helpers at least upgrade the existing soft clip at that end. No fgbio test reaches this.
  • Some pre-existing fgumi overlap tests still assert only > 0 (_with_multiple_insertions, _insertion_at_overlap_boundary, _complex_cigar, _soft_with_mask). The ported fgbio table now pins the same behavior exactly.

Mutation check

Corrupting one expected value in each of three new tests fails exactly those three.

@nh13
nh13 deployed to github-actions October 4, 2026 22:12 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: dd57c585-1dc7-40e7-b893-49b5217e9d3d
📥 Commits

Reviewing files that changed from the base of the PR and between 7b02a4c and 5c14575.

📒 Files selected for processing (2)
  • crates/fgumi-sam/src/clipper.rs
  • src/lib/commands/clip.rs

Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.


⚠️ A high-level summary could not be generated for this review. CodeRabbit will regenerate it on the next update, or you can request a refresh with @coderabbitai summary.

Walkthrough

The pull request reuses a shared helper to count existing raw end clips and adds fgbio-derived tests for raw and command-level clipping. Tests also require clipping-related tags to exist with expected types and cover clipping outputs, mate fields, regenerated tags, metrics, and unmapped reads.

Changes

Read clipping

Layer / File(s) Summary
Raw clipper helpers and coverage
crates/fgumi-sam/src/clipper.rs
Raw start and end clipping use a shared helper to count existing clips. Test adapters and fgbio-derived cases cover read-end, alignment, overlap, and past-mate clipping. Strand-normalization tests now include SoftWithMask.
Clipping attribute assertions
crates/fgumi-sam/src/clipper.rs
Clipping tests now fail when expected tags are missing or have the wrong type, rather than skipping value checks.
Pair and fragment clipping
src/lib/commands/clip.rs
Command tests cover overlap and fixed clipping, clipping metrics, paired-read output and mate fields, regenerated tags, and fragment output.
Clipping upgrades and mate extension
src/lib/commands/clip.rs
Command tests cover clipping-mode upgrades, reads extending past their mates, and clipping requests that leave reads unmapped. A comment clarifies the scope of the template-wide upgrade pass.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~30 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 5c145

The clipping changes add tests and clarify a comment; no issue requiring correction before merge was identified.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required conventional-commit format, names the affected clipping scope, and describes the test-porting change with a lowercase imperative description.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@nh13

nh13 commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.20807% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.44%. Comparing base (1b203b1) to head (5c14575).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/fgumi-sam/src/clipper.rs 88.47% 31 Missing ⚠️
src/lib/commands/clip.rs 98.66% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
+ Coverage   96.43%   96.44%   +0.01%     
==========================================
  Files         299      299              
  Lines      151289   152039     +750     
==========================================
+ Hits       145894   146634     +740     
- Misses       5395     5405      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13
nh13 force-pushed the cv_clip_keep_soft_clips branch from 0c2d31f to e29b074 Compare October 4, 2026 23:54
@nh13

nh13 commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 force-pushed the nh/clip-port-fgbio-tests branch from 32bafec to 058535a Compare October 4, 2026 23:55
@nh13
nh13 force-pushed the cv_clip_keep_soft_clips branch from e29b074 to c0c9aa3 Compare October 5, 2026 01:05
@nh13
nh13 force-pushed the nh/clip-port-fgbio-tests branch from 058535a to 6f4d522 Compare October 5, 2026 01:06
@nh13
nh13 force-pushed the cv_clip_keep_soft_clips branch 2 times, most recently from ce0dacc to ebeba98 Compare October 5, 2026 02:32
@nh13
nh13 force-pushed the nh/clip-port-fgbio-tests branch from 6f4d522 to 54ebee7 Compare October 5, 2026 02:34
@nh13
nh13 changed the base branch from cv_clip_keep_soft_clips to main October 5, 2026 03:03
nh13 added 3 commits October 4, 2026 21:37
…mments

clip_start_of_read_raw and clip_end_of_read_raw now use the
clipping_at_end_raw helper that overlap clipping already uses. Comments
that pointed at the removed typed SamRecordClipper now cite fgbio, and
the clip_template_records comment no longer claims the per-read helpers
never upgrade clipping (they upgrade the end they clip, as fgbio does).
The strand-normalization overlap tests now also run in soft-with-mask
mode.
…as missing

An audit of fgbio's SamRecordClipperTest and ClipBamTest (fgbio
e51a661) against fgumi found 30 + 10 cases with no fgumi counterpart and
8 + 11 whose fgumi version dropped clipping modes or assertions (for
example checking only the CIGAR, not bases, qualities, return values,
mate info or metrics). This ports each of those cases with fgbio's
inputs and expected values, in dedicated fgbio_sam_record_clipper_tests
and fgbio_clip_bam_tests modules; every test cites its fgbio source
line. fgumi already met every expectation, so no production code changes.

Fixture-only deviations are documented on the affected tests: ClipBamTest
L182's SEQ is resized to match its hard-clipped CIGAR (fgbio's fixture
is malformed), and numBasesExtendingPastMate is exercised through the
MC-based num_bases_extending_past_mate_raw.
28 clipper tests checked per-base tag values inside `if let
Some(Value::...)`, so a missing tag or a changed value type skipped the
assertion and the test passed. They now use let-else and panic.
@nh13
nh13 force-pushed the nh/clip-port-fgbio-tests branch from 54ebee7 to 5c14575 Compare October 5, 2026 04:39
@nh13

nh13 commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 deployed to github-actions October 5, 2026 15:39 — with GitHub Actions Active
@nh13
nh13 added this pull request to the merge queue Oct 5, 2026
Merged via the queue into main with commit 8083173 Oct 5, 2026
38 of 39 checks passed
@nh13
nh13 deleted the nh/clip-port-fgbio-tests branch October 5, 2026 16:00
@nh13 nh13 mentioned this pull request Oct 5, 2026

This branch was successfully deployed

1 active deployment
github-actions — 5c145753 Deployed Oct 5, 2026 by nh13 via coverage #4817
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