Skip to content

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

Closed

Description

@nh13

Problem

The integration test suite under tests/integration/ invokes the fgumi CLI as a subprocess (std::process::Command::new(...)) in ~120 places across 22 files, covering every user-facing command. The coverage CI job uses cargo llvm-cov nextest, which only instruments the test binary — spawned subprocesses are not instrumented. As a result, every Command::execute() body is effectively unmeasured even though it is well exercised by integration tests, which depresses reported coverage and produces misleading patch-coverage failures on PRs that touch execute() (see #351 for a recent example: 4 of 4 patch-coverage misses fell inside MarkDuplicates::execute).

Proposal

Convert the integration test suite to construct the clap-parsed command struct directly and call Command::execute() in-process. The Command trait (src/lib/commands/command.rs) is already shaped for this — &self, all I/O parameterized via PathBuf fields, returns Result<()> — and one test already does it.

Existing template

tests/integration/test_dedup_command.rs::test_dedup_no_umi_large_position_group (around line 201) is the model the rest of the suite should follow:

use clap::Parser;
use fgumi_lib::commands::command::Command as FgumiCommand;
use fgumi_lib::commands::dedup::MarkDuplicates;

let cmd = MarkDuplicates::try_parse_from([
    "dedup", "--input", in_bam, "--output", out_bam, "--no-umi", "--compression-level", "1",
])?;
cmd.execute("test")?;

Subprocess footprint by command

Command Files Calls
extract 1 18
group 2 3
dedup 1 3 (+1 already in-process)
correct 1 4
zipper 2 9
fastq 1 7
filter 1 6
clip 1 2
sort 3 8
downsample 1 7
simplex 2 13
duplex 2 11
codec 1 6
review 1 3
e2e / utilities 5 24

Prerequisite refactors

Two commands need small refactors before their tests can move in-process:

  1. zipper reads std::io::stdin() directly when --input=- (affects 4 tests in test_streaming_input.rs). Options: parameterize the reader, or have the tests use a temp file instead of the stdin path.
  2. compare calls std::process::exit(1) on a mismatch in src/lib/commands/compare/{bams,metrics}.rs. Change to Result<bool> (or a dedicated error variant) so in-process callers can observe the result. Affects test_compare_bams.rs and uses in test_e2e_regression.rs.

No other global-state hazards were found: no static Once/Mutex, no global env_logger init dependency, rayon pools are scoped per-command.

Phasing

Land as stacked PRs so the test-mechanics review stays separable from any logic changes:

  • Phase 1 — extract, group, dedup, correct, fastq, filter, clip (~48 tests). Pure --input/--output PathBuf commands; straight 1:1 conversion.
  • Phase 2 — simplex, duplex, codec, *-metrics, review (~28 tests). Same pattern, larger argument surfaces.
  • Phase 3 — zipper refactor + its 9 tests; compare refactor + its tests.
  • Phase 4 — sort, downsample, remaining e2e files (~15 tests).

Why this beats relaxing codecov.yml

Adding patch/project thresholds would hide the symptom for #351 but leave the underlying gap: every future execute() change would land uncovered by metrics. Capturing subprocess coverage via LLVM_PROFILE_FILE=%p-%m.profraw propagation is also possible, but in-process invocation is simpler, gives faster tests (no fork/exec per case), and produces clearer failure traces (real Rust backtraces instead of "process exited with status 1").

Acceptance criteria

  • All integration tests that exercise a single fgumi subcommand call Command::execute() in-process.
  • cargo llvm-cov nextest reports coverage on execute() bodies.
  • compare and zipper refactors land with their own focused commits.
  • Remaining subprocess invocations are limited to genuine pipeline tests (e.g., fgumi extract | samtools view, multi-binary end-to-end flows).

Activity

  1. nh13 commented on May 22, 2026

    @nh13
    MemberAuthor

    Completed across phases 1–4 plus follow-ups:

    Command::new invocations in tests/integration/ dropped from ~137 to 37. The remaining 37 are intentional and not in scope:

    • Real subprocess required by the test surface: stdin/pipe scenarios (test_async_reader.rs, test_bgzf_eof.rs, test_streaming_input.rs, test_zipper_command.rs::test_zipper_bam_stdin_input)
    • Cross-process determinism guards that depend on a fresh AHashMap seed per process (test_group_determinism.rs, test_extract_command.rs::test_parallel_parse_determinism, test_e2e_regression.rs)
    • External-tool wrappers, not fgumi itself (test_simulate_sort.rs, test_sort_correctness.rs, test_sort_write_index.rs call samtools and cargo)
    • test_compare_bams.rs exit-code assertion

    Coverage instrumentation for Command::execute bodies is restored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions