sneak/sfdupes
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

No test covers the CLI surface: exit codes, stream separation, or the TSV writers #16

New Issue
Open
opened 2026-08-09 03:46:13 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:46:13 +02:00
Collaborator
Copy Link

There is no main_test.go. All 43 existing test functions call internal helpers — collectDupeGroups, collectTreeGroups, syncScan, hashRuns — and none of them ever calls runScan, runReport, runTrees, or main. The entire user-visible contract is untested.

README-mandated behaviours with no coverage at all:

  • Exit 2: no subcommand, unknown subcommand, scan with no PATH operand, report or trees with a positional argument (README §Error handling).
  • Exit 1: a PATH operand that does not exist, a missing database for report and trees.
  • Exit 0 with warnings after per-file skips.
  • "scan writes nothing to stdout" (README §scan mode) — nothing asserts stdout is empty.
  • The header line and exact row format of both reports, and both stderr summary lines.

These are also the behaviours most likely to regress silently, because nothing about them is exercised by the internal-helper tests.

Definition of done

  1. A table-driven main_test.go builds the binary once via TestMain (or drives the cobra root command in-process, once #4 makes that possible without os.Exit) and, for each case, asserts the exit code, the exact stdout bytes, and the exact stderr line where the README specifies one.
  2. Every case listed above is covered.
  3. scan cases additionally assert stdout is empty.
  4. The suite still finishes well inside the 20-second budget and the 30-second timeout in script/test.
  5. make check green.

Best done after #4, which removes the os.Exit calls that currently make these paths untestable in-process.

There is no `main_test.go`. All 43 existing test functions call internal helpers — `collectDupeGroups`, `collectTreeGroups`, `syncScan`, `hashRuns` — and none of them ever calls `runScan`, `runReport`, `runTrees`, or `main`. The entire user-visible contract is untested. README-mandated behaviours with no coverage at all: - Exit 2: no subcommand, unknown subcommand, `scan` with no `PATH` operand, `report` or `trees` with a positional argument (README §Error handling). - Exit 1: a `PATH` operand that does not exist, a missing database for `report` and `trees`. - Exit 0 with warnings after per-file skips. - "`scan` writes nothing to stdout" (README §scan mode) — nothing asserts stdout is empty. - The header line and exact row format of both reports, and both stderr summary lines. These are also the behaviours most likely to regress silently, because nothing about them is exercised by the internal-helper tests. ## Definition of done 1. A table-driven `main_test.go` builds the binary once via `TestMain` (or drives the cobra root command in-process, once #4 makes that possible without `os.Exit`) and, for each case, asserts the exit code, the exact stdout bytes, and the exact stderr line where the README specifies one. 2. Every case listed above is covered. 3. `scan` cases additionally assert stdout is empty. 4. The suite still finishes well inside the 20-second budget and the 30-second timeout in `script/test`. 5. `make check` green. Best done after #4, which removes the `os.Exit` calls that currently make these paths untestable in-process.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:46:13 +02:00
clawbot commented 2026-08-09 04:40:15 +02:00
Author
Collaborator
Copy Link

Three additions to this issue's scope, all surfaced by the independent review of #4 (merged as 2a055c0). They belong here rather than in their own issues because each is about making the CLI surface properly testable.

1. Thread the output writers through the subcommands. run(args []string, stderr io.Writer) now exists as a single in-process entry point, but it only routes cobra's own output. The subcommands still write to the package-global os.Stdout and os.Stderr, so a test that wants to assert on program output has to monkey-patch os.Stdout. The concrete cost today: TestRunScanSucceedsDespiteWarnings cannot assert that the warning it is named for was actually emitted. Thread the writers down so tests can capture output by passing it in.

2. Add a guard test for runE. The exit-code classification added in #4 works because each subcommand is wired through a runE adapter that sets SilenceUsage/SilenceErrors and wraps failures in fatalError. That is opt-in: a future subcommand wired with a bare RunE would have its runtime failures misclassified as exit 2 with a usage dump — precisely the regression #4 existed to prevent, reintroduced silently. Walk root.Commands() in a test and assert every command goes through the adapter.

3. Rename TestRunMissingOperandIsFatalNotUsage. It covers a nonexistent PATH operand, which is fatal (exit 1). "Missing operand" is README's phrase for scan invoked with no operand at all, which is a usage error (exit 2) and is covered by a different test. Suggested: TestRunNonexistentPathIsFatalNotUsage.

Item 1 in particular should be done before the rest of this issue's table-driven tests are written, since it determines whether they can assert on output directly or have to go through the filesystem.

Three additions to this issue's scope, all surfaced by the independent review of #4 (merged as `2a055c0`). They belong here rather than in their own issues because each is about making the CLI surface properly testable. **1. Thread the output writers through the subcommands.** `run(args []string, stderr io.Writer)` now exists as a single in-process entry point, but it only routes cobra's own output. The subcommands still write to the package-global `os.Stdout` and `os.Stderr`, so a test that wants to assert on program output has to monkey-patch `os.Stdout`. The concrete cost today: `TestRunScanSucceedsDespiteWarnings` cannot assert that the warning it is named for was actually emitted. Thread the writers down so tests can capture output by passing it in. **2. Add a guard test for `runE`.** The exit-code classification added in #4 works because each subcommand is wired through a `runE` adapter that sets `SilenceUsage`/`SilenceErrors` and wraps failures in `fatalError`. That is opt-in: a future subcommand wired with a bare `RunE` would have its runtime failures misclassified as exit 2 with a usage dump — precisely the regression #4 existed to prevent, reintroduced silently. Walk `root.Commands()` in a test and assert every command goes through the adapter. **3. Rename `TestRunMissingOperandIsFatalNotUsage`.** It covers a *nonexistent* `PATH` operand, which is fatal (exit 1). "Missing operand" is README's phrase for `scan` invoked with no operand at all, which is a usage error (exit 2) and is covered by a different test. Suggested: `TestRunNonexistentPathIsFatalNotUsage`. Item 1 in particular should be done before the rest of this issue's table-driven tests are written, since it determines whether they can assert on output directly or have to go through the filesystem.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI checks failing or not yet green
 needs-rebase
Branch conflicts with main; rebase required
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found blocking problems; author must rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/sfdupes#16
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?