Skip to content

test: prune the dead tests, and give the suite a shape - #396

Open
hugolytics wants to merge 8 commits into
mainfrom
chore/prune-and-consolidate-tests
Open

test: prune the dead tests, and give the suite a shape#396
hugolytics wants to merge 8 commits into
mainfrom
chore/prune-and-consolidate-tests

Conversation

@hugolytics

Copy link
Copy Markdown
Collaborator

The problem

The suite was 359 files and 3,192 tests, green in 33 seconds. Speed was never
the issue — sprawl was. tests/unit/ was 269 files in one flat directory, and
some of them were guarding code that nothing calls any more.

Three things were wrong, and only the first is obvious:

  1. Tests outliving their subject. Six modules test code with no caller
    anywhere in src/ or scripts/ — superseded when haunting moved to
    fateforger/haunt/ and the flow moved to the stage-gated agent. One of them
    asserts the line numbers of methods in a code-navigation helper.
  2. A refactor scaffold that outlived its refactor. _parse_event_dt exists
    at three call sites, and each grew its own branch-by-branch table written,
    per its own docstring, "before and after the refactoring to Pydantic
    dispatch". The refactor landed. All three now delegate to one
    EventDateTime.to_datetime, whose table is tested directly elsewhere.
  3. Doubles copied instead of shared. Nine modules had each grown their own
    DummyClient for the Slack web client, differing only in the timestamps
    they invent. Where a double was shared, it was shared by importing another
    test module — which works only while both happen to be collected.

What changed

before after
test files 359 341
tests/unit/ flat files 269 0
tests 3,192 3,137 (+89 slow, unchanged)
lines 76,892 75,820
  • Pruned six dead-subject modules (44 tests, 6 of them already skipped).
  • Collapsed the three duplicated date-parsing tables. What each call site
    still owns — the ISO-string branch — stays, because those genuinely differ:
    reconcile keeps the parsed offset, the planner converts into tz. That
    divergence now has a test naming it, which it did not before.
  • Restructured tests/unit/ into subpackages beside the tmbx/ one that
    already worked this way: timeboxing/ (96), slack/ (49), constraints/
    (34), core/ (33), tmbx/ (22), haunt/ (14), tasks/ (9), schedular/ (7).
  • Extracted tests/doubles/ — one Slack client recorder replacing nine, plus
    the reconciler, planning, required-block and kernel doubles that modules were
    importing out of each other.
  • Added tests/repo.py. Twenty files found the repo root by counting
    parents[N], so moving a test one level down broke it with a
    FileNotFoundError that reads like a missing fixture. It walks up to
    pyproject.toml instead.
  • Folded 20 one- and two-test fragment files into nine subject modules.

Proof

Every lost line is accounted for. Coverage measured per line, before and
after, on the same command:

before  22,221 / 28,593  77.71%
after   21,947 / 28,593  76.76%

All 274 lost lines are in the six modules with no caller, plus the
llm/factory and tools_config branches only their tests reached. No other
file lost a covered line.

The one exception, timeboxing_session_store.py:157, is the raise StaleSessionRevision inside a concurrent-save race. Per-test coverage contexts
run against origin/main show that line is not covered reliably there
either
— the concurrent-save test wins the race nondeterministically. Not a
regression from this branch.

Merges cannot silently shadow. The merge tool refuses any merge where two
sources define the same top-level name.

Three near-duplicates were investigated and kept. planning_day_for against
the starter that calls it, two ConstraintStore prune scenarios, two MCP
clients' empty-loader guard — same test name, different seam. Only genuine
duplicates were removed.

3,137 passed, 4 skipped, 89 deselected, 1 xfailed
3,231 collected including the slow evals (not run — they hit OpenRouter)

What I did not do, and why

The dead source modules are still here. This PR is test-only. Deleting them
is a source change and wants its own PR. The evidence, if you want it:

