Skip to content

test: gate command output against its @HD sort-order claim - #580

Merged
nh13 merged 1 commit into
mainfrom
nh/test-verify-sort-output
Jul 18, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/test-verify-sort-output

Conversation

@nh13

@nh13 nh13 commented Jul 12, 2026 •

Copy link
Copy Markdown
Member

Closes task-list T2. A command's @HD SO/GO/SS tags are a promise about output order that nothing verified — the same class of gap that hid the simulate sort bug, one layer up.

Audit

Commands writing a sorted claim: group ("output is always template-coordinate"), dedup, and merge (default --order template-coordinate). sort (via --verify) and simulate (since #576's hermetic rewrite) were already covered; simplex/duplex/codec emit SO:unsorted (nothing to gate); extract emits SO:unsorted.

What

A shared assert_bam_sorted(bam, order, key_types) helper runs fgumi's own fgumi sort --verify on a command's output. Applied to:

  • group — asserts the output verifies as template-coordinate (it does).
  • dedup — same assertion, which surfaced a real test-data defect: create_sorted_bam never actually sorted — it wrote records verbatim under a template-coordinate header, so dedup (which trusts the header) emitted mislabelled-but-unsorted output. Fixed the helper to genuinely sort via fgumi sort; dedup's output then verifies, confirming dedup preserves the order (no dedup bug).
  • merge — new test (merge had zero integration coverage): merge two genuinely template-coordinate sorted inputs whose positions interleave, then assert the merged output verifies as template-coordinate sorted.

Verification

cargo ci-lint clean; the group/dedup/merge tests pass (9/9). The assert_bam_sorted helper is now the reusable gate for any future command that claims a sort order.

Summary by CodeRabbit

  • Tests
    • Expanded integration coverage for BAM sorting and output ordering.
    • Added verification that dedup, group, and merge outputs maintain the expected template-coordinate order.
    • Added end-to-end merge scenarios using multiple sorted BAM inputs.
    • Enhanced checks for required molecular identifier tags in generated output.

@nh13
nh13 temporarily deployed to github-actions July 12, 2026 00:49 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The tests add subprocess-based BAM sort verification, create genuinely template-coordinate-sorted fixtures, and validate ordering claims for dedup, group, and merge outputs, including MI key handling.

Changes

BAM output ordering validation

Layer / File(s) Summary
BAM verification helper
tests/integration/helpers/assertions.rs
Adds assert_bam_sorted, which runs fgumi sort --verify with an order and optional key types.
Dedup and group output checks
tests/integration/test_dedup_command.rs, tests/integration/test_group_command.rs
Creates genuinely template-coordinate-sorted dedup inputs and verifies dedup and group outputs retain the claimed ordering.
Merge output-order gate
tests/integration/test_merge_command.rs
Builds paired-end sorted inputs, runs template-coordinate merge on interleaving inputs, and verifies the merged BAM ordering with MI keys.

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

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding test coverage that verifies command output matches its declared BAM sort-order claim.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/test-verify-sort-output

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

@codecov

codecov Bot commented Jul 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.88%. Comparing base (f55ac4b) to head (075fb5e).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
+ Coverage   92.84%   92.88%   +0.03%     
==========================================
  Files         166      166              
  Lines      102064   102064              
==========================================
+ Hits        94765    94804      +39     
+ Misses       7299     7260      -39     

☔ 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.

Commands that write an `@HD SO/GO/SS` sort tag were promising an order nothing
verified. `group` ("output is always template-coordinate"), `dedup`, and `merge`
(default `--order template-coordinate`) all advertise SS:template-coordinate; only
`sort` (via --verify) and `simulate` (since the hermetic rewrite) actually checked
their output. (`simplex`/`duplex`/`codec` emit SO:unsorted, so there is nothing to
gate.)

Add a shared `assert_bam_sorted(bam, order, key_types)` helper that runs fgumi's
own `fgumi sort --verify` on a command's output, and apply it:

- group: assert the output verifies as template-coordinate (it does).
- dedup: assert the same. This surfaced that `create_sorted_bam` never actually
  sorted its records -- it wrote them verbatim under a template-coordinate header,
  so dedup (which trusts the header) emitted mislabelled-but-unsorted output.
  Fixed the helper to genuinely sort its input via `fgumi sort`; dedup's output
  then verifies, confirming dedup preserves the order (no dedup bug).
- merge: new test (merge had zero integration coverage) -- merge two genuinely
  template-coordinate sorted inputs whose positions interleave, then assert the
  merged output verifies as template-coordinate sorted.
@nh13
nh13 force-pushed the nh/test-verify-sort-output branch from 4284e0c to 075fb5e Compare July 16, 2026 20:08
@nh13
nh13 temporarily deployed to github-actions July 16, 2026 20:08 — with GitHub Actions Inactive
@nh13

nh13 commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 18, 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 commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 18, 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/helpers/assertions.rs`:
- Around line 234-245: Retain assert_bam_sorted as a diagnostic, but add an
independent identity-complete oracle and use it alongside existing checks: in
tests/integration/helpers/assertions.rs:234-245, provide test-only expected
record/key validation; in tests/integration/test_dedup_command.rs:137-139,
verify every output name, flag, position, and MI/duplicate state; in
tests/integration/test_group_command.rs:51-53, verify output read identities and
assigned MI values; and in tests/integration/test_merge_command.rs:616-617,
verify both records for each a/b/c/d template appear in the exact expected
order.
🪄 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: bb170e88-cc43-4601-b1d4-47c06d49cea8

📥 Commits

Reviewing files that changed from the base of the PR and between f55ac4b and 075fb5e.

📒 Files selected for processing (4)
  • tests/integration/helpers/assertions.rs
  • tests/integration/test_dedup_command.rs
  • tests/integration/test_group_command.rs
  • tests/integration/test_merge_command.rs

Comment thread tests/integration/helpers/assertions.rs

This branch was previously deployed

1 inactive deployment
github-actions — 075fb5e3 Deployed Jul 16, 2026 by nh13 via coverage #2610
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