refactor(compare,fastq) + test: in-process Command::execute for Phase 3 (#352) by nh13 · Pull Request #355 · fulcrumgenomics/fgumi

nh13 · 2026-05-18T19:54:52Z

Summary

Phase 3 of #352, stacked on #354. Contains the two production refactors the issue called out as prerequisite, plus the test conversions they unlock.

Production refactors

compare: replaces 7 std::process::exit(1) sites in src/lib/commands/compare/{bams,metrics}.rs with Err(CompareMismatch(...)). CompareMismatch is a new public, downcastable anyhow-compatible error variant. src/main.rs downcasts on it and maps to exit(1) so the CLI exit-code contract is preserved byte-for-byte.
fastq: adds optional --output/-o flag. When provided, FASTQ writes to the file; when omitted, the prior stdout() behavior is preserved (still the default for fgumi fastq -i in.bam | bwa mem ...). The conversion loop is factored into a generic Fastq::run_with_writer<W: Write> helper.

Test conversions

test_compare_bams.rs: 8 of 17 tests converted to in-process (those that only check IDENTICAL/DIFFER — fully covered by the Result type). The other 9 tests stay on the subprocess path because they assert on specific diagnostic substrings (EQUIVALENT, Order/name mismatches: N, tags in different order, Missing MI in BAM[12]: N, etc.) that aren't observable in-process. Both helpers (run_compare for subprocess, run_compare_in_process for in-process) coexist; tests pick the one that matches their assertion shape.
test_e2e_regression.rs: the compare_bams and assert_bams_identical helpers move in-process; the test_different_seeds_produce_different_output test now asserts !compare_bams_in_process(...) instead of output.status.code() == Some(1). The file's other subprocess helpers (fgumi, fgumi_ok) stay untouched — they're Phase 4 scope.
test_fastq_command.rs: all 7 tests now pass -o <tempfile> to write FASTQ directly to a temp file; the fs::write(&output_fq, &output.stdout) middle step is gone.
test_zipper_command.rs: 3 of 5 tests converted to in-process. Two stay subprocess:
- test_zipper_bam_stdin_input — exercises the std::io::stdin() pipe path inside zipper, which only exists at the process boundary.
- test_zipper_bam_mapped_input — asserts on a log::warn!("BAM input detected") warning captured from stderr; capturing that in-process would require global-logger plumbing in the test harness.

Phasing

Phase 1 (test: in-process Command::execute for Phase 1 commands (#352) #353): extract, group, dedup, correct, filter, clip
Phase 2 (test: in-process Command::execute for Phase 2 commands (#352) #354): simplex, simplex-metrics, duplex, duplex-metrics, codec, review
Phase 3 (this PR): compare refactor + fastq refactor + their tests + non-stdin zipper tests
Phase 4: sort, downsample, remaining e2e

Commits

refactor(compare)!: return CompareMismatch instead of process::exit
test(compare):     invoke compare bams Command::execute in-process
feat(fastq):       add --output/-o flag for file output
test(fastq):       invoke fastq Command::execute in-process
test(zipper):      invoke zipper Command::execute in-process for non-stdin tests

Test plan

cargo nextest run --features compare,simulate --test integration — 193/193 passed, 23 skipped
cargo build (default features) — clean
cargo build --features compare — clean
cargo ci-fmt clean
cargo ci-lint clean (default features)
Codecov reports coverage gains on compare/fastq execute() bodies
Verify on a downstream consumer that the breaking compare refactor (now returning Err(CompareMismatch) instead of process::exit(1) in-process) does not affect any caller — only main.rs and the integration tests use it, both updated here

codecov · 2026-05-18T19:57:43Z

Codecov Report

❌ Patch coverage is 69.23077% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.91%. Comparing base (c3b6dc6) to head (b964c20).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/commands/compare/metrics.rs	0.00%	11 Missing ⚠️
src/lib/commands/fastq.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   90.93%   90.91%   -0.03%     
==========================================
  Files          78       78              
  Lines       51046    51074      +28     
==========================================
+ Hits        46421    46432      +11     
- Misses       4625     4642      +17

☔ View full report in Codecov by Sentry.
📢 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 · 2026-05-20T02:06:49Z

@coderabbitai resume

nh13 · 2026-05-20T02:06:50Z

@coderabbitai review

coderabbitai · 2026-05-20T02:06:54Z

📝 Walkthrough

Walkthrough

This PR refactors compare subcommands to return errors instead of calling std::process::exit(1), introduces a CompareMismatch error type, adds Fastq output file support via --output/-o, and migrates integration tests to in-process command execution. The compare infrastructure change enables callers to distinguish mismatch failures from I/O errors. Fastq gains a run_with_writer helper to support both file and stdout output. All integration tests now use library APIs directly instead of spawning subprocesses, simplifying assertions and test maintenance.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly identifies the main refactoring work (compare, fastq) and test conversions, directly matching the changeset scope.
Description check	✅ Passed	Description comprehensively covers all changes: production refactors (compare, fastq), test conversions (4 test files), phasing context, and test results.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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/issue-352-phase-3

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai · 2026-05-20T02:06:55Z

✅ Actions performed

Review triggered.

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 · 2026-05-20T02:06:57Z

✅ Actions performed

Reviews resumed.

nh13 · 2026-05-20T02:14:57Z

@coderabbitai resume

nh13 · 2026-05-20T02:14:57Z

@coderabbitai review

coderabbitai · 2026-05-20T02:15:02Z

✅ Actions performed

Reviews resumed.

coderabbitai · 2026-05-20T02:15:03Z

✅ Actions performed

Review triggered.

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 · 2026-05-20T02:26:05Z

@coderabbitai resume

nh13 · 2026-05-20T02:26:06Z

@coderabbitai review

coderabbitai · 2026-05-20T02:26:10Z

✅ Actions performed

Reviews resumed.

coderabbitai · 2026-05-20T02:26:12Z

✅ Actions performed

Review triggered.

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

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 `@src/lib/commands/fastq.rs`:
- Around line 200-203: The code currently calls File::create(path) (inside the
match on self.output) which truncates the target before input is opened in
run_with_writer, risking clobbering when --output equals --input; update the
logic to detect and prevent that by checking the output path against the input
path (e.g., compare self.output path to self.input path) before calling
File::create, and return an error if they are the same, or alternatively open
the input first or write to a temporary file and atomically rename into place;
change the branch that uses File::create + BufWriter::with_capacity to perform
this safety check (refer to self.output, self.input, File::create, and
run_with_writer).

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 56a9a693-324c-40eb-919d-f2443e61d790

📥 Commits

Reviewing files that changed from the base of the PR and between 29dec92 and 040eef4.

📒 Files selected for processing (9)

src/lib/commands/compare/bams.rs
src/lib/commands/compare/metrics.rs
src/lib/commands/compare/mod.rs
src/lib/commands/fastq.rs
src/main.rs
tests/integration/test_compare_bams.rs
tests/integration/test_e2e_regression.rs
tests/integration/test_fastq_command.rs
tests/integration/test_zipper_command.rs

test_group_determinism.rs is intentionally kept on the subprocess path so each run picks up a different per-process AHashMap hasher seed; that variance is the property the test was written to exercise.

…cess

The compare subcommand previously called std::process::exit(1) on any mismatch or validation failure. This kept in-process callers (e.g. integration tests under cargo llvm-cov nextest) from observing the result. Introduce a CompareMismatch error variant; the CLI maps it back to exit code 1 in main.rs, while in-process callers can downcast. No change to CLI behavior or exit codes.

Convert the 8 test_compare_bams.rs tests that assert only on IDENTICAL/DIFFER and the run_compare_bams_mode / assert_bams_identical helpers in test_e2e_regression.rs to call CompareBams::execute() directly, downcasting on CompareMismatch to distinguish match from mismatch. The remaining 9 test_compare_bams.rs tests stay on the subprocess path because they assert on specific report substrings (EQUIVALENT, Order/name mismatches: N, tags in different order, Missing MI in BAM[12]: N, etc.) that aren't captured in-process.

The fastq command previously wrote unconditionally to stdout, which made in-process tests need an explicit stdout capture. Add an optional --output/-o flag; when present, the FASTQ stream is written to the named file. When omitted, the existing stdout behavior is preserved (stdout remains the default to keep `fgumi fastq -i in.bam | bwa mem` working without flag changes). Extract the conversion loop into a generic Fastq::run_with_writer helper so execute() dispatches the writer type at one place instead of duplicating the loop.

Pass the new --output flag (added in the prior commit) to write FASTQ output directly to a temp file, then parse the file as before. No subprocess stdout capture needed.

… tests Converts the 3 fully-in-process-eligible subprocess invocations to in-process Command::execute() calls. Two tests intentionally stay on the subprocess path: - test_zipper_bam_stdin_input exercises the std::io::stdin() code path inside zipper, which only exists at the process boundary. - test_zipper_bam_mapped_input asserts on a log::warn! "BAM input detected" warning sent to stderr; observing that in-process would require global-logger plumbing in the test harness.

nh13 temporarily deployed to github-actions May 18, 2026 19:54 — with GitHub Actions Inactive

nh13 mentioned this pull request May 18, 2026

test: in-process Command::execute for Phase 4 commands (#352) #356

Merged

3 of 4 tasks

nh13 force-pushed the nh/issue-352-phase-2 branch from 51d4d49 to 29dec92 Compare May 18, 2026 21:42

nh13 force-pushed the nh/issue-352-phase-3 branch from bedf332 to 040eef4 Compare May 18, 2026 21:42

nh13 temporarily deployed to github-actions May 18, 2026 21:42 — with GitHub Actions Inactive

coderabbitai Bot requested changes May 20, 2026

View reviewed changes

Comment thread src/lib/commands/fastq.rs

nh13 added 9 commits May 21, 2026 15:47

test(clip): invoke clip Command::execute in-process

65985d4

test(dedup): invoke dedup Command::execute in-process

0cf1ecf

test(correct): invoke correct Command::execute in-process

eaea8de

test(group): invoke group Command::execute in-process

88d0b9d

test_group_determinism.rs is intentionally kept on the subprocess path so each run picks up a different per-process AHashMap hasher seed; that variance is the property the test was written to exercise.

test(filter): invoke filter Command::execute in-process

d3c90a8

test(extract): invoke extract Command::execute in-process

04adadd

test(simplex): invoke simplex Command::execute in-process

532b063

test(simplex-metrics): invoke simplex-metrics Command::execute in-pro…

6f8e9f5

…cess

test(duplex): invoke duplex Command::execute in-process

e709527

nh13 added 3 commits May 21, 2026 15:53

test(duplex-metrics): invoke duplex-metrics Command::execute in-process

17675ee

test(codec): invoke codec Command::execute in-process

e6d29c2

test(review): invoke review Command::execute in-process

bb40c3b

nh13 force-pushed the nh/issue-352-phase-2 branch from 29dec92 to bb40c3b Compare May 21, 2026 23:03

nh13 added 5 commits May 21, 2026 16:03

test(fastq): invoke fastq Command::execute in-process

8e7aba1

Pass the new --output flag (added in the prior commit) to write FASTQ output directly to a temp file, then parse the file as before. No subprocess stdout capture needed.

nh13 force-pushed the nh/issue-352-phase-3 branch from 040eef4 to b964c20 Compare May 21, 2026 23:08

coderabbitai Bot approved these changes May 21, 2026

View reviewed changes

nh13 temporarily deployed to github-actions May 21, 2026 23:08 — with GitHub Actions Inactive

Base automatically changed from nh/issue-352-phase-2 to main May 22, 2026 00:19

nh13 merged commit 5f75140 into main May 22, 2026
8 of 10 checks passed

nh13 deleted the nh/issue-352-phase-3 branch May 22, 2026 00:21

nh13 mentioned this pull request May 22, 2026

test: convert integration tests to in-process Command::execute() calls #352

Closed

Conversation

nh13 commented May 18, 2026

Summary

Production refactors

Test conversions

Phasing

Commits

Test plan

Uh oh!

codecov Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nh13 commented May 20, 2026

Uh oh!

nh13 commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

nh13 commented May 20, 2026

Uh oh!

nh13 commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

nh13 commented May 20, 2026

Uh oh!

nh13 commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented May 18, 2026 •

edited

Loading

coderabbitai Bot commented May 20, 2026 •

edited

Loading