test: clear the audit debt in the test suite - #230
Conversation
0xKT
left a comment
There was a problem hiding this comment.
Reviewed the whole diff (merge-base..head, 51 files, renames resolved with -M) in a
separate worktree. Verdict: approve / mergeable. No before-merge findings. Four
items below, all follow-up or nit; R4 from my first pass is withdrawn.
Verified, so no need to re-check
- Renames are complete. Grepped all 20 old basenames across the whole repo
(docs/, .github/, Makefile, scripts/, benchmarks/): zero hits. The
detect-private-keyexclude in.pre-commit-config.yaml:25and the three docs
paths were updated in the same commit. - Both deletions are justified.
test_chat_e2e.pywas a module-level skip over
threeNotImplementedErrorstubs;tui_commands.py:520does instantiate
SubscriptionEmitter, anddocs/openspec/changes/tui-chat/no longer exists.
test_tui_exit_e2e.pyassertedlancedb_finalization_hazard()is true, and the
probe keys on a live thread name that nothing opens today, so the test could only
fail (rc 43) or skip.tests/test_cli_exit.pyis a strictly better replacement. - The six dropped
importorskipguards were all on hard dependencies.httpx
andeveros[multimodal]==1.1.3are in[project.dependencies];
raven.plugin.memory.everosis bundled in-repo. - New API references all exist.
SessionManager.find_most_recent_chat_id/
peek/get_history(raven/session/manager.py:192,402,88),clipToWidth
(ui-tui/src/lib/text.ts:65),hasMeaningfulReasoning
(ui-tui/src/lib/reasoning.ts:51). _WORKING_REmatches the source of truth. All 15 verbs are identical to
VERBSinui-tui/src/content/verbs.ts:22-38.- The removed locals and dead blocks are genuinely dead (the
responseblock in
test_task_discoverer.pywas shadowed by the reassignment below it). - Branch name, commit type, header length,
Co-authored-bytrailer, and ASCII-only
in both the commit message and this description: all compliant.
R1 - the hazard probe test asserts process-global state
- Tier: follow-up
- Where:
tests/test_cli_exit.py:28 - Claim:
assert not any(t.name == _LANCEDB_THREAD for t in threading.enumerate())
asserts a property of the whole process, not of the unit under test. - Why it matters: it holds today (full suite is green), but any future test that
opens a lancedb connection earlier in collection order flips it red, and the
failure message will read as "the exit guard broke" rather than "something opened
lancedb".tests/conftest.py:15also still documents the opposite ("native
runtimes pulled in by the suite (lancedb's Rust/tokio thread ...)"), so the two
docstrings now contradict each other. - Fix: turn the precondition into a guard rather than an assertion, and reconcile
the conftest comment with what is actually true today:
def test_hazard_false_without_the_lancedb_thread() -> None:
if any(t.name == _LANCEDB_THREAD for t in threading.enumerate()):
pytest.skip("a lancedb connection is already open in this process")
assert lancedb_finalization_hazard() is False- Verify:
uv run pytest tests/test_cli_exit.py -qstandalone and inside
uv run --all-extras pytest -q; both still pass, and the test skips rather than
fails when a thread of that name is alive at collection time.
R2 - the coverage pins a gate that never fires
- Tier: follow-up (separate PR - out of scope here, this one is test-only)
- Where:
raven/cli/commands.py:177-178 - Claim:
if lancedb_finalization_hazard(): flush_and_hard_exit(code)is dead.
The probe returns False in every configuration today (this PR's own docstring says
so), and the only live caller offlush_and_hard_exitistests/conftest.py:32. - Why it matters: not a defect in this PR - the scoping call to leave production
code alone is right. Flagging it so the new coverage does not later become the
argument for keeping the branch: an audit that proves a gate is dormant should end
in deleting the gate, not in pinning it. - Fix: open an issue to delete the branch in
commands.pyand keep
flush_and_hard_exitfor the pytest hook. If it is kept deliberately (e.g. a
planned embedded-lancedb mode), say so in_exit.py's module docstring, which
currently describes the gate as load-bearing. - Verify: n/a for this PR.
R3 - the working-verb regex is now duplicated verbatim
- Tier: nit
- Where:
tests/tui/autotest/tests/test_e2e_streaming_no_log_overlay.py:40-48
andtests/tui/autotest/tests/test_e2e_raven_tui_chat.py:43-51 - Claim: both files now carry the same hand-mirrored
_READY_RE/_WORKING_RE
pair, each with a "keep in sync with verbs.ts" comment. - Why it matters: two hand-maintained copies of one source of truth; a third
arrives with the next e2e file, and a verb-pool edit inverbs.tshas to find all
of them. - Fix: lift both regexes into one module under
tests/tui/autotest/and import
them in the two test files. - Verify:
uv run --all-extras pytest tests/tui/autotest -m e2e -qlocally
(tui-use installed,ui-tui/distbuilt).
R4 - withdrawn
I first read _run_turn's harness.screen() polling
(tests/tui/autotest/tests/test_e2e_raven_tui_chat.py:130-139) as a flake vector
versus harness.wait(). It is not, on two counts: the same loop already exists on
main at test_tui_chat_round_trip (so the helper follows the in-file idiom rather
than introducing one), and the loop has to race two patterns at once -
model_not_available routes to skip, the working verb routes to continue - which a
single-pattern wait() cannot express. No action.
R5 - commit body and description numbers disagree
- Tier: no action
- Claim: the commit body says "Renamed 18 files" / "4615 passed"; this
description says 19 / 4654. The rename count is 19. - Why it matters: it does not. The repo squash-merges with
squash_merge_commit_message=PR_BODY, so the correct numbers are what land on
main. Noting it only in case the commit is ever amended.
A read-only audit of all 298 test files (issue #223) found the suite in good shape -- exactly one genuinely fake test -- with the real debt sitting in file names and a scattering of weak assertions. This clears that, and fixes the bugs found while clearing it. Removed two tests that could never pass anything: - test_chat_e2e.py was a module-level skip over three NotImplementedError stubs. The wire-up it waited for has shipped (tui_commands instantiates SubscriptionEmitter) and the proposal doc it cited is gone. - test_tui_exit_e2e.py asserted that building the agent loop leaves lancedb's thread live. It does not: the loop is constructed but never started, and the memory plugin talks to everos over HTTP, so lancedb is not even imported. The assertion could not hold in any configuration. Renamed 19 files off phase, ticket and bug codes (AGENTS.md section 5.1), reversed one CLI name, and moved two integration files onto a legal kind. Reference updates ride along: .pre-commit-config.yaml names one of these files in a detect-private-key exclude, and three docs pages point at them. Tightened assertions that passed for the wrong reason: eight over-broad pytest.raises(Exception) narrowed to the type each site actually raises, an except clause that swallowed the AssertionError its fakes use as a tripwire, a hasattr probe replaced by binding a broker and checking it stuck, and a recall assertion that read as a pass on an empty result set. Cleaned ten dead locals, three dead code blocks, six importorskip guards on hard dependencies, and two hardcoded /tmp paths. Two gaps the deletions exposed are now covered: - tests/test_cli_exit.py pins the hard-exit guard, which had no tests at all. - The TUI chat e2e gains a multi-turn case. It reads the persisted session rather than the screen, because a prompt is echoed into the transcript the moment it is typed -- screen-scraping for a planted word passes even when history is broken. Fixed three timing bugs in the TUI e2e tests along the way. They keyed off the banner, which paints about seven seconds before the app accepts input, and submitted the moment text was typed. Dropped prompts made the pipeline look dead in one test and made a negative assertion pass vacuously in another. The status-bar patterns both tests wait on now live in one module instead of a hand-mirrored copy per file. Full unit suite: 4654 passed, 25 deselected. The e2e tier passes locally with tui-use installed. No production code changes. The same audit covered the two features that landed while this was in review, and a few gaps there were small enough to close here rather than defer: - session.py fills update_available / update_command for the status bar, but removing that wiring left the suite green; the init-bundle tests now pin both the populated and the up-to-date case. - update_notice._upgrade_command_works was stubbed to True by a fixture everywhere, so its real branches had no coverage. - One episodeSummary case was an empty test body, reported as passing while the +added -removed rendering it named went untested. - clipToWidth and hasMeaningfulReasoning had no coverage at all: replacing the first with an identity function and making the second return true unconditionally both left the suite green. - test_tools_registry.py is renamed to test_tool_registry_execute.py, matching the singular module name its sibling test already uses. Each of those was checked by mutating the code under test and confirming the new assertion fails. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
slashParity.test.ts shelled out to python for raven_cli.commands.COMMAND_REGISTRY and skipped all three cases when the import failed. That package no longer exists -- there is no python-side command registry at all now; unknown slashes go to the CLI through slash.exec -- so the file had been dead, reported as three skips on every run. The parity it wanted needs no python. createSlashHandler.ts:45-51 routes a slash locally when findSlashCommand resolves it and otherwise hands it to slash.exec, which runs the CLI in a subprocess where a mutation cannot reach the live session. The test now asserts that predicate directly. Dropped NATIVE_MUTATING_COMMANDS and the three-way classifyRoute with it: all six names in that set are also in the local registry, so the native branch could not change any outcome and the assertions keyed on it were restating the set. Local vitest: 936 passed, no skips (was 933 passed, 3 skipped). Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
2357153 to
d8f3c86
Compare
Comments across ui-tui cited another project's review threads and issue numbers -- 36 mentions of #19835 as "Copilot round-N review", plus #18994, #19194 and #14045. None of those resolve to anything in this repo, and AGENTS.md section 1.1 rules out comments that reference information only visible elsewhere. The technical rationale each one carried is kept; only the attribution clause is gone, and four describe() titles lose a trailing "(#18994)". Three comments went further and documented a python counterpart that does not exist here: - platform.ts claimed the ``ctrl`` / ``alt`` spellings are normalized identically by raven_cli/voice.py, and that the CLI warns at startup for ``super``. There is no such module; raven/tui_rpc/methods/_stubs.py answers voice.toggle with "voice not supported in Raven v0.1". Reduced to what the file itself does. - useConfigSync.ts and its test credited an ``interrupt`` framework default to raven_cli/config.py and tui_gateway/server.py::_load_busy_input_mode. busy_input_mode appears nowhere in python, and neither module exists; the TUI default is the only one there is. Comments and test titles only -- no behaviour change. Local vitest 936 passed, tsc and prettier clean. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
0xKT
left a comment
There was a problem hiding this comment.
Re-reviewed the full diff at 9c03caa (not just the delta). R1 and R3 are fixed as
suggested, R4 stays withdrawn, R5 needs nothing. One new finding from the third
commit, cosmetic. Approve-ready either way - it is a comment, so merging with it is
also fine.
R7 - stray character left by a comment rewrap
- Tier: nit
- Where:
ui-tui/src/lib/platform.ts:225 - Claim: joining the last two lines of the
parseVoiceRecordKeydoc comment kept
the continuation line's leading marker as/:
* back to the documented Ctrl+B default so a typo never silently
* disables the shortcut. / */
- Why it matters: harmless to tsc, prettier and vitest (it is comment text, which
is why CI is green), but it is visible garbage in the one commit dedicated to
cleaning comments up, and it lands on main until somebody next edits that block. - Fix: drop the
/so the block endsdisables the shortcut. */. - Verify:
cd ui-tui && npx tsc --noEmit && npx prettier --check "src/**/*.{ts,tsx}".
I grepped the same pattern across all 11 files this commit touches; this is the only
occurrence.
Verified on this round, so no need to re-check
cd591a6(R1/R3).test_cli_exit.py:28now skips instead of asserting
process-global thread state, andtests/conftest.pyno longer credits the pytest
segfault to a lancedb thread the suite never starts. The status-bar patterns live in
tests/tui/autotest/statusbar.pyand both e2e files import them;reis still used
in both, so nothing was left dangling.d8f3c86(slash routing). Every claim holds:raven_cliis gone repo-wide (no
COMMAND_REGISTRYanywhere),createSlashHandler.ts:45-51is the right anchor,
findSlashCommandisbyName.get(name.toLowerCase())over names plus aliases, and
all sixNATIVE_MUTATING_COMMANDSnames are in the local registry - so the dropped
native branch could not change an outcome, and "native" was never a route the
implementation had. The negative and alias fixtures are real: no localchannels
command,bgis an alias ofbackground(session.ts:46-48),modelexists
(session.ts:70).9c03caareally is comments only. Read every hunk ofplatform.test.ts(45
lines),useConfigSync.test.ts(31),createSlashHandler.test.ts(19) and
session.ts(22): no assertion added, changed or removed, andsession.ts's
const parsed = r.record_key ? ... : undefinedis untouched.- The sweep is complete. Zero repo-wide hits for
19835,18994,19194or
14045, and zeroraven_cli/tui_gatewayreferences left inui-tui/src. The
remainingCopilotmatches are all GitHub Copilot the provider. - The three deleted python back-references were genuinely fictional. No
raven_cliortui_gatewaymodule exists,busy_input_modeappears nowhere in
python, andraven/tui_rpc/methods/_stubs.pyanswersvoice.togglewith "voice not
supported in Raven v0.1". R2unchanged and still correct.raven/cli/commands.py:177-178cannot fire:
nothing underraven/imports lancedb (all hits are prose), and everos runs
out-of-process viasubprocess.Popen(_server.py:56), so its lancedb thread never
lives in the CLI process. Still worth a separate PR, still out of scope here.
Residual risk, stated for the record
CI does not run tests/integration/* (deselected) or tests/tui/autotest (needs
tui-use and a reachable model). So the two deletions, the xfail change in
test_everos_backend_e2e.py, the new multi-turn e2e and the three timing fixes rest
on the local runs reported in the description rather than on a machine. That is the
repo's existing CI gap, not something this PR introduced.
Rejoining the parseVoiceRecordKey doc comment in the previous commit pulled the block's closing marker into the text, leaving "disables the shortcut. / */". Comment text only, so tsc, prettier and vitest never saw it. Replaying the rewrap with the marker handled correctly reproduces the rest of that commit byte for byte, so this line was the only site affected. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…cess test_stdio_no_executor_does_not_raise set command="true" and let connect_mcp_servers actually spawn it, so the assertion depended on how a real child process raced the MCP handshake. It failed on CI in the full-suite run (CancelledError out of the session's cancel scope) while passing 30 out of 30 standalone runs locally. It also asserted almost nothing: only that whatever exception came back was not SandboxInitError. Following the idiom its sibling test_stdio_sandboxed_with_spawning_does_not_raise already uses, the transport now raises instead of running, and the test asserts the stdio branch was reached with the configured command. Both halves fail under mutation -- dropping "executor is not None" from the guard, and making the stdio branch unreachable. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
A read-only audit of all 298 test files found the suite in good shape -- exactly one genuinely fake test -- with the real debt sitting in file names and a scattering of assertions that passed for the wrong reason. This clears that, and fixes the bugs found while clearing it.
Three dead tests dealt with. Two removed because neither could ever assert anything:
test_chat_e2e.pywas a module-level skip over threeNotImplementedErrorstubs. The wire-up it waited for has shipped (tui_commandsinstantiatesSubscriptionEmitter) and the proposal doc it cited is gone.test_tui_exit_e2e.pyasserted that building the agent loop leaves lancedb's thread live. It does not: the loop is constructed but never started, and the memory plugin talks to everos over HTTP, so lancedb is not even imported. The assertion could not hold in any configuration.The third,
slashParity.test.ts, shelled out to python for araven_cli.commands.COMMAND_REGISTRYthat no longer exists, and skipped all three of its cases when the import failed -- three skips on every run. The parity it wanted needs no python:createSlashHandler.ts:45-51routes a slash locally whenfindSlashCommandresolves it and otherwise hands it toslash.exec, a CLI subprocess a mutation cannot reach the live session from. It now asserts that predicate directly.NATIVE_MUTATING_COMMANDSand the three-wayclassifyRoutewent with it: all six names in that set are also in the local registry, so the native branch could not change any outcome.19 files renamed off phase, ticket and bug codes (AGENTS.md section 5.1), including one reversed CLI name and two integration files moved onto a legal
<kind>. Reference updates ride along:.pre-commit-config.yamlnames one of these files in adetect-private-keyexclude, and three docs pages point at them.Assertions that passed for the wrong reason, tightened: eight over-broad
pytest.raises(Exception)narrowed to the type each site actually raises; anexceptclause that swallowed theAssertionErrorits own fakes use as a tripwire; ahasattrprobe replaced by binding a broker and checking it stuck; and a recall assertion that read as a pass on an empty result set.Dead weight removed: ten unused locals, three dead code blocks, six
importorskipguards on hard dependencies, two hardcoded/tmppaths, and ticket codes left in docstrings after the renames.Two coverage gaps the deletions exposed are now closed:
tests/test_cli_exit.pypins the hard-exit guard, which had no tests at all.Three timing bugs fixed in the TUI e2e tests. They keyed off the banner, which paints about seven seconds before the app accepts input, and submitted the moment text was typed. Dropped prompts made the pipeline look dead in one test, and made a negative assertion (
assert not leak_detected) pass vacuously in another -- no prompt means no streaming means no leak.Gaps in the two features that landed during review, small enough to close here: the
session.pywiring that fillsupdate_available/update_command(removing it left the suite green),_upgrade_command_works(a fixture stubbed it toTrueeverywhere), anepisodeSummarycase with an empty test body, andclipToWidth/hasMeaningfulReasoning, which had no coverage at all. The larger items from that audit are tracked in #229.Upstream review attributions dropped from ui-tui comments. 36 mentions of another project's review thread (#19835 as "Copilot round-N review"), plus #18994, #19194 and #14045 -- none resolve to anything here, and AGENTS.md section 1.1 rules out comments pointing at information only visible elsewhere. The rationale each carried is kept; four
describe()titles lose a trailing(#18994). Three went further and documented a python counterpart that does not exist:platform.tscredited key-spelling normalization toraven_cli/voice.py(no such module --raven/tui_rpc/methods/_stubs.pyanswersvoice.togglewith "voice not supported in Raven v0.1"), anduseConfigSync.tsplus its test credited aninterruptdefault toraven_cli/config.pyandtui_gateway/server.py::_load_busy_input_mode(busy_input_modeappears nowhere in python; neither module exists).One flaky test fixed, caught by this PR's own CI run.
test_stdio_no_executor_does_not_raisesetcommand="true"and letconnect_mcp_serversreally spawn it, so the result depended on how a live child process raced the MCP handshake -- it failed once on CI in the full-suite run while passing 30 out of 30 standalone runs locally. It also asserted only that whatever came back was not aSandboxInitError. The transport now raises instead of running (the idiom its sibling test already uses) and the test asserts the stdio branch was reached.Review follow-ups, folded in: the hard-exit test now skips rather than asserting no lancedb thread exists process-wide,
tests/conftest.pyno longer credits the pytest segfault to a lancedb thread the suite never starts (raven imports lancedb nowhere), and the status-bar patterns the two TUI e2e tests wait on live in one module instead of a hand-mirrored copy per file.Every new or changed assertion was checked by mutating the code under test and confirming the assertion fails.
Type
Test files, test configuration, and comments. The only non-test files touched are ui-tui comments and four
describe()titles -- no behaviour change.Verification
Mutation checks, each confirming the new assertion fails when the code under test is broken: the
session.pynudge wiring removed;clipToWidthreplaced with identity;hasMeaningfulReasoningforced totrue; a never-sent prompt added to the multi-turn session assertion.Risk
No behaviour changes: the non-test diff is comments only. The renames touch git history for 19 test files; #208 is the only open PR affected and has been notified.
Two known items are deliberately left alone:
test_dogfood_whitelist.pyis flaky when the whole file runs (tracked in #228, unchanged here), and thelancedb_finalization_hazardgate incommands.pynever fires today -- its test docstring says so explicitly, so the coverage does not read as an argument for keeping it.Related Issues
Closes #223