462 lines
23 KiB
Markdown
462 lines
23 KiB
Markdown
# Test Suite Policy Review
|
|
|
|
## Context
|
|
|
|
Notarius now has a canonical [Testing Policy](../policy/testing.md). The policy
|
|
defines a risk-based approach that favors durable behavioral coverage while
|
|
removing redundant, brittle, misleading, or obsolete tests whose lifetime cost
|
|
exceeds their protective value.
|
|
|
|
Much of the existing test suite predates that policy. Recent CLI and
|
|
configuration work has already applied several of its principles, but the suite
|
|
has not been reviewed consistently as a whole. This roadmap calls for that
|
|
review before additional domain capabilities materially expand the number of
|
|
tests and fixtures.
|
|
|
|
The review is not based on a presumption that old tests are bad, that direct
|
|
tests of package-private behavior must be removed, or that the suite should be
|
|
made smaller at any cost. Existing tests should be retained when they provide
|
|
durable and nonredundant protection for a meaningful risk. The goal is a leaner
|
|
and clearer allocation of test ownership together with credible protection for
|
|
important behavior.
|
|
|
|
## Objective
|
|
|
|
Review the complete Notarius test suite against the Testing Policy, identify
|
|
both low-value coverage and meaningful protection gaps, and define the
|
|
justified target revisions. The ordered execution plan is maintained separately
|
|
in [Implementation](implementation.md).
|
|
|
|
The review must answer:
|
|
|
|
- Which important contracts, invariants, failure modes, and integration
|
|
boundaries does the current suite protect?
|
|
- Where do multiple tests protect the same behavior without providing distinct
|
|
failure isolation or integration confidence?
|
|
- Which tests are coupled to incidental implementation details, duplicated
|
|
policy literals, closed-world inventories, unstable formatting, or mock
|
|
choreography?
|
|
- Which important risks can still fail silently despite the existing suite?
|
|
- Which tests should be retained, consolidated, rewritten, or deleted, and
|
|
which new tests are warranted?
|
|
|
|
## Review Scope
|
|
|
|
Review all committed Go tests and their supporting fixtures. Organize the work
|
|
by behavioral layer rather than treating test count or package coverage as the
|
|
unit of quality.
|
|
|
|
### CLI and configuration
|
|
|
|
Review command parsing, configuration decoding and precedence, validation,
|
|
effective configuration, run controls, reference selection, output/cache/debug
|
|
state, production composition, and maintained examples.
|
|
|
|
Pay particular attention to:
|
|
|
|
- duplicated assertions across parser, resolver, CLI, and assembled-run tests;
|
|
- tests that restate complete defaults or registry contents rather than
|
|
protecting operator-visible behavior;
|
|
- exact error or output assertions broader than the documented CLI contract;
|
|
- fixture mutation that can silently stop establishing a test precondition;
|
|
and
|
|
- whether representative CLI workflows provide sufficient assembled coverage.
|
|
|
|
### Framework and durable state
|
|
|
|
Review pipeline preparation and execution, artifact contracts, validation and
|
|
retry behavior, LLM scheduling and transport seams, chunk-plan storage,
|
|
checkpoint compatibility, debug bundles, and file persistence.
|
|
|
|
Presume durable protection is important for data integrity, serialization,
|
|
compatibility, cancellation, concurrency, cache correctness, resume behavior,
|
|
atomic or failure-safe persistence, and recovery. Look for opportunities to
|
|
replace many narrow structural tests with a smaller invariant, round-trip, or
|
|
behavior-level test only when protection is not weakened.
|
|
|
|
### Modules and domain behavior
|
|
|
|
Review generic, seriatim, and D&D module tests, including codecs, chunking,
|
|
extraction, merging, normalization, validators, registration, prompt assets,
|
|
and module integration.
|
|
|
|
Confirm that domain rules and artifact schemas have clear test ownership.
|
|
Identify tests that merely reproduce schemas, prompt asset inventories, or
|
|
implementation structure, while preserving tests that protect compatibility,
|
|
source provenance, normalization, validation, or other consequential domain
|
|
invariants.
|
|
|
|
### Cross-cutting suite quality
|
|
|
|
Across all packages, evaluate:
|
|
|
|
- deterministic, offline, credential-free execution;
|
|
- isolation from mutable machine and process-global state;
|
|
- appropriate use of real collaborators, fakes, stubs, and mocks;
|
|
- stable behavioral assertions and useful failure diagnostics;
|
|
- golden files and large snapshots;
|
|
- helper and fixture complexity;
|
|
- test runtime, race safety, repetition stability, and parallel-execution
|
|
assumptions; and
|
|
- semantic ownership and duplication across package, integration, and
|
|
end-to-end layers.
|
|
|
|
Use coverage only as a diagnostic to locate unexpectedly untested critical
|
|
branches. Do not recommend tests solely to increase a percentage or make
|
|
coverage uniform across packages.
|
|
|
|
## Review Method
|
|
|
|
1. Read the Testing Policy and the canonical documentation for each subsystem
|
|
before judging its tests.
|
|
2. Establish a clean baseline with the repository validation commands, focused
|
|
race tests where concurrency or shared state is relevant, and a coverage
|
|
report used only for investigation.
|
|
3. Inventory tests and map each meaningful test or closely related group to the
|
|
contract, invariant, integration boundary, or regression it protects.
|
|
4. Inspect production code only as needed to understand the protected behavior,
|
|
identify the stable boundary, and detect untested risk. Do not infer a
|
|
contract merely from current implementation detail.
|
|
5. Evaluate marginal value across layers. Similar assertions are not redundant
|
|
when one owns a package contract and another distinctly proves production
|
|
wiring or end-to-end integration.
|
|
6. Record evidence for every proposed change. Name the affected test or fixture,
|
|
the realistic defect it currently catches or fails to catch, and why the
|
|
recommendation improves confidence or reduces unnecessary friction.
|
|
7. Check historical context when a test appears unusually specific. Preserve a
|
|
regression test when the underlying defect remains plausible and
|
|
consequential, even if its purpose is not obvious from the current code.
|
|
|
|
Do not modify production code or tests during the review. If the review reveals
|
|
incorrect production behavior, report it separately from test-suite
|
|
harmonization rather than treating a changed test expectation as the fix.
|
|
|
|
## Review Deliverable
|
|
|
|
Produce an evidence-backed report organized by priority and subsystem. Each
|
|
finding must classify the proposed disposition as one of:
|
|
|
|
- **retain:** valuable protection at an appropriate boundary;
|
|
- **consolidate:** overlapping protection that can be represented more simply;
|
|
- **rewrite:** meaningful protection expressed through a brittle or misleading
|
|
boundary;
|
|
- **delete:** no sufficient plausible defect or distinct protection justifies
|
|
the lifetime cost; or
|
|
- **add:** a consequential risk lacks credible protection.
|
|
|
|
For consolidate, rewrite, delete, and add findings, describe the protected risk,
|
|
current evidence, recommended boundary, and expected effect on confidence and
|
|
maintenance. Do not produce a raw list of every test when a package or related
|
|
group shares one clear disposition.
|
|
|
|
Distinguish required changes from optional cleanup. Absence of a finding is not
|
|
evidence that a package needs more tests.
|
|
|
|
## Completion Criteria
|
|
|
|
The review is complete when:
|
|
|
|
- the full suite has been considered at an appropriate behavioral grouping;
|
|
- important test ownership and integration boundaries are mapped;
|
|
- every recommended change is supported by a concrete risk and evidence;
|
|
- high-risk behavior without credible protection is identified;
|
|
- redundant or brittle protection is distinguished from valuable intentional
|
|
overlap;
|
|
- production defects, if any, are reported separately; and
|
|
- every target revision is specific enough to support a decision-complete
|
|
implementation roadmap.
|
|
|
|
## Revisions Needed
|
|
|
|
### Review baseline
|
|
|
|
The review was completed on 2026-07-18 against 90 committed Go test files,
|
|
536 named tests, approximately 21,900 lines of test code, and the committed
|
|
fixtures under `testdata/` and `examples/`.
|
|
|
|
The baseline is clean:
|
|
|
|
- `go test ./...`, `go vet ./...`, and `go build ./cmd/notarius` pass;
|
|
- `go test -race` passes for `internal/framework/pipeline`,
|
|
`internal/framework/llm`, `internal/cli`, and
|
|
`internal/modules/integration`;
|
|
- five shuffled repetitions of the full suite pass; and
|
|
- the default suite is offline, credential-free, and fast. The only HTTP
|
|
behavior uses local test servers or deliberately unreachable loopback
|
|
endpoints without making provider calls.
|
|
|
|
Coverage was used only to investigate risk. It confirms broad behavioral
|
|
coverage in CLI, configuration, source, pipeline, and production module
|
|
packages. It also exposes the material checkpoint gap described below: most
|
|
filesystem loader paths and all source, merge, and normalize checkpoint
|
|
round trips are unexercised. No production defect was identified during this
|
|
review.
|
|
|
|
### Test ownership that should be retained
|
|
|
|
The following overlap is intentional and should remain:
|
|
|
|
- CLI command, run, cache, reference, state-hardening, production-composition,
|
|
and maintained-example tests own exit classification, option mapping,
|
|
physical state placement, default production wiring, and representative
|
|
assembled workflows. Package tests continue to own the underlying parsing,
|
|
resolution, and persistence rules.
|
|
- Configuration contract tests own file decoding, precedence, validation,
|
|
redaction, and effective resolution. Pipeline resolver tests own module,
|
|
capability, typed-variant, validator-chain, and reference resolution after
|
|
configuration has produced a profile.
|
|
- Pipeline runner tests own bounded concurrency, deterministic ordering,
|
|
cancellation, retries, rejection propagation, checkpoint decisions, debug
|
|
recording, candidate/final encoding, and terminal failure behavior. The one
|
|
cross-family concurrency test remains valuable because it proves that the
|
|
framework worker bound and shared provider scheduler remain independent in
|
|
an assembled production-style pipeline.
|
|
- Source, chunk-plan store, file I/O, debug-bundle, output encoder, artifact
|
|
codec, and manifest tests own data integrity, confinement, serialization,
|
|
compatibility, atomic publication, redaction, and round trips. The exact
|
|
checkpoint schema identifier and exact artifact-schema digest tests are
|
|
compatibility tests, not incidental constant assertions, and must remain.
|
|
- Seriatim adapter, D&D scene chunker and spell extractor, D&D validators,
|
|
generic chunker and validators, prompt preparation, and durable spell codec
|
|
tests own their domain rules and external or durable schemas. Leaf
|
|
constructor/spec/registration tests remain justified because the extension
|
|
contract explicitly requires each leaf to be independently buildable.
|
|
- The centralized production import-boundary test remains a valuable
|
|
executable architecture rule. Its synthetic rejected fixture is necessary
|
|
to prove that the checker itself is active.
|
|
|
|
Absence from the revisions below means the current behavioral group has an
|
|
appropriate owner and no justified change was found.
|
|
|
|
### Required findings
|
|
|
|
#### P0 - add filesystem checkpoint compatibility and recovery coverage
|
|
|
|
**Disposition: add.** `internal/framework/checkpoint/recorder_test.go` currently
|
|
proves only that an empty successful extract checkpoint can be written and
|
|
loaded. Coverage confirms that `FilesystemLoader.Source`, `Merge`, `Normalize`,
|
|
manifest validation, payload digest validation, and most recorder status paths
|
|
are otherwise silent. CLI resume coverage proves root selection and one happy
|
|
reuse, while pipeline checkpoint tests use controlled collaborators; neither
|
|
owns the filesystem format.
|
|
|
|
Add package-level tests at the real recorder/loader boundary that:
|
|
|
|
- prove `NewIdentity` is deterministic under reordered lanes, references, and
|
|
fingerprints, changes when pipeline, source/input, selected lanes, runtime
|
|
overrides, references, or provenance change, and produces a confined stable
|
|
relative path;
|
|
- round-trip representative source, extract, merge, and normalize payloads,
|
|
including serialized artifact identity, warnings, extract rejections,
|
|
metadata, and dependency fingerprints;
|
|
- prove caller mutation cannot alter recorded or loaded values and retain the
|
|
existing restrictive-permission expectation through real files; and
|
|
- mutate one persisted artifact at a time to prove that missing or malformed
|
|
JSON, old or unknown workspace schema versions, wrong identity/stage/lane/
|
|
module/status/dependencies, incomplete codec identity, invalid base64, and
|
|
content/output-digest mismatches yield a non-reused decision with useful
|
|
category context rather than a panic or silent reuse.
|
|
|
|
Use relationships and category fragments rather than snapshotting complete
|
|
manifests or error sentences. This addition protects cache correctness,
|
|
compatibility, recovery, and sensitive durable state; it is not intended to
|
|
raise a coverage percentage.
|
|
|
|
#### P1 - remove tests that exercise only their own fakes or obsolete APIs
|
|
|
|
**Disposition: delete.** Remove the fake-behavior tests in
|
|
`internal/framework/contracts/contracts_test.go`:
|
|
`TestFakeExtractorReturnsTypedOutput`, `TestFakeChunkerReturnsSourcePlan`,
|
|
`TestFakeChunkerReceivesPerRunContext`,
|
|
`TestFakeExtractorReceivesChunkAndAmbientContext`,
|
|
`TestFakeMergeNormalizeAndOutputContracts`, and `TestReferenceSetDataTypes`.
|
|
They detect changes in test helpers or Go struct assignment, not defects in
|
|
production contracts. Retain the compile-time interface assertions and the
|
|
clone, JSON omission, and content-ownership tests, which protect type
|
|
compatibility and non-leakage invariants.
|
|
|
|
**Disposition: delete.** Remove
|
|
`internal/framework/llm/scriptorium_api_test.go`. It was introduced as
|
|
pre-adapter API grounding and still mirrors unused Scriptorium request/result
|
|
fields, option constructors, and sentinel errors. Production compilation now
|
|
grounds the API actually used, while `scriptorium_client_test.go` and module
|
|
prompt-preparation tests protect adapter mapping, local integration,
|
|
cancellation, validation, profile selection, and credential redaction.
|
|
|
|
**Disposition: delete.** Remove
|
|
`TestHelpersReturnValidationResults` from
|
|
`internal/framework/validate/validate_test.go`; it is a compile-only assignment
|
|
that adds no protection beyond the function signatures and the two behavioral
|
|
helper tests.
|
|
|
|
**Disposition: delete.** Remove the direct
|
|
`TestTypedNormalizerPreservesValue` test for the generic no-op normalizer. Its
|
|
single assignment is already exercised through resolver, runner, production
|
|
composition, and maintained-example output tests. Retain the direct append-order
|
|
merger test because input ordering is a meaningful transformation invariant.
|
|
|
|
#### P1 - remove misleading D&D integration claims
|
|
|
|
**Disposition: delete.** In
|
|
`internal/modules/integration/dnd_spells_runner_test.go`, delete:
|
|
|
|
- `TestRunnerDoesNotExtractSpellMentionedOnlyInPartyReference`, whose fake LLM
|
|
is programmed to return no spells and therefore cannot prove the claimed
|
|
extraction policy;
|
|
- `TestRunnerCarriesDNDSpellCastWithInvalidSourceRefToSerializedOutput`, which
|
|
omits the production validator chain and duplicates the extractor's explicit
|
|
invalid-evidence handoff test; and
|
|
- `TestRunnerRejectsMalformedDNDSpellsArtifactAtSerializationBoundary`, whose
|
|
final-codec invariant is already owned by the codec and runner candidate/
|
|
final-encoding tests.
|
|
|
|
Retain the representative Seriatim-to-spell assembled workflow and the test
|
|
that proves party/glossary references and provenance cross the production
|
|
module boundary. The policy that reference material is not source evidence is
|
|
durably protected by extractor mapping, prompt-input assertions, source-ref
|
|
validators, and the production validator-chain composition test; it cannot be
|
|
proved by controlling an LLM stub's semantic answer.
|
|
|
|
#### P1 - consolidate superseded configuration and CLI state tests
|
|
|
|
**Disposition: consolidate.** Delete `internal/core/config/v3_test.go` after
|
|
moving its two distinct protections into the current contract owners:
|
|
|
|
- add the version-2 migration rejection case to the strict file-decoding cases
|
|
in `file_config_contract_test.go`; and
|
|
- ensure the positive per-user chunk-plan/checkpoint root separation remains
|
|
in `env_contract_test.go`.
|
|
|
|
The remaining default, precedence, invalid-source, redaction, and removed-field
|
|
assertions are already more completely owned by `file_config_contract_test.go`,
|
|
`env_contract_test.go`, `validation_contract_test.go`, and
|
|
`redaction_test.go`.
|
|
|
|
**Disposition: consolidate.** Delete `internal/cli/state_surfaces_test.go`.
|
|
Move the shared `emptyLookup` helper to
|
|
`contract_test_helpers_test.go`. Its debug flag syntax, allocation timing,
|
|
no-debug absence, version-3 validation, and removed-field cases are already
|
|
owned by `run_contract_test.go`, `state_hardening_test.go`,
|
|
`command_contract_test.go`, and the configuration contract tests. Preserve the
|
|
state matrix, failure retention, no-debug terminal-writer, and pre-resolution
|
|
debug allocation protections in those current owners.
|
|
|
|
**Disposition: rewrite.** Keep
|
|
`TestDefaultCLICompositionResolvesMaintainedConfigurations` as a narrow default
|
|
composition smoke test, but run one representative config-validation command
|
|
through empty `Options` rather than revalidating every maintained example.
|
|
`example_contract_test.go` owns both maintained examples. Remove the repeated
|
|
maintained-example loops from `TestProductionCatalogCoversMaintainedConfigurations`
|
|
and `TestProductionConfigValidationCoversModuleAndVariantFailures`; retain the
|
|
production registry/codec/default-chain checks and the distinct production
|
|
failure cases.
|
|
|
|
#### P1 - consolidate resolver and module-composition duplication
|
|
|
|
**Disposition: delete.** Remove
|
|
`internal/framework/pipeline/default_modules_test.go`. Its large local catalog
|
|
only re-proves default binding selection, already owned by
|
|
`profile_test.go` and `effective_config_contract_test.go`; production default
|
|
keys and wiring are separately exercised by leaf registration, the production
|
|
catalog, default CLI composition, and the maintained end-to-end example.
|
|
|
|
**Disposition: delete.** Remove
|
|
`internal/modules/seriatim/input/transcript/config_test.go`. It builds an
|
|
entire fake catalog to repeat generic resolver success, deterministic digest,
|
|
missing-capability, and unknown-lane behavior. Retain Seriatim parsing and leaf
|
|
registration tests, the Seriatim runner integration, D&D cross-family
|
|
capability tests, resolver contract tests, and production CLI examples.
|
|
|
|
**Disposition: consolidate.** In
|
|
`internal/modules/integration/dnd_spells_config_test.go`, retain only the two
|
|
cross-family capability contracts: Seriatim must provide `source.transcript`
|
|
to the spell extractor, and the spell extractor must provide
|
|
`dnd.spell_casts` to append-order. Express them as one compact table over a
|
|
programmatic profile/catalog. Remove the generic successful-resolution,
|
|
stable-digest, and unknown-lane cases, which are already exercised by the
|
|
retained runner integration and resolver/configuration owners.
|
|
|
|
#### P1 - replace brittle assertions with behavioral ones
|
|
|
|
**Disposition: rewrite.** In `internal/core/source/source_test.go`, make
|
|
`ValidateRef` the owner of malformed reference categories. `ValidateDocument`
|
|
should retain one propagation/context case plus the document-only self-reference
|
|
invariant instead of repeating missing, foreign, and reversed-reference cases.
|
|
Replace complete internal error-sentence equality with the smallest stable
|
|
category and field fragments. Retain exact acceptance/rejection boundaries,
|
|
duplicate detection, deterministic digests, and reference ordering.
|
|
|
|
**Disposition: rewrite.** Replace the decoded-schema-structure walk in
|
|
`internal/modules/dnd/chunk/scenes/schema_test.go` with representative JSON
|
|
Schema validation: accept a valid source-unit-boundary response and reject old
|
|
segment fields, non-positive bounds, invalid enums, empty caveats, and unknown
|
|
properties. Likewise, change the private spell response-schema assertion in
|
|
`internal/modules/dnd/extract/spells/schema_test.go` to accept the LLM transport
|
|
shape without `source_id` and reject a response that supplies canonical
|
|
`source_id`. Retain schema identity/hash, mutation safety, diagnostics
|
|
non-leakage, DTO decoding, and the codec's durable fixture tests. These rewrites
|
|
preserve schema regression protection without coupling tests to nested map
|
|
layout or unsafe type assertions.
|
|
|
|
**Disposition: rewrite.** In `internal/framework/llm/schema_registry_test.go`,
|
|
assert that returned keys are sorted and required framework test schemas are
|
|
present without asserting that the registry contains exactly two entries.
|
|
Retain the explicit negative D&D lookup because it protects the framework/
|
|
domain ownership boundary.
|
|
|
|
#### P2 - make composition and architecture tests open to legitimate extension
|
|
|
|
**Disposition: rewrite.** The family registrar tests in
|
|
`internal/modules/generic/register`, `internal/modules/seriatim/register`, and
|
|
`internal/modules/dnd/register` currently assert closed-world key and asset
|
|
inventories that duplicate the CLI production catalog. Change them to require
|
|
the family-owned registrations they need, permit unrelated future additions,
|
|
and prove representative entries can be built or prepared. Keep exact order
|
|
for the D&D default spell validator chain because that order is documented
|
|
production policy. Keep missing-dependency-before-mutation and contextual
|
|
duplicate-registration cases.
|
|
|
|
**Disposition: consolidate.** Move the source/framework independence rules
|
|
from `internal/framework/chunkplan/import_boundaries_test.go` into the
|
|
centralized `internal/modules/import_boundaries_test.go` checker and delete the
|
|
second repository walker. Preserve both unique rules: `internal/core/source`
|
|
may import neither framework nor modules, and `internal/framework/chunkplan`
|
|
may not import modules. Add rule-level cases so a broken checker still fails.
|
|
|
|
#### Documentation alignment
|
|
|
|
**Disposition: rewrite.** When the affected tests move or are deleted, update
|
|
the `Tests To Inspect` sections in `docs/internal/state.md`,
|
|
`docs/internal/pipeline.md`, `docs/internal/modules.md`, and
|
|
`docs/internal/llm.md`. They currently name files such as
|
|
`internal/cli/state_surfaces_test.go` and `internal/cli/run_test.go` that will
|
|
be deleted or do not exist. Point each document at the retained contract,
|
|
state-hardening, production-composition, example, checkpoint, and integration
|
|
owners without recreating an exhaustive test inventory.
|
|
|
|
### Optional cleanup
|
|
|
|
No additional cleanup is recommended now. In particular, do not mechanically
|
|
convert the large resolver and runner suites to table-driven form, merge all
|
|
leaf registration tests into family registrars, add tests for trivial accessor
|
|
coverage, or introduce a golden-update framework. Those changes do not provide
|
|
enough additional confidence to justify their immediate cost.
|
|
|
|
### Target state
|
|
|
|
The revision is complete when:
|
|
|
|
- every checkpoint stage has a real filesystem round trip and incompatible or
|
|
corrupted state is demonstrably recomputed rather than silently reused;
|
|
- all protection named as retained above remains present at its stated owner;
|
|
- the fake-only, obsolete, misleading, duplicate, and closed-world assertions
|
|
named in the required findings are removed or rewritten exactly as specified;
|
|
- maintained examples are each owned by one example contract plus one narrow
|
|
default-composition smoke path, rather than repeated across production tests;
|
|
- default tests remain deterministic, offline, credential-free, order
|
|
independent, and race-clean;
|
|
- failures identify the violated behavioral category without snapshotting
|
|
complete incidental diagnostics;
|
|
- internal documentation points to existing retained test owners; and
|
|
- the suite reaches this state without changing production behavior.
|