Record test coverage and effectiveness audit findings

2026-10-25 17:07:39 +00:00
parent eb5a82f1f3
commit b1c278b38b
1 changed files with 281 additions and 0 deletions
--- a/docs/roadmap/testing.md
+++ b/docs/roadmap/testing.md
@@ -0,0 +1,281 @@
+# Test Coverage And Effectiveness Audit
+
+Reviewed revision: `eb5a82f1f3a3286506c2d527e7980db7ccae5398`.
+Audit date: 2026-10-25.
+
+## Assessment And Architecture
+
+The suite broadly follows the [testing policy](../policy/testing.md): real
+parsing, composition, decoding, and filesystem fixtures dominate; narrow doubles
+control I/O and environment failures. Engine tests own mechanisms and semantics,
+root tests protect public conversions and integration, and maintained consumers
+exercise representative workflows. The private document tree and YAML adapter
+support the synchronous engine behind the root-owned public facade; see the
+[component overview](../internal/overview.md).
+
+Strengths include presence/default/zero distinctions, strict composition versus
+precedence, collection policy, inheritance/history agreement, safe rendering and
+bilateral diff sensitivity, independent loads, reader claims, callback ordering,
+bootstrap partial failures, and realistic source/containment behavior. Parser and
+path fuzz targets have deterministic seeds in the ordinary suite. Architecture,
+documentation, and development-check tests protect explicit repository contracts.
+
+Six targeted findings follow: four coverage/assertion improvements and two areas
+for removal or consolidation. They do not justify a wholesale test purge, a
+percentage target, or another test for every private helper. No new production
+defect was established during this pass; the existing audits own their findings.
+
+## Coverage Evidence
+
+On Go 1.26.10, Linux amd64, the ordinary suite listed 771 `Test` functions, three
+fuzz targets, and 29 executable examples. These counts exclude Windows-only
+compiled tests and do not count table cases as separate test functions.
+
+Cross-package coverage, aggregated by source block across all test binaries, was:
+
+| Package | Covered statements | Coverage |
+| --- | ---: | ---: |
+| Root public package | 348 / 428 | 81.3% |
+| `internal/engine` | 9,089 / 9,987 | 91.0% |
+| `internal/document` | 13 / 13 | 100.0% |
+| `internal/yaml` | 151 / 168 | 89.9% |
+| Maintained consumer examples | 259 / 327 | 79.2% |
+| Total | 9,860 / 10,923 | 90.3% |
+
+The document package has no standalone tests, but its code is covered through
+real collaborators. The per-test-binary percentages printed by `-coverpkg=./...`
+have a different denominator and must not be mistaken for these aggregate
+package percentages. Statement coverage measures execution, not assertion
+strength or state-space coverage: the current suite passes despite the defects
+already recorded in the correctness, resource, error-handling, and security audits.
+
+Some zero-coverage private functions have no production caller; trivial public
+preset forwards are also unexecuted. Neither observation alone warrants adding
+tests. Prioritize the behavioral risks below over filling those percentages.
+
+## 1. Confirmed Exceptional Cases Lack Durable Regression Coverage
+
+Status: open. Priority: medium. Category: gaps in state transitions and boundaries.
+
+The earlier audits reproduced defects outside the checked-in suite. Relevant
+tests exercise nearby ordinary cases but do not yet protect these transitions:
+
+- **Schema and dynamic-value failure state.**
+  [schema_test.go](../../internal/engine/schema_test.go) covers ordinary recursive
+  structs; [projection_test.go](../../internal/engine/projection_test.go) checks
+  independent invalid values and cycles. Neither covers pointer-only recursive
+  descriptors, acyclic slice views sharing a starting pointer, or a second
+  dynamic/helper operation after a failed descriptor build in the same load.
+  Add the narrowly scoped regressions described by
+  [correctness findings 1–3](correctness.md), retaining null/acyclic success cases
+  and asserting safe classification and termination. Avoid an unbounded
+  stack-overflow/hang fixture in the default test process.
+- **Exceptional callback exit.** The retained-context tests in
+  [resolution_test.go](../../internal/engine/resolution_test.go) and
+  [facade_context_test.go](../../facade_context_test.go) verify normal callback
+  return. File-close tests cover ordinary load failures. They do not exercise
+  external recovery after version/resolution/validation callback panic or an
+  imported version check while a host root is owned. Extend the existing owners
+  when fixing [correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding)
+  and [resource finding 3](resources.md#3-an-imported-version-check-panic-leaves-the-host-root-open).
+  Preserve propagation of the original panic and verify closure of the callback
+  state and the actually owned root independently; a native file-close counter
+  alone cannot verify an `os.Root` lifetime.
+- **Budgets before allocation and through publication.**
+  `TestProjectionRejectsNonFiniteLimitsAndCodecFailuresSafely` checks a small
+  scalar tree against supplied limits. `TestHistoryNodeBudgetCountsSharedTreesOnce`
+  and the inheritance-history budget tests verify the accounting helper with
+  explicitly supplied roots. They cannot detect a container reservation before
+  charging, or roots omitted by the publication caller. Add bounded regressions
+  with [resource findings 1–2](resources.md): oversized zero-sized-element input
+  can avoid a huge input allocation, and a small controlled history allowance can
+  test the complete retained-root boundary. Keep count-once sharing coverage;
+  do not allocate millions of nodes just to cross the production default.
+- **Nested finite link expansion.**
+  [filesystem_links_test.go](../../internal/engine/filesystem_links_test.go) and
+  the public finite-reuse test cover simple reuse and actual cycles, but not the
+  compact repeated-dependency graph in [security finding 1](security.md#1-finite-symlink-reuse-can-cause-exponential-import-resolution-work).
+  When that remediation is selected, use a small real link fixture and a bounded
+  operation observer at the filesystem boundary. Protect finite reuse and
+  nonrecursive discovery without running an exhaustion workload or adding a
+  wall-clock performance threshold.
+
+These are test accompaniments to the original remediation owners, not duplicate
+product fixes or a request for a broad new cross-product matrix. The existing
+tests should remain where they protect distinct ordinary behavior.
+
+## 2. Public Historical-Path Enumeration Is Not Exercised
+
+Status: open. Priority: medium. Category: public facade integration.
+
+**Relevant code/tests.** `Metadata.HistoricalPaths` in
+[metadata.go](../../metadata.go), lines 49–56, has zero executed statements in
+the aggregate profile. Every checked-in `HistoricalPaths` test call is in the
+engine package. `TestMetadataHistoricalPaths` in
+[metadata_test.go](../../internal/engine/metadata_test.go) thoroughly covers the
+engine method, including removed descendants, escapes, contextual numeric
+tokens, and detached results, but does not cross the public conversion.
+
+**Risk.** Returning no paths or incorrectly converting the numeric segment intent
+at the root boundary could leave that engine suite green while breaking the
+documented consumer enumeration/query workflow. Coverage of `History` and
+`Lookup` separately does not exercise enumeration's result conversion.
+
+**Improvement.** Add one small external-package public-API test that enumerates
+effective and removed paths, then uses the returned paths with `Lookup` and
+`History`, including a numeric-key/sequence shape change. Check sorted unique
+pointer spelling, zero metadata, and replacement of a returned slice element
+without affecting later queries. Keep detailed path mutation and engine shape
+cases with their existing owner; do not duplicate the complete internal matrix
+or expose private segments to test the facade.
+
+## 3. Native-Filesystem Failure Tests Observe Too Little
+
+Status: open. Priority: medium. Category: doubles and inadequate assertions.
+
+**Relevant tests.** `TestNativeFilesystemClosesFilesOnSuccessAndFailure`,
+`observedFilesystem`, and `observedFile` in
+[native_filesystem_test.go](../../internal/engine/native_filesystem_test.go),
+especially lines 150–162 and 278–319. The fixture is also used by
+`TestLoadingClosesNativeFilesAcrossPipelineOutcomes` in
+[resource_context_test.go](../../internal/engine/resource_context_test.go).
+
+**Deficiencies and plausible survivors.**
+
+- Failure cases require only a non-nil error, not a nil result, the expected
+  diagnostic classification, or evidence identifying the intended failed
+  source. An unrelated early failure could satisfy the assertion.
+- The non-disclosure check searches only `err.Error()` for the fixture canary.
+  `LoadError.Error` deliberately renders an error count, not diagnostic messages.
+  Accidentally forwarding the raw filesystem error into `Diagnostics()` could
+  therefore pass this check. The parser/callback canary tests cover different
+  failure-producing boundaries.
+- `assertClosed` checks only total opens versus total `Close` calls. The counter
+  increments even for a repeated close, so one leaked handle and one double-close
+  can cancel out. It does not prove that each acquired handle was released.
+- The double supplies immediate generic read/directory errors and successful
+  closes. It cannot model entries/data plus an error, or a close error whose
+  identity resembles absence. This leaves the
+  [directory failure-phase defect](error-handling.md#1-directory-read-and-close-failures-can-be-misclassified-as-absence)
+  unprotected on both the batched and fallback paths.
+
+**Improvement.** Keep the real in-memory filesystem backing and small controlled
+failure seams. Track each returned handle's close state, assert the relevant
+structured load error and nil result, and inspect diagnostics/rendered diagnostics
+for the canary. Add only the read/close phase cases needed by the existing
+error-handling remediation, including a second successful directory so required
+selection cannot mask the failed one. Preserve intentionally ignored read-only
+regular-file close errors; do not invent a universal close-error contract.
+
+## 4. Fuzz Detachment Assertions Miss Shared Descendants
+
+Status: open. Priority: low. Category: ineffective ownership assertion.
+
+**Relevant target.** `FuzzCompositionAndOverlay` in
+[composition_fuzz_test.go](../../internal/engine/composition_fuzz_test.go),
+lines 46–49 and 63–66, appends a new root mapping entry and then compares inputs
+against their earlier copies to establish result detachment.
+
+**Deficiency.** A shallow-copied root can still share existing entries, descendant
+nodes, or sequence backing storage. Appending through the copied root's slice
+header changes neither the input's header nor its existing elements; even reuse
+of the backing array writes beyond the input's old length. The comparison can
+pass while the result still shares mutable descendants. It primarily detects
+aliasing the root object itself, not the deeper ownership property in its error
+message.
+
+**Improvement.** Within the existing bounded target, mutate an existing result
+descendant or an element inside the original slice length, then check both
+inputs. Keep empty-container cases separate and retain the existing input
+immutability and commutativity properties. Deterministic tests in
+[compose_test.go](../../internal/engine/compose_test.go),
+[overlay_test.go](../../internal/engine/overlay_test.go), and
+[recursive_merge_test.go](../../internal/engine/recursive_merge_test.go) already
+give meaningful nested-mutation protection; improve the fuzz assertion rather
+than adding a parallel matrix or claiming all ownership coverage is absent.
+
+## 5. Remove Exact Stat-Call Choreography From The Ordering Test
+
+Status: open. Priority: low. Category: removal/rewrite candidate.
+
+`TestSelectNamedImportsChecksNormalizedPathsLexically` in
+[import_selection_test.go](../../internal/engine/import_selection_test.go),
+lines 151–166, asserts the exact trace `a.yaml,a.yaml,z.yaml,z.yaml`.
+
+Lexically ordered target handling is meaningful. Performing exactly two `Stat`
+calls per target is not an established requirement: reusing successful preflight
+metadata or changing a filesystem helper could preserve selection, safe failure,
+and ordering while failing this test. The selected-candidate assertion meanwhile
+checks only the count, not the order or paths.
+
+Remove the duplicated-stat trace expectation. Check ordered candidate paths and,
+if first-failure order needs separate protection, the selected diagnostic site
+using two failing targets. Consolidate that case into the existing selection
+tests and delete this standalone test if its distinct protection is then
+subsumed. Keep meaningful interaction assertions such as not opening excluded
+file contents; those protect an actual resource/selection behavior.
+
+## 6. Consolidate Redundant And Representation-Coupled Facade Scaffolding
+
+Status: open. Priority: low. Category: removal/consolidation candidates.
+
+- In [facade_compatibility_test.go](../../facade_compatibility_test.go), the
+  six-type package-ownership loop in `TestFacadeCompatibility` is a subset of
+  the comprehensive `TestFacadeValuesRemainRootOwned` loop in
+  [facade_candidate_test.go](../../facade_candidate_test.go). Remove the duplicate
+  loop while retaining the compile-time interface checks, public construction,
+  and any distinct lifecycle behavior. The runtime assertion that a freshly
+  constructed `Result.Config` still equals its literal field value adds no
+  protection beyond ordinary Go assignment and compilation.
+- `TestWrappedPathsRetainEngineSegmentIntent` in
+  [facade_internal_test.go](../../facade_internal_test.go) compares private
+  `.value` representations for inequality. Distinct representations are not the
+  consumer contract; correct contextual mapping/index lookup is. Replace its
+  protection with public query behavior, coordinated with finding 2, then remove
+  the private-representation test. Keep the engine's own segment-intent tests.
+- In that same file, `TestWrappedSourcesPreserveEngineOwnership` invokes
+  `engine.Load` directly through `source.toEngine()`. Its byte snapshot and
+  copied-reader risks are legitimate, but the facade test should use public
+  `Load` and root-owned errors. Consolidate overlapping reader behavior with
+  `TestRepeatedImportsAcrossTiersAndReaderConsumption` in
+  [facade_equivalence_test.go](../../facade_equivalence_test.go), preserving
+  explicit copied-source and byte-mutation coverage before deleting scaffolding.
+- The clone portion of `TestSourceInfoDestinationConversionsAndCopies` in
+  [records_test.go](../../records_test.go), lines 39–42, replaces the whole copied
+  `Destination` field. A shallow struct copy passes this without proving nested
+  detachment. Remove that non-protective assertion or consolidate the relevant
+  returned-record ownership check into existing public source-snapshot tests;
+  retain conversion checks that protect fields at the facade boundary. `Path`
+  is opaque and immutable through its public builders, so exposing internals or
+  adding a new deep-copy guarantee is not justified by this test.
+
+This is a small cleanup, not a recommendation to remove root integration tests
+because the engine has tests. Public conversion, error typing, callback bridging,
+source namespace identity, disclosure, and returned-record ownership have real
+cross-package risks. The static public/dependency-boundary checks also protect
+the expressly selected architecture and should remain.
+
+## Validation And Limits Of This Review
+
+The following passed using cached dependencies with automatic network/toolchain
+acquisition disabled:
+
+```sh
+go test ./... -coverpkg=./... -covermode=atomic -coverprofile=/tmp/configkit-testing-audit.ZNsVuW/coverage.out -count=1
+go tool cover -func=/tmp/configkit-testing-audit.ZNsVuW/coverage.out
+go test ./... -shuffle=on -count=3
+```
+
+The profile stayed outside the repository. Coverage blocks were combined across
+test binaries before calculating package totals. A separate JSON test run found
+no individual skipped test/subtest on this Linux host; `internal/document` was
+reported as having no standalone tests. This is not native macOS/Windows evidence.
+Only the checked-in fuzz seeds were run; no extended fuzz campaign or mutation
+testing was performed. Assertion weaknesses above are grounded in source-level
+counterexamples, not a claimed measured mutation score.
+
+No production code, tests, fixtures, dependencies, or release gates were changed.
+Recommended additions should accompany the relevant fixes and existing test
+owners. Use small controlled budgets and independent outcomes, retain selected
+stable formatting/code/path contracts, and avoid making implementation details
+or speculative guarantees into new policy through assertions.