test: in-process Command::execute for Phase 4 commands (#352) by nh13 · Pull Request #356 · fulcrumgenomics/fgumi

nh13 · 2026-05-18T20:22:53Z

Summary

Phase 4 of #352, stacked on #355. Wraps up the migration by converting the remaining single-command files (sort, downsample) and the end-to-end pipeline file.

Converted commands: downsample, sort, plus the in-process e2e pipeline (extract → sort → group → simplex → filter, plus the simulate subcommands grouped-reads and fastq-reads, plus dedup).

Highlights worth flagging

test_sort_correctness.rs and test_sort_write_index.rs drop dead --cell-tag CB and --fast args that prior subprocess tests passed but Sort no longer exposes. Sort::parse_cell_tag hard-codes SamTag::CB for template-coordinate; raw-bytes mode selection is internal. The flags were dead under subprocess too — both files are #[ignore = "requires samtools"] so the silent parse error never surfaced in default CI.
test_sort_fast_write_index and test_sort_standard_write_index are merged into a single test_sort_write_index because --fast no longer exists, so the two tests exercised identical code paths.
test_group_rejects_extract_output_without_sort_step intentionally stays on the subprocess path. It asserts on specific stderr substrings (template-coordinate, SS:template-coordinate, fgumi sort) that are CLI-shaped user-facing diagnostics — keeping it subprocess is correct per the issue's acceptance criteria.
test_simulate_sort.rs is unchanged. Its tests use cargo run --features simulate --release -- simulate ... (not direct fgumi binary invocations) and are #[ignore]-gated; converting them is low value and out of scope.
test_streaming_input.rs, test_bam_pipeline.rs, test_codec_pipeline.rs, test_simplex_pipeline.rs, test_pipeline_concurrency.rs, test_async_reader.rs, test_bgzf_eof.rs, test_error_paths.rs stay subprocess — they're genuine pipeline / pipe-shape tests, exactly the kind of "remaining subprocess invocations" the issue's acceptance criteria allows.

Phasing — complete after this PR

Phase 1 (test: in-process Command::execute for Phase 1 commands (#352) #353): extract, group (excl. determinism), 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 (refactor(compare,fastq) + test: in-process Command::execute for Phase 3 (#352) #355): compare refactor + fastq refactor + non-stdin zipper tests
Phase 4 (this PR): sort, downsample, e2e pipeline

Commits

test(downsample): invoke downsample Command::execute in-process
test(sort):       invoke sort       Command::execute in-process
test(e2e):        invoke pipeline commands in-process via Command::execute

Test plan

cargo nextest run --features compare,simulate --test integration — 193/193 passed, 22 skipped
cargo ci-fmt clean
cargo ci-lint clean
Codecov reports coverage gains on the converted command bodies

codecov · 2026-05-18T20:26:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.88%. Comparing base (5f75140) to head (24a952d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #356      +/-   ##
==========================================
- Coverage   90.97%   90.88%   -0.09%     
==========================================
  Files          78       78              
  Lines       51102    51102              
==========================================
- Hits        46491    46446      -45     
- Misses       4611     4656      +45

☔ 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:07:50Z

@coderabbitai resume

nh13 · 2026-05-20T02:07:51Z

@coderabbitai review

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

Warning

Rate limit exceeded

@nh13 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 1 second before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9ab79b32-0052-4ddf-89a4-e7c84c2ecd6c

📥 Commits

Reviewing files that changed from the base of the PR and between 76fda55 and 24a952d.

📒 Files selected for processing (1)

tests/integration/test_e2e_regression.rs

📝 Walkthrough

Walkthrough

This PR converts four integration test modules from spawning the fgumi binary via subprocess to invoking commands in-process through the library API (Command::try_parse_from(...).execute("test")). Tests were updated to assert Result success/error and to preserve existing output and determinism checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly and specifically describes the main change: converting Phase 4 test commands to in-process execution.
Description check	✅ Passed	Description is well-related to the changeset, detailing the Phase 4 migration of downsample, sort, and e2e pipeline tests to in-process execution.
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 unit tests (beta)

Create PR with unit tests
Commit unit tests in branch nh/issue-352-phase-4

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

coderabbitai · 2026-05-20T02:07:56Z

✅ 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:07:57Z

✅ Actions performed

Reviews resumed.

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

@coderabbitai resume

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

@coderabbitai review

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

✅ 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:26:15Z

✅ Actions performed

Reviews resumed.

nh13 · 2026-05-20T02:38:05Z

@coderabbitai resume

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

@coderabbitai review

coderabbitai · 2026-05-20T02:38:11Z

✅ 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: 3

🤖 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_downsample_command.rs`:
- Around line 317-344: Refactor the two invalid-fraction assertions in
tests/integration/test_downsample_command.rs to use rstest parameterization and
ensure each case writes to a unique output path: use Downsample::try_parse_from
for the same argument sets but supply distinct output files (e.g.,
output_zero.bam for fraction "0.0" and output_gt1.bam for fraction "1.5")
instead of sharing output_bam; keep invoking cmd.execute("test") and asserting
is_err() for each parameter so artifacts from the first run cannot make the
second pass spuriously.

In `@tests/integration/test_sort_correctness.rs`:
- Around line 23-25: The current fgumi_sort_in_process maps OsString to &str
with to_str().unwrap(), which can panic on non-UTF-8; instead stop converting
and pass the OsString values directly to clap by calling Sort::try_parse_from
with the original args (or an iterator over them) so remove the str_args mapping
and use Sort::try_parse_from(&args) / an OsString iterator to avoid the UTF-8
roundtrip and potential panic.

In `@tests/integration/test_sort_write_index.rs`:
- Around line 83-94: Remove the .to_str().unwrap() calls inside the arrays
passed to Sort::try_parse_from and pass the PathBuf variables directly (e.g.,
input_bam, sorted_bam_path and the other temp path variables used in the other
Sort::try_parse_from invocations); Sort::try_parse_from accepts Into<OsString>
so passing PathBuf avoids panics on non-UTF8 paths—update every occurrence where
you currently pass input_bam.to_str().unwrap(),
sorted_bam_path.to_str().unwrap(), etc., to just pass the PathBuf variables.

🪄 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: 04f2b875-9496-4267-91da-2b32a2c8e294

📥 Commits

Reviewing files that changed from the base of the PR and between 040eef4 and 517f938.

📒 Files selected for processing (4)

tests/integration/test_downsample_command.rs
tests/integration/test_e2e_regression.rs
tests/integration/test_sort_correctness.rs
tests/integration/test_sort_write_index.rs

coderabbitai · 2026-05-20T02:41:53Z

✅ Actions performed

Reviews resumed.

Converts the fgumi sort invocations in two test files to in-process Sort::execute() calls. Samtools invocations stay on the subprocess path (samtools is an external tool). - test_sort_correctness.rs: replaces the fgumi(args) subprocess helper with fgumi_sort_in_process(args), wiring both verify_sorted and sort_bam_with_args through Sort::try_parse_from + execute(). - test_sort_write_index.rs: replaces 4 direct fgumi binary invocations with in-process calls; drops the unused fgumi_binary_path() helper. Merges test_sort_fast_write_index and test_sort_standard_write_index into a single test_sort_write_index because the --fast/standard distinction no longer exists on Sort, so the two tests exercised identical code paths. Drops the --cell-tag CB and --fast arguments at several call sites because Sort no longer exposes these as CLI flags (template-coordinate sort hard-codes SamTag::CB via parse_cell_tag; raw-bytes mode is internal). The flags were dead under subprocess too — both files are #[ignore="requires samtools"] so the parse error never surfaced.

nh13 · 2026-05-22T00:26:10Z

@coderabbitai review

coderabbitai · 2026-05-22T00:26:15Z

✅ 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 `@tests/integration/test_e2e_regression.rs`:
- Around line 68-76: The test constructs clap arguments using string coercion
(to_str().unwrap()) when calling GroupedReads::try_parse_from, which can panic
on non-UTF-8 paths; update all such occurrences in
tests/integration/test_e2e_regression.rs (e.g., where
GroupedReads::try_parse_from is called) to use OS-native argument values (use
as_os_str() or convert to OsString) so the args passed into try_parse_from are
native OS strings rather than UTF-8 coerced &strs; apply this replacement
consistently for every to_str().unwrap() used in arg construction in that file.

🪄 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: ed85d69c-24ab-4fa3-a427-f6a6bffafb36

📥 Commits

Reviewing files that changed from the base of the PR and between 517f938 and 76fda55.

📒 Files selected for processing (4)

tests/integration/test_downsample_command.rs
tests/integration/test_e2e_regression.rs
tests/integration/test_sort_correctness.rs
tests/integration/test_sort_write_index.rs

Convert the remaining fgumi-subprocess uses in the e2e regression file to direct \`<Struct>::try_parse_from([...]).execute("test")\` calls. Covers extract, sort, group, simplex, filter, dedup, and the two simulate subcommands (grouped-reads, fastq-reads). The \`fgumi_ok\` helper is removed (no callers remain). The \`fgumi\` helper and \`args!\` macro are retained for \`test_group_rejects_extract_output_without_sort_step\`, which must pin exit-status and stderr content of the real binary.

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

nh13 force-pushed the nh/issue-352-phase-4 branch from 3bf6bd0 to 517f938 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

This was referenced May 18, 2026

test: in-process follow-ups from Opus/Sonnet review of #352 #357

Merged

feat(sort,merge): add --cell-tag flag to override default CB #358

Closed

coderabbitai Bot requested changes May 20, 2026

View reviewed changes

Comment thread tests/integration/test_downsample_command.rs Outdated

Comment thread tests/integration/test_sort_correctness.rs Outdated

Comment thread tests/integration/test_sort_write_index.rs

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

nh13 force-pushed the nh/issue-352-phase-4 branch from 517f938 to 691eead Compare May 21, 2026 23:14

coderabbitai Bot approved these changes May 21, 2026

View reviewed changes

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

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

nh13 added 2 commits May 21, 2026 17:22

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

c65ce41

nh13 force-pushed the nh/issue-352-phase-4 branch from 691eead to 76fda55 Compare May 22, 2026 00:24

nh13 temporarily deployed to github-actions May 22, 2026 00:24 — with GitHub Actions Inactive

coderabbitai Bot requested changes May 22, 2026

View reviewed changes

Comment thread tests/integration/test_e2e_regression.rs Outdated

nh13 force-pushed the nh/issue-352-phase-4 branch from 76fda55 to 24a952d Compare May 22, 2026 00:51

nh13 temporarily deployed to github-actions May 22, 2026 00:52 — with GitHub Actions Inactive

coderabbitai Bot approved these changes May 22, 2026

View reviewed changes

nh13 merged commit d5d7745 into main May 22, 2026
9 of 10 checks passed

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

This was referenced May 22, 2026

chore: release v0.3.0 #309

Merged

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

Closed

Conversation

nh13 commented May 18, 2026

Summary

Highlights worth flagging

Phasing — complete after this PR

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!

Rate limit exceeded

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

nh13 commented May 22, 2026

Uh oh!

coderabbitai Bot commented May 22, 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