From b1c278b38bead1fceeb80b99d2f336b0cb6fe2fd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 17:07:39 +0000 Subject: [PATCH] Record test coverage and effectiveness audit findings --- docs/roadmap/testing.md | 281 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 docs/roadmap/testing.md diff --git a/docs/roadmap/testing.md b/docs/roadmap/testing.md new file mode 100644 index 0000000..f7091fc --- /dev/null +++ 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.