tests: remove implementation-asserting tests repo-wide - #1012
Conversation
A sweep of every suite (simlin-engine, five other Rust crates, six TypeScript packages, pysimlin) for tests that assert implementation rather than behavior: re-assertions of source literals, tests of local copies of production logic, strict duplicates, and tests that cannot fail. About 660 tests are deleted; the ones whose names promised real coverage were rewritten to earn it (unit_checking's twenty assert_no_unit_diagnostics swaps, exact-value pysimlin sim tests, the invariance oracle's non-empty floor, wasm default-engine and per-sim isolation), and a few documented-but-untested contracts gained their first tests (IconButton href mode, EditOperation's RemoveVariable arm, SD-AI relationship regeneration). Removing the tests exposed prod code with no other callers, now deleted: six dead simlin-serve functions, the engine's monolithic compile remnants and roughly twenty dead items, libsimlin's test-only errors.rs re-export, pysimlin's from_runtime_scores classifier. Test-support modules no longer compile into release binaries (feature-gated, verified with nm against built artifacts). The sweep also surfaced real defects, addressed here: VDF byte readers could panic on arbitrary input despite documented totality, the checked-in SD-AI schema was 139 lines stale behind a test that mutated it instead of checking it, format_diagnostic leaked its "<unknown>" placeholder into the structured variable_name field, and CI's shallow clones made the pysimlin version-tag guard skip vacuously on every run.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
+ Coverage 92.04% 92.17% +0.13%
==========================================
Files 249 247 -2
Lines 161135 158229 -2906
==========================================
- Hits 148318 145855 -2463
+ Misses 12817 12374 -443 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| survives only as a standalone scalar-array convenience utility.) | ||
| separate Python slot-0 reclassification path. (Python's own | ||
| `LoopPolarity.from_runtime_scores` -- a standalone scalar-array port of the | ||
| Rust classifier that no production path called -- has been removed, so the |
There was a problem hiding this comment.
good documentation is NEVER a changelog. it is evergreen writing about the current state of the source code. people can dig through git history to find the changelog.
Documentation describes the current state of the source; it never
narrates the edit that produced it ("X was removed", "used to Y",
"behaviour is unchanged") -- git history is the changelog. This
rewrites the changelog-style passages the test-suite-diet branch
introduced across the ltm design doc, tech-debt.md, three CLAUDE.md
files, and five rustdoc comments, converting the lessons worth keeping
into standing constraints (e.g. why TestFileSystemAccess must remain
an alias for the production impl rather than a second implementation).
Codifies the rule in the root CLAUDE.md's Comment and Rustdoc
Standards so future work lands evergreen by default. GH-anchored
regression rationale (e.g. belt_tests' "the reject GH #924 removed")
is deliberately kept: an issue number is a pointer, not a narrative.
Review FindingsReviewed the full diff (246 files, +2,421/-14,338), with particular attention to production-code changes, removed public APIs, and newly-added/rewritten tests. The sweep is careful — feature-gating for One real, small finding below. [P3] Stale comment claims CI is a shallow clone that skips this guard, but this same PR made CI fetch tags
Overall correctness verdict: correct. No blocking issues; the one finding above is a documentation nit that ships stale but does not affect behavior. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b4d750af6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // present locally. CI checks out with `actions/checkout@v4` and no | ||
| // `fetch-depth`, i.e. a shallow clone with no tags, so this guard runs | ||
| // only on a developer's full clone and is a no-op in CI. That is a |
There was a problem hiding this comment.
The same change sets fetch-tags: true in .github/workflows/ci.yaml, so this guard is now expected to execute in CI; this newly added comment and the skip message below claim the opposite. If tags are unexpectedly unavailable, the logs will incorrectly describe the skip as normal CI behavior and recommend an unnecessary unshallow fetch. Update both descriptions to match the workflow's tag-fetching behavior.
AGENTS.md reference: AGENTS.md:L115-L118
Useful? React with 👍 / 👎.
| @@ -196,20 +192,6 @@ impl Drop for LtmCircuitBudgetGuard { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Deleting ltm::detect_loops leaves the scoped development map claiming that ltm/mod.rs still defines this top-level helper. Future work following that documented module surface will try to import an API that no longer exists, so update the map as part of this removal or retain the helper.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
| Classification of a runtime score series -- including the confidence | ||
| cutoff separating Rux/Bux from U -- happens engine-side in | ||
| `LoopPolarity::from_runtime_scores` / `POLARITY_CONFIDENCE_THRESHOLD` | ||
| (`src/simlin-engine/src/ltm/types.rs`); Python only carries the label | ||
| and the confidence the engine computed, so there is no second |
There was a problem hiding this comment.
This new documentation correctly says Python no longer has a separate classifier, but db::analysis::reclassify_loops_from_results still states at lines 2843–2846 that Run.loops uses LoopPolarity.from_runtime_scores on slot 0 and exposes the engine path separately as Run.loops_runtime. That method is removed in this change and Run.loops now uses the engine result, so update the rustdoc to avoid documenting the opposite A2A semantics.
AGENTS.md reference: AGENTS.md:L115-L118
Useful? React with 👍 / 👎.
Review Findings[P3] Stale doc comment on
|
There was a problem hiding this comment.
💡 Codex Review
Remove the dangling focused-test reference
The focused scalar_target_agg_value_matches_inlined_reducer test is deleted in this change and its assertions are folded into this test, but the rustdoc at line 701 still says that the missing test pins the behavior in detail; a repo-wide search finds no remaining definition. Update the rustdoc to point to the inline assertions instead of a nonexistent regression test.
AGENTS.md reference: AGENTS.md:L115-L118
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| p.x >= r.left && p.x <= r.right && p.y >= r.top && p.y <= r.bottom | ||
| } | ||
|
|
||
| /// Clipped parameter interval `[t0, t1]` of segment `p0 + t*(p1-p0)` (t in |
There was a problem hiding this comment.
Deleting rect_contains_point leaves the scoped development map claiming that diagram/common.rs still provides this shared geometry helper. Future layout work following that documented surface will try to use an API that no longer exists, so remove it from the map or retain the helper.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L192-L192
Useful? React with 👍 / 👎.
| for v in &all { | ||
| match v { |
There was a problem hiding this comment.
When a new BuiltinFn variant is introduced, this loop still ranges only over the manually constructed all vector. The exhaustive match merely forces adding a pattern; adding that pattern while forgetting the constructor leaves the test green, so it does not enforce its claimed every-variant coverage. Derive the cases from a canonical variant list or otherwise make omissions fail.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
| fn convert_operation_maps_every_edit_operation() { | ||
| use simlin_engine::ModelOperation; | ||
|
|
||
| let ops = vec![ |
There was a problem hiding this comment.
When another EditOperation variant is added, the production match will require an implementation but this hand-written vector remains unchanged and the test stays green, leaving the new arm's field mapping unverified despite the test claiming complete dispatch coverage. Generate these rows from the variant enumeration or explicitly document which arms are covered elsewhere.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
| @@ -51,7 +51,6 @@ use simlin_engine::serde as engine_serde; | |||
| use std::ffi::CStr; | |||
|
|
|||
| // ── internal modules ─────────────────────────────────────────────────── | |||
There was a problem hiding this comment.
After deleting libsimlin::errors, the engine development map in src/simlin-engine/AGENTS.md line 92 still says that libsimlin re-exports the engine's error-formatting surface. Repo-wide search confirms that no simlin::errors module remains, so update that cross-component description to avoid directing callers to a removed import path.
AGENTS.md reference: AGENTS.md:L115-L118
Useful? React with 👍 / 👎.
| //! It lives here rather than in the binary so the tests and the binary run | ||
| //! the SAME impl. `simlin-mcp-core`'s integration suites previously used a | ||
| //! hand-maintained near-copy (`test_support::TestFileSystemAccess`), which | ||
| //! had drifted at exactly the two points where this file is non-trivial: it | ||
| //! did not reject `.mdl` writes, and it did not regenerate the SD-AI |
There was a problem hiding this comment.
This newly added module rustdoc narrates where the implementation previously lived and how the former test double drifted, even though repository documentation is required to describe the current state rather than serve as a changelog. Rewrite this as the standing invariant—tests and the binary share this implementation so write semantics cannot diverge—without the historical sequence.
AGENTS.md reference: AGENTS.md:L120-L120
Useful? React with 👍 / 👎.
| it('the toolbar spacer height comes from --toolbar-dense-height', () => { | ||
| const css = readCss('Home.module.css'); | ||
| const m = /\.toolbarSpacer\s*\{([^}]*)\}/.exec(css); | ||
| expect(m).not.toBeNull(); | ||
| expect(m![1]).toContain('height: var(--toolbar-dense-height)'); |
There was a problem hiding this comment.
Deleting this file removes the only assertion that Home.module.css derives its toolbar spacer from the same --toolbar-dense-height token as the actual Toolbar; the surviving theme-tokens.test.ts checks only the Toolbar side. If the Home spacer is later replaced with a literal and the shared token changes, the header and content offset can diverge with the suite still green, so retain a coupling guard rather than dropping this coverage.
AGENTS.md reference: AGENTS.md:L189-L199
Useful? React with 👍 / 👎.
…ode (#1013) Follow-up to #1012, addressing the three codex review findings that arrived as it merged. All three are the stale-claim class: documentation describing a state of the world the code no longer has. - **`simlin-mcp` version guard**: the comment and skip message described CI as a tagless shallow clone where the guard "is a no-op" — the opposite of what `ci.yaml`'s `fetch-tags: true` checkout (added in #1012) does. An unexpected skip in CI would have been misread as documented-normal behavior. Both now state that CI fetches tags specifically so the guard runs, and that a skip is expected only on a local clone without them. (Verified live: with the fresh `pysimlin-v0.8.0` tag fetched, the guard passes against the 0.8.0 release.) - **Engine module map**: `src/ltm/` still advertised the deleted top-level `detect_loops` helper; anyone following the documented surface would import a nonexistent API. - **`reclassify_loops_from_results` rustdoc**: claimed pysimlin `Run.loops` "still reclassifies via its own Python `LoopPolarity.from_runtime_scores` mirror (slot-0 only)" and "exposes the all-slots engine path separately as `Run.loops_runtime`" — both false: `Run.loops` rides this helper via `Sim.get_loops_runtime`, there is no separate property, and the Python classifier no longer exists. The A2A-semantics note is now correctly a two-site comparison (this helper's all-slots concatenation vs discovery's strongest-path scalar) instead of three. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01S3vQ72pUeYXdMgCv7sSjaB
1 participant
Why
A sweep of every test suite (simlin-engine, five other Rust crates, six TypeScript packages, pysimlin) for tests that assert implementation rather than behavior: re-assertions of source literals, tests of local copies of production logic, strict duplicates of stronger tests, and tests that cannot fail (zero assertions, swallowed exceptions, tautologies). Such tests must be edited whenever the source changes even when behavior is preserved, so they impose maintenance cost while constraining nothing.
Deleting them exposed a second layer: production code whose only callers were the deleted tests, and test-support surfaces compiled into release binaries. Net: 246 files, +2,421/−14,338 (about 660 tests deleted, 15 added or rewritten to be real).
Highlights by area
_monolithic/_vmtwin pairs deleted -- both helpers compile through the same incremental path since the salsa unification, so each pair ran one path twice (the stale_incrementalhelper spellings are renamed too). The mdl literal-echo cluster, thestack_effecttranscription family (38 of 100 opcodes -- a sample, not an enumeration), the circularly-self-verifyingdb/differential_tests.rs, nine duplicate corpus registrations insimulate.rs, and ~20 genuinely dead prod items including both 145-linestrip_locwalks,get_incoming_links, and theltm_postscalar pair whose rustdoc claimed an FFI consumer that does not exist.localIdentName: '[local]'); two whole files tested local copies of production logic -- one encoded a wrong copy of the drag-select predicate while passing green.worker-servergetters removed after rewriting their tests to observe behavior.check_and_increment,refresh_meta,save_to_disk,diagnostics_set_changed,baseline_from_errors,datamodel_to_canonical_json) were kept compiling solely by their own tests; three helper duplications consolidated (forward-slash x3, extension dispatcher x2, sidecar rule x2); ~2.4s of pure test sleeps removed.patch.rsfor a substring; tests that pass whether or not the patch applied; the test-onlyerrors.rsre-export module deleted after migrating the uncoveredformat_diagnosticarms into a 7-row table in the engine.LoopPolarity.from_runtime_scores+ threshold constant (hand-duplicated Rust logic, zero callers),ErrorDetailDict,error_code_to_string.Tests strengthened or added instead of deleted
unit_checking_test.rstests whose only assertion was "it compiles" now chainassert_no_unit_diagnostics()-- mutation-tested: breakingmodel_time_unitsreds 6 of them and nothing else in the file.explain()Module arm covered.IconButton's documented href/disabled invariant (diagram CLAUDE.md) gained its first test, mutation-checked against the source line.EditOperationconversion is now a 5-arm table (theRemoveVariablearm had no test anywhere in the repo).TestFileSystemAccessis now a type alias for productionFileSystemAccess(it had silently diverged exactly where production is non-trivial), and the SD-AI relationships-regeneration path has its first test.race.test.tsnow asserts the patches it claims are ordered actually landed.Real defects found and fixed by the sweep
read_u32/u16/f32) violated their documented totality:offset..offset+4could panic on overflow in debug builds (load-bearing under libsimlin's panic=abort). Nowchecked_add.docs/sdai-model.schema.jsonwas 139 lines stale -- caught the moment the schema test became a drift guard instead of a test that mutates the checked-in file on everycargo testrun.format_diagnosticleaked the"<unknown>"placeholder into the structuredvariable_namefield on the Unit arm (the Equation arm correctly passedNonethrough).fetch-tags: trueand the guard is verified to pass against the real tags.test_common, serve/mcp-coretest_support, and the 1.3k-linelayout/eval_statsno longer compile into release binaries (feature-gated; verified withnmon the built artifacts).simlin-serve/simlin-mcp-coretest-supportfeatures must land together (serve's dev-dependency names mcp-core's feature).Known judgment calls / surfaced-not-fixed
design-tokens.test.ts(app) deleted: it cannot detect the drift it names but breaks on token renames. Cheap to reverse if you want that diagram: finish design-token consolidation (shadow tokens, px->rem typography, remaining hardcoded colors, toolbar-spacer token) #799-era guard back.view.size() == 0guards in vm.rs can be deleted with the suite green, and the wasm lowering restates the contract with no parity test. Closing it needs hand-assembled bytecode (a zero-element dimension cannot be built through the pipeline), which would swamp this branch -- documented on the renamedreduce_view_testsmodule instead.🤖 Generated with Claude Code
https://claude.ai/code/session_01S3vQ72pUeYXdMgCv7sSjaB