282 lines
16 KiB
Markdown
282 lines
16 KiB
Markdown
# 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.
|