admonisher/{base,calendar,commitment}, schedular/diffing_agent,
timeboxing/{flow,prompts,state,notebook_entrypoints},
slack_bot/{relay_agent,topics}, tools_config/ — no caller in src/ or
scripts/, last touched January–February 2026, and now visibly at 0% coverage.

The docs are in this PR, not a follow-up. tests/README.md was a
hand-maintained per-file index — which is why it rotted, and why it still
listed test_timeboxing_flow.py as live logic. It now describes the shape
(subpackages, tests/doubles/, tests/repo.py) instead of the inventory.
CALENDAR_QUERY_LOCATIONS.md and MIGRATION_ARCHIVE_TO_CALENDAR_HAUNTER.md
carry superseded banners pointing at fateforger/haunt/, with their original
text intact as a record.

Two claims in that rewrite were wrong and are fixed in a follow-up commit: it
said the fast suite is "what CI and pre-merge checks run" (nothing in
.github/workflows/ runs pytest at all), and it told you to run
.venv/bin/python while explaining why worktrees break the venv — a worktree
has no .venv, so that path resolves to nothing exactly where the warning
applies.

timeboxing/ is still 96 files. It is the largest genuine subject in the
repo and folding it further needs judgement per file, not another pass of the
merge tool.

One gap this surfaced but did not fix: fateforger/haunt/orchestrator.py
has its own capped exponential backoff and no test reaches it. The capped-backoff
test I deleted belonged to the dead BaseHaunter, so nothing regressed — but
the live one has never been covered. Worth a ticket.

Checklist for review

  • The six pruned modules really are dead — spot-check one with
    grep -rn CalendarHaunter src/ scripts/
  • The kept _parse_event_dt string-branch tests are the right call: the two
    implementations disagree about timezone conversion, and neither is
    obviously wrong
  • The subpackage boundaries match how you think about the system —
    constraints/ split out of timeboxing/ is the one most open to argument
  • tests/doubles/ is the right home, and one Slack recorder for nine call
    sites does not over-generalise
  • Whether the dead source modules should go in a follow-up or be left

🤖 Generated with Claude Code

hugocool and others added 8 commits September 8, 2026 02:03
The unit suite had grown to 300 files in one flat directory, and some of
them were guarding code nothing calls any more.

Pruned, subject by subject. Each of these targets a module with no caller
anywhere in `src/` or `scripts/`, superseded when haunting moved to
`fateforger/haunt/` and the timeboxing flow moved to the stage-gated agent:

  test_backoff.py                          admonisher.base
  test_calendar_haunter.py                 admonisher.calendar
  test_calendar_haunter_integration.py     admonisher.calendar (mostly skipped)
  test_diffing_agent.py                    schedular.diffing_agent
  test_timeboxing_flow.py                  timeboxing.flow
  test_timeboxing_notebook_entrypoints.py  timeboxing.notebook_entrypoints

The last one asserted the *line numbers* of methods in a code-navigation
helper. The root docs still call CalendarHaunter production code; they are
stale, and the follow-up ticket says so.

Collapsed one expired refactor scaffold. `_parse_event_dt` exists at three
call sites, and each had its own branch-by-branch table written "before and
after the refactoring to Pydantic dispatch". The refactor landed: all three
dict branches now delegate to `EventDateTime.to_datetime`, whose table lives
in test_contracts_event_datetime.py. What the sites still own is the ISO-string
branch -- and those genuinely differ, so they stay: reconcile keeps the parsed
offset, the planner converts into `tz`. That divergence now has a test naming it.

Merged test_notion_mcp_url.py into test_mcp_url_validation.py, which already
covered the same validator for both MCP servers.

Then gave the rest a shape: tests/unit/{timeboxing,slack,constraints,core,
haunt,tasks,schedular}/ beside the tmbx/ package that already worked this way.

