Audit the test suite: cut what guards nothing, and stop tests copying the code they check - #162
Merged
Merged
Conversation
Four assert that a typed settings store has the keys its own TypeScript type declares — skyConfig, footprint, island, and the repoLabel default shape. The type system already rejects every case they cover; none can fail without a compile error firing first. The fifth, sceneCommands, asserts that selectCommit and focusCommit do not throw before the scene boots. Both are `SCENE_HANDLE.peek()?.…` — optional chaining. There is nothing there to break. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…checks sortForRendering, translateRectsToWorld (with TypedRect), and the HARD_FALLBACK_FILE / HARD_FALLBACK_FOLDER pair have no production caller: the tests deleted here were the only thing referencing them. The __test seam went too, since it re-exported nothing but _rectsOverlap, which rect.ts already exports publicly. Those _rectsOverlap cases move to rect.test.ts, which also moves out of city/utils to sit beside the module it covers. layoutAsserts carried its own copy of overlaps.ts's T-junction predicate, character for character down to the half-unit slop, plus its own re-derivation of rectOfStreet and rectOfBuilding. Two copies of geometry that have to agree is a hand-sync waiting to rot, so assertNoOverlap now runs on findLayoutOverlaps and the predicate is exported rather than duplicated. assertStemOrder was building a childBuildings list and then filtering every entry back out before asserting. That work is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hsl-glsl-parity held 1,181 of the suite's 2,999 cases, 40% of every run, to stop hsl.glsl drifting from hsl.ts. It could not do that. What it compared was hsl.ts against a hand-written JS twin living in the test file; the real shader was read once, for three signature checks. Edit a shader body and all 1,181 cases still pass. Edit hsl.ts and it goes red, and the fix is to edit the twin, which resyncs the twin while the shipped shader keeps the old behaviour. Three implementations, three legs to keep in sync, one leg checked, and it was the leg that does not ship. hsl.test.ts already covers hsl.ts directly with literal expected values. What the shader gets instead is a contract on the operative lines of each body rather than its signature, so an edited formula has to break something. That is still a text match and cannot catch a compile error; a GPU-backed check is the real fix and is not this PR. rainbowChase had a smaller version of the same thing: an expectedRgb helper restating the formula under test. Its cases now assert literal colours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ives src/city has no top-level fireflies/ or trees/; both have lived under components/ since the scene-to-city refactor. The tests stayed behind, so each subject was split across two directories and every one of these files was already importing @/city/components/... from the old location. The two underscore-prefixed helpers were consumed from outside trees/, so they move to _helpers alongside the rest. treeRendererCommitLookup folds into treeRenderer: it is the picker-facing half of the same handle, not a separate module. Its three WIDTH_AGE_FLOOR cases go, since treeEncoding already covers that attenuation against treeRadius directly, and its four sha-lookup null cases collapse into one table. The renderer's height and radius assertions now carry literal numbers instead of restating treeEncoding's curves. They still pin that the renderer feeds those curves into the instance matrices; they no longer pass by construction. The commits helper now defaults to a distinct sha per entry. It was handing every fixture commit the same one, which made the sha-keyed lookups untestable without hand-written shas at each call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…owns createFireflies was exported from both the component door and the inner assembly it builds. index.ts already had to import it as `assembleFireflies` to say which one it meant, and both test files needed a prose header for the same reason. Every sibling component distinguishes the two (createTrees / createTreeRenderer), so the inner one is now createFireflyAssembly. Five cases in fireflies.test.ts asserted ring behaviour that orbitRings.test.ts already asserts directly against the pool: hover-plus-selected collapsing to one mesh, deselect restoring the hover ring, clearing hover emptying the group. Testing the pool through the assembly adds a layer, not a guarantee. The author-colour case stays, since that path (author to hue to lightRgb to material) is only covered end to end here. Also gone: a typeof check on two methods the type already declares, and two no-throw-only cases on an empty renderer. firefliesPlacement's four per-orb range checks were the same assertion over four fields, now one table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
layoutPacker.test.ts tests stemSolver; there is no layoutPacker module, so it is renamed. It also carried three import statements in the middle of the file and a layoutCity end-to-end block that belongs with layoutCity: imports are hoisted, the block moves to algorithm.test.ts. slimManifest and stemScanEquivalence were a file each for one invariant about layoutCity (the worker payload slice produces an identical layout; the hot scan matches the traced scan). Both are real and both now live in algorithm.test.ts. armOnFirstTick and onSettings sat under components/ while testing city/utils/; cityState sat at city/ while testing city/state/. All three moved to match. getStreetWidth was ten it() blocks differing by two numbers, now one table over each tier's first and last count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_extension, _is_binary, _compute_busyness, _derive_tree_signals' date ranges, _annotate_same_day_totals, _parse_svg_length, media_kind and probe_media_dims were 45 methods across seven unittest classes, most of them one input and one expected value. They are now parametrized, so a new case is a row rather than a method, and a failure names the input that broke. Three of the classes existed only to hold a fixture builder shared by their methods; those builders are now module-level functions and the classes are gone. _is_binary drops its mkstemp/addCleanup helper for pytest's tmp_path. Coverage of these functions goes up, not down. Collapsing the media_kind extension loops into rows means each extension is its own case, so a failure says which one; the same for the svg length table. Three cases are new: a unit SVG does not define, explicit svg dimensions outranking a disagreeing viewBox, and same-day totals over non-adjacent dates. Backend: 394 test declarations to 368, 426 cases running, coverage 90%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twelve methods each fed _maybe_raise_clean_clone_error one stderr string and asserted one exception type. They are now two tables: the classification rows, and the five auth-failure spellings that all have to read as unreachable without the message ever claiming the repo is private. The class kept the tests that are about retry behaviour rather than classification, so it is renamed for what is left in it. Verified the table bites: making the classifier raise RepoNotFoundError where it should raise BranchNotFoundError fails exactly the two branch rows, and restoring it goes green. Backend now 363 test declarations from 394, 432 cases running, coverage 90%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cityScene-collision and cityScene-stem-diagnostic were two files for one module, city/diagnostics, named after a scene that no longer exists. They are now diagnostics.test.ts. Both had an import wedged into the middle of their header comment; that is fixed on the way through. repoLabelPositioning restated four of repoLabel.test.ts's cases: the same setAnchor arithmetic, the same HEIGHT_PCT=0 floor case, and the same two settings-effect assertions with different wording. The one case it had that repoLabel did not, a non-zero anchor.y adding to heightWorld rather than being ignored, moves across; the file goes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
renderer, buildingIndex, cellTile and dataFacade were a file each for one function; StreetPaneExclude, infoPane and filePreviewSection were a file each for one or two cases about a component already covered next door. All seven move into the suite for their module. dataFacadeKind's three extension loops become a table, so a failure says which extension. streetPane carried _extBreakdown, a reimplementation of the backend's per-dir extension aggregation. StreetPane reads descendants_ext_breakdown verbatim off the manifest, so the fixture now takes it as data. The counts the tests assert on are visible at the call site instead of being derived by a copy of scan.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleven methods across four cache kinds asserted the same thing: a read of a file that is gone, truncated, or written by older code has to miss rather than raise, or a stale cache bricks the scan. They are now one table of four kinds by three kinds of damage, so a new cache kind is a row. The files row initially did not bite. Its stale-version payload carried malformed entries, which the entry filter drops regardless, so a broken version guard still produced an empty result and the assertion could not tell the two apart. It now carries a well-formed entry: breaking the guard fails exactly [stale-version-files], and restoring it goes green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thalida
commented
Aug 10, 2026
| assert _is_binary(p) is is_binary | ||
|
|
||
|
|
||
| def _commits_per_day(*per_day: int) -> list: |
Owner
Author
There was a problem hiding this comment.
no, shouldnt this be a mock instead?
| const seeded = (seed: number, commitIndex: number) => | ||
| placement(seed * 10, seed * 10, seed, commitIndex); | ||
|
|
||
| const lookupCommits = (n: number) => |
Owner
Author
There was a problem hiding this comment.
Again! I'm going to stop commenting on all of them, please do a thorough pass over all tests.
| // estimateDirAlongReach fix, the phantom seeded into api's local occupancy | ||
| // was sized with parentMaxBoundary*2 + 1000 at recursion start (when api | ||
| // was apps' first child, parentMaxBoundary was tiny); deep grandchildren | ||
| // placed past the phantom could land on top of apps' trunk. |
The same-day block was seven cases over one line of UI: plural vs singular, the tooltip, and four permutations of whether the swatch shows. The swatch ones are now one table over (colour, count), which states the rule the four were circling: the swatch needs both a colour to show and a non-zero count to belong to. The four "renders X synchronously" cases each blocked fetch, mounted, and asserted one selector. One table over (selector, text) does the same and makes the shared point once: everything except the body comes off the CommitEntry the picker already holds, so a blocked fetch cannot delay it. 748 lines to 668, 40 declarations to 32, same 41 cases running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_is_binary reads a file and hands the bytes to gitobj.is_binary_bytes, which is already public. The table wrote four real files to exercise that pure function. It now calls the classifier with byte literals and touches no disk, and the read itself gets two mocked cases: that it reads one chunk, and that an unreadable file fails safe to binary. 'firefly-orbit-rings' was a literal in orbitRings.ts copied into two test files. The name is exported now and both import it. The scene-graph traversals that went looking for it move to a _helpers module, so no test walks children by hand. The fireflies tests hand-wrote CommitEntry and TreePlacement literals that the shared factories already build; both now use them, and the two dynamic await import() calls for authorColor are static imports. Comment pass over every test file: dropped nine headers that echoed the file's own path, and rewrote the longest blocks. Gone with them are the references to Stage 4 Commit 2, Task 9, Task 15, and the drift narration in buildingFixture. Blocks over four lines: 76 down to 63. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
commitTarget was declared three times, twice needing an `as unknown as PickTarget` cast because the literal it built was incomplete. One typed factory in _helpers replaces all three, built on the commits helper so the cast is gone. Nine files hand-wrote CommitEntry literals, and four more declared their own commit(i) / makeCommits(n) generators. They all use the shared factory now, with a commitSeries(n) helper for the n-consecutive-days shape that treeRenderer, picker-commit and picker-timeline each had their own copy of. 'fireflies-orbs' was a second name literal duplicated out of source; it is exported as FIREFLY_ORBS_MESH and imported, like ORBIT_RINGS_GROUP before it. The last four dynamic await import() calls are static imports, so the tests name their dependencies at the top like everything else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #160
Numbers
it/defBoth directions are intended. Frontend runtime cases fall 40%, nearly all of it one file. Backend runtime cases rise, because collapsing a loop-inside-one-test into a parametrized table turns ten silent iterations into ten named cases: same coverage, better failure reporting.
Backend coverage is 90.19%, unchanged. Both goldens have a zero-byte diff from the merge-base.
The two questions this audit asked
The issue framed the problem as size. Sorting by size alone missed the interesting failures, so every file was judged on two questions instead: what bug does this catch, and does it reimplement what it tests.
hsl-glsl-parity.test.tswas 40% of the suite and could not catch the drift it existed for1,181 of 2,999 cases. Its job was to stop
hsl.glsldiverging fromhsl.ts. What it compared washsl.tsagainst a hand-written JS twin inside the test file. The real shader was read once, for three signature checks. Edit a shader body and all 1,181 cases still pass. Edithsl.tsand it goes red, and the fix is to edit the twin, which resyncs the twin while the shipped shader keeps the old behaviour.Three implementations, three legs to keep in sync, one leg checked, and it was the leg that does not ship.
Deleted.
hsl.test.tsalready covershsl.tsdirectly with literal expected values. The shader now gets a contract on the operative lines of each body rather than its signature. That is still a text match and cannot catch a compile error; a GPU-backed check is the real fix and is not this PR.Tests that reimplement their subject
A test that recreates its subject and compares the two proves only that the copies agree. It is blind to a shared mistake, it rots silently, and it hides the expected value. Fixed here:
layoutAsserts(a character-for-character copy ofoverlaps.ts, down to the half-unit slop),rainbowChase(expectedRgbrestating the formula under test), and the height/radius assertions intreeRenderer(now literal numbers, still pinning that the renderer feedstreeEncodings curves into the instance matrices).api/tests/services/test_stats.pyalready does this correctly, pinning a cross-language hash with golden literals, and is the model.Files that guard nothing
Five deleted outright: four asserted that a typed settings store has the keys its own type declares, one asserted that optional chaining does not throw.
Source changes
Dead exports whose only callers were the deleted tests:
sortForRendering,translateRectsToWorld(withTypedRect),HARD_FALLBACK_FILE/HARD_FALLBACK_FOLDER, and the__testseam that re-exported one symbolrect.tsalready exports publicly.createFireflieswas exported from both the component door and the inner assembly;index.tsalready had to alias it to say which one it meant. The inner one is nowcreateFireflyAssembly.overlaps.tsexportsisStreetJoinPairso the test helper can use it instead of carrying a copy.Kept despite matching the dead-export rule:
shadeByRatiohas no production caller, but its GLSL twin is used bybuilding.frag.glsland it is what the contract check is written against.Structure
src/cityhas no top-levelfireflies/ortrees/; both have lived undercomponents/since the scene-to-city refactor, and every one of those test files was already importing from the new location. They moved. So didarmOnFirstTick,onSettings,cityState, andrect, each of which sat in a different directory from the module it tests.layoutPacker.test.tstestsstemSolverand is renamed; there is nolayoutPackermodule.Notes for review
aa0aaddrather than10cb818where its message lives, because it was already staged. Content is correct and green; I did not rewrite history to fix the split.test_clonetable was verified to bite: making the classifier raise the wrong error type fails exactly the two branch rows, and restoring it goes green.Not done, and why
Filed rather than fixed: #161,
createScrubControllertakes the whole world, which is whyscrubController.test.tsreaches line 535 before its first assertion. Restructuring a 558-line controller does not belong in a test PR.Still on the list from the audit, all mechanical: the remaining small-file merges (
StreetPaneExclude,useManifestSourceExcludes,infoPane,filePreviewSection,pathLine/renderer,streetOpacity, the three tinybuildings/files), thetest_cache.pyload-missing/corrupt/version triple repeated across four cache kinds,scrubControllerandcommitPane, and replacing the derived_helpersfixtures with literal manifest data.The full per-file audit, including the finding classes and the sweeps behind them, is in
.claude/superpowers/specs/issue-160-test-audit.md(untracked by request).🤖 Generated with Claude Code