TimZander / TimZander/claude

skill: merge-base test check — do a PR's new tests fail without the change they cover?

Open
#212 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
0
Forks
1
Avg merge
1d 3h
Merged PRs (30d)
7

Description

Problem

Tests get shipped that pass even when the code they cover is deleted. In one repo this was the most frequent defect class found in review over a week — across several PRs, including one where three of four new tests stayed green with the guard they named removed.

A written agreement ("a guard is not finished until you have watched it fail") did not change behaviour: it was in a file that had already been read. A bespoke mutation-testing tool built to mechanise that agreement did not either, for a reason worth recording — nothing invoked it. It required the same person who skipped the rule to voluntarily hand-author an honest mutation spec and remember to run it. It also only probed what its author already thought of: the author's own mutation matrix scored 24/24 caught, while an adversarially-written set found survivors in most of the same file.

The check

Run the PR's new tests against its merge-base. A test that passes without the change it covers cannot be constraining that change.

git worktree add --detach /tmp/base "$(git merge-base origin/main HEAD)"
# overlay the PR's changed test files onto the base
cd /tmp/base && <run the scoped suite>

No spec to author, nothing pinned to source text that can rot, nothing author-selected. It reads the diff that already exists, and it is language-agnostic — it tests the tree, not the language, so it covers shell, which has no maintained mutation tooling at all.

Evidence

Run against the PR that motivated the original tooling effort: 14 seconds. Of the 11 tests new in that PR, 8 fail on the merge-base and 3 pass — and the 3 that pass are exactly the negative-space tests, the same ones that had stayed green when the guard was deleted. A check with nothing to write isolated the whole vacuous population.

The rule has to be "every", not "at least one"

"At least one new test fails on the merge-base" would have passed that PR — eight of its tests failed — and shipped all three vacuous ones. The workable form is:

Every new test must fail on the merge-base, or carry an in-diff annotation naming its positive-direction partner, e.g. # passes-on-parent: guards the false-positive direction; the killed-guard case is test_<name>.

Negative-space tests are legitimate. The check forces classification rather than deletion, and the annotation is "I watched it fail" made mechanical, reviewable, and sitting in the diff where a reviewer sees it.

Measurement gotcha, learned the hard way

Determine per-test results by running each test individually and reading its exit status. Parsing unittest -v output with a line-oriented regex gives the wrong answer, because -v prints the test's docstring between the test name and ... ok. That mistake scored every test as failed and nearly shipped a false conclusion.

Known limits

  • New source files. A new test referencing a file absent from the merge-base fails with a collection error — technically "failed", but it proves the test references the new code, not that it constrains it. Report the failure kind; treat collection errors as inconclusive rather than verified.
  • Refactors legitimately stay green — needs a per-PR skip.
  • Characterization tests legitimately pass on the merge-base — same annotation escape hatch.
  • Scope to unit suites; long e2e suites and stubs that drift against older trees make this unreliable and slow.

Why a skill rather than documentation

It is several commands with a non-obvious measurement step, repeated for every test-bearing PR. Documentation describing it would be skipped exactly as the working agreement was. Cost is one extra scoped test run — seconds — so it can live in a pre-push hook or a path-gated CI step rather than needing to be remembered.

Acceptance criteria

  • A skill/command that finds the merge-base, overlays the PR's changed test files, and runs the scoped suite
  • Per-test results determined by individual exit status, not by parsing runner output
  • Reports each new test as fails-on-base / passes-on-base, and distinguishes collection errors as inconclusive
  • Recognises a passes-on-parent: annotation and treats an annotated test as classified rather than failing
  • Refuses to run on a dirty tree; leaves no worktree or scratch state behind
  • Documents that a pass is evidence about one probe, not proof the suite is sound

Out of scope

Framing this as proof of coverage. A test that fails on the merge-base is constrained by that change; it says nothing about the rest of the suite. The control that has actually caught every instance of this defect is adversarial review — this makes that review cheaper and should not be described as replacing it.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the repository's existing skill or command entry point and its test conventions. Reproduce the described merge-base workflow with the scoped suite, measuring each test through its individual exit status. Done means the acceptance criteria are met, including annotation handling, inconclusive collection errors, dirty-tree refusal, and cleanup of temporary worktree state.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, python
Domain
devtools, testing-qa, tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.