Two things had to be fixed to make the move safe, and both were latent bugs:

  - Nine modules had each grown their own `DummyClient` for the Slack web
    client, differing only in the timestamps they invent. They are one
    double now (tests/doubles/slack.py), along with the reconciler, planning,
    required-block and kernel doubles that two or more modules shared by
    importing each other's test files -- which only worked while both
    happened to be collected.
  - Twenty files found the repo root by counting `parents[N]`, so moving a
    test one level down broke it with a FileNotFoundError. tests/repo.py
    walks up to pyproject.toml instead.

Coverage before 22221 lines, after 21957. Every one of the 264 belongs to
the five pruned modules, plus the llm/factory and tools_config branches only
their tests reached; measured per line, no other file lost one. The single
remaining difference, timeboxing_session_store.py:157, is a race the
concurrent-save test wins nondeterministically -- per-test contexts show
origin/main does not cover it reliably either.

3141 passed, 4 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
84 of the 348 test modules held one or two tests each, so a subject was
spread over four files and no file said what it was about. Folded, subject
by subject, with the sources kept verbatim under a heading each:

  test_skeleton.py              the context, Stage 3's draft contract, the
                                timeout fallback, Stage 2 pre-generation
  test_stage_decisions.py       the decision node, the fallback, the gate's
                                JSON context
  test_memory_surface.py        review actions, tool results
  test_constraint_extractor_tool.py  strict signature, non-blocking, background
  test_slack_setup.py           bootstrap, invite, response
  test_slack_revisor.py         channel redirect, weekly review
  test_planning_card.py         + dismiss, time picker, date reselect,
                                register instrumentation
  test_slack_timeboxing_surface.py   DM routing, focus recovery
  test_timeboxing_constants.py  + the timeouts derived from them

Each merge is refused automatically if two sources define the same top-level
name, so nothing is silently shadowed.

Two merges were judgement rather than concatenation:

  - The three stage-prompt files asserted overlapping substrings, one of them
    against `timeboxing.prompts` -- a module with no caller. test_stage_prompts.py
    keeps the four assertions that carry a contract (blocks over durations,
    deterministic-first defaulting, never naming a tool the agent lacks) and
    drops the phrasing locks. 6 tests to 4.
  - test_timebox_backend_routing.py was two subjects, and its half about the
    harness body duplicated test_timebox_bare_command.py outright. Backend
    selection stays; the unique body test moved across; the two duplicates
    are gone.

Three other same-name pairs the AST scan flagged turned out to be different
seams wearing one name -- `planning_day_for` against the starter that calls
it, two ConstraintStore prune scenarios, two MCP clients' empty-loader guard.
All kept.

348 files to 328. Coverage: 274 lines lost, every one of them in the six
modules with no caller (notebook_entrypoints, admonisher.base and .calendar,
diffing_agent, timeboxing.flow and .prompts) plus the llm/factory and
tools_config branches only their tests reached. No other file lost a line.

3137 passed, 4 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…prune

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tests/README.md carried a hand-maintained per-file index: a row per test
module, already listing test_timeboxing_flow.py as "Legacy flow logic" for a
subject with no caller since February, and dozens of other files that no
longer exist after the unit suite moved from 300 flat files into subject
subpackages (tests/unit/{timeboxing,slack,constraints,core,haunt,tasks,
schedular,tmbx}/). That index rotted because every new test file is a doc
edit nobody makes. Replaced it with the shape instead of the inventory: what
each subpackage is for, the rule behind tests/doubles/ (a double used by two
modules lives there, because pytest's importlib collection mode makes
importing a fake out of another test file order-dependent) and tests/repo.py
(walk up to pyproject.toml, don't count parents[N]), and how to actually run
things.

Checked every command before writing it down. `poetry run pytest` resolves
to the same venv as running it directly today, but a `poetry install` inside
a worktree silently repoints the parent .venv (the 2026-09-03 incident in
AGENTS.md), so the README now tells people to invoke .venv/bin/python
directly rather than through poetry. Ran the full `pytest tests -m "not
slow"` before and after to confirm the doc rewrite touched nothing else:
3137 passed, 4 skipped both times.

CALENDAR_QUERY_LOCATIONS.md and MIGRATION_ARCHIVE_TO_CALENDAR_HAUNTER.md
called CalendarHaunter (admonisher/calendar.py) production and recommended
migrating to it. Nothing has constructed it since haunting moved to
fateforger/haunt/, and its tests were removed in this same prune. Marked
both superseded with a pointer at fateforger/haunt/ rather than rewriting
them, since they're a record of what was true when written.

Did not touch docs/superpowers/research/ or docs/superpowers/plans/, and did
not delete the dead source modules themselves — that's a separate,
out-of-scope change per the ticket.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rewrite said the fast suite is 'what CI and pre-merge checks run'.
Nothing in .github/workflows/ runs pytest -- there are two workflows, for
docs deployment and notebook checks. A README that says CI has your back
when it does not is worse than one that says nothing, so it now says
plainly that someone has to run them.

It also told you to run '.venv/bin/python' while explaining why worktrees
break the venv -- and a worktree has no .venv of its own, so that path
resolves to nothing exactly where the warning applies. Says where the one
venv lives, and how to name it from a worktree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n make what survives composable

Two projects in a fixed order. Reachability from the real entry points
shows the legacy TimeboxingFlowAgent is the only path to 33 modules and
24.7k lines, and 442 tests touch that code; making those tests DRY before
deleting them is the waste the order exists to prevent.

Project 2 is a five-seam taxonomy with one construction rule per seam,
six builders where the literal-call counts earn them, one injection
keyword on the four MCP clients tests currently __new__, and an AST guard
that fails the suite on any __new__ or private assignment so the rule
holds after the refactor, not just during it.

The conftest autouse fixture that pins the suite to the legacy backend is
named as project 1's real risk: it exists because the harness path spawns
a subprocess per test, so the flag is replaced by a stubbed harness turn,
not just removed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The first draft was written from a static import graph. Five Opus spikes
measured it -- one performed the deletion in a throwaway worktree, one
audited every August legacy fix against the harness, one drove a stubbed
AutoGen runtime, one traced the constraint stores and the host env, one
implemented and mutation-tested the injection seam. Three assertions were
wrong and are corrected:

  - 24,657 lines / a third of src was a double-count in blast.py. The cut
    is 15,830 lines, 23.6%.
  - The conftest legacy pin was not what kept the suite in-process, and
    _harness_turn -- the function the draft proposed stubbing -- has zero
    callers. The suite ran identically with the fixture removed (29.2s vs
    31.6s). The fixture is deleted, not replaced.
  - The four-client injection section is gone. Two of the clients die with
    the agent; the other two have no workbench and need no source change.

Two things the draft did not know:

  - Four live send_message sites still address timeboxing_agent (the DM
    follow-up route fires on every second turn). Each becomes a bare
    "Recipient not found". They close first, guarded by a registry-
    consistency test that fails on two paths today.
  - The harness lacks one legacy behaviour: the consecutive-no-progress cap
    from 9eb333e. NeedsAnotherTurn is uncapped. It is ported into the kernel
    before the legacy tests that guarded it are deleted.

Undo, the five stage cards, the confirm gate and the migrated constraint
store all exist on the harness; the routing test's docstring claiming
otherwise goes with the test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…closed first

Task 1 is a test that fails today: no Slack path may hand the runtime an
AgentId for timeboxing_agent. Tasks 2-3 close the four sends and retire
the legacy cards. Task 4 ports the consecutive-no-progress cap into the
kernel, read from the outcome kinds the snapshot already records. Task 5
deletes the class, the flag, the conftest pin and the orphaned subtree
with the reachability tool as the authority. Tasks 6-7 drop the store
only legacy wrote and rewire two live tests. Task 8 lands the AST guard
with a measured allowlist. Task 9 is docs, the two spec deviations, and
the live-drive checklist the suite cannot stand in for.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants