Skip to content

test(p2): pin loose error/output assertions to exact strings (#62 P2)#77

Merged
bashandbone merged 1 commit into
mainfrom
test/p2-tighten-assertions
Jul 26, 2026
Merged

test(p2): pin loose error/output assertions to exact strings (#62 P2)#77
bashandbone merged 1 commit into
mainfrom
test/p2-tighten-assertions

Conversation

@bashandbone

Copy link
Copy Markdown
Owner

Closes the last open item from the #62 audit, plus two follow-ups found while auditing it.

P2 — the loose assertions

integration_tests.rs and command_contract_tests.rs were the two files the earlier P2 batch never reached. Between them they had 11 wide || disjunctions and zero exit-code assertions.

Several couldn't fail in a useful way:

Assertion Why it was weak
contains("already exists") || contains("Use --force") both halves come from the same format string
contains("Nuking") || contains("💥") likewise one println!
contains("Added submodule") || contains("movable-lib") second half implied by the first, which embeds the name
contains("complete") || contains("_submod") || contains("submod") the last alternate matches any output mentioning the binary — the bash completion test passed on anything
contains("Nuking") || contains("Reinitializing") a nuke-without---kill that skipped the reinit half would still pass
contains("Initializing") || contains("initialized") could not distinguish the two states it was meant to check
contains("Updated") || contains("Already up to date") "Already up to date" appears nowhere in the source — only one half was ever reachable

Each is now pinned to the exact string the binary emits, and the two-part cases (nuke + reinit, --all across both submodules) assert each part separately so half-completion fails.

Added assert_eq!(output.status.code(), Some(1)) to the four failing-command tests in integration_tests.rs. The invalid-repo test now requires that the underlying Repository not found cause survives the Failed to create manager wrapping, instead of accepting either one.

Found while writing the update assertion: submod update never names the submodules it touched — it forwards raw fetch refspec output to stdout and then prints a summary count:

+refs/heads/*:refs/remotes/origin/*
	78a3d071… refs/heads/main -> refs/remotes/origin/main [up-to-date]
Updated 1 submodule(s)

The test now pins the count, which is the real contract. The plumbing passthrough is cosmetic and left as a separate concern — flagging it because it's the kind of thing an untightened assertion hides.

Follow-ups

  • #![forbid(unsafe_code)] in lib.rs and main.rs. Cargo.toml was relaxed from forbid to deny so the test crate could #![allow(unsafe_code)] for its tracking allocator — which left every src/ file able to opt out silently. The library and binary now can't.
  • CLAUDE.md claimed performance_tests.rs — benchmarks (uses criterion), which is exactly the doc drift P3 called out; criterion lives in benches/benchmark.rs. Corrected, and the file list now covers fallback_tests.rs, git_ops_tests.rs, and security_tests.rs, with a pointer to the harness's git-state helpers so future tests reach for those over printed output.

Verification

Full suite 569 pass, 0 failed; cargo fmt --check and cargo clippy --all-features --all-targets clean.

Closes #62.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XocU6NjgZricrRWMLyLXDn

Closes the last open P2 item from the audit. `integration_tests.rs` and
`command_contract_tests.rs` still asserted on wide `||` disjunctions and,
between them, had no exit-code assertions at all.

Several of the disjunctions could not fail in a useful way:

- `contains("already exists") || contains("Use --force")` — both halves come
  from the same format string, so the alternation added nothing.
- `contains("Nuking") || contains("💥")` — likewise one println.
- `contains("Added submodule") || contains("movable-lib")` — the second half
  is implied by the first, which embeds the name.
- `contains("complete") || contains("_submod") || contains("submod")` — the
  last alternate matches any output mentioning the binary, so the bash
  completion test passed on anything.
- `contains("Nuking") || contains("Reinitializing")` — a nuke-without-kill
  that skipped the reinit half would still pass. Now both are required.
- `contains("Initializing") || contains("initialized")` could not tell the
  two states apart; both sites now assert the specific state reached.
- `contains("Updated") || contains("Already up to date")` — "Already up to
  date" appears nowhere in the source, so only one half was ever reachable.

Writing the update assertion surfaced that `submod update` never names the
submodules it touched: it forwards raw fetch refspec output to stdout and
then prints a summary count. The count is what the test now pins; the
noisy passthrough is left alone as a separate concern.

Also added `assert_eq!(status.code(), Some(1))` to the four failing-command
tests in `integration_tests.rs`, and made the invalid-repo test require that
the underlying "Repository not found" cause survives the "Failed to create
manager" wrapping rather than accepting either.

Two follow-ups from the same audit ride along:

- `#![forbid(unsafe_code)]` in lib.rs and main.rs. Cargo.toml was relaxed
  from `forbid` to `deny` so the test crate could allow unsafe for its
  tracking allocator, which left every src file able to opt out silently.
  The library and binary now cannot.
- CLAUDE.md's testing section claimed `performance_tests.rs` uses criterion,
  which is the doc drift P3 called out — criterion lives in
  `benches/benchmark.rs`. Corrected, and the file list now covers
  `fallback_tests.rs`, `git_ops_tests.rs`, and `security_tests.rs`, with a
  pointer to the harness's git-state helpers.

Full suite 569 pass, fmt + clippy clean.

Closes #62

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XocU6NjgZricrRWMLyLXDn
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/main.rs 97.46% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bashandbone
bashandbone merged commit ed6fd35 into main Jul 26, 2026
9 checks passed
@bashandbone
bashandbone deleted the test/p2-tighten-assertions branch July 26, 2026 02:38
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.

Test suite audit: tests verify output/config text, not git state (fallback, destructive ops, security under-covered)

1 participant