diff --git a/docs/roadmap/audit/08-testing.md b/docs/roadmap/audit/08-testing.md new file mode 100644 index 0000000..cc1b819 --- /dev/null +++ b/docs/roadmap/audit/08-testing.md @@ -0,0 +1,284 @@ +# Test Coverage And Effectiveness Audit + +Reviewed 2026-11-04 against commit `f7085d7` and the current worktree. Existing +audit-document moves were preserved. No production code or tests were changed. + +## Assessment And Method + +The suite broadly follows the [testing policy](../../policy/testing.md): real +ConfigKit composition, filesystems, rendering, validation, and engine assembly +are exercised alongside controlled model clients and HTTP transports. Internal +tests own focused rules and coordination, external-package root tests protect +the public facade, and the three maintained examples have Go workflow tests. +Capacity, prepared-handle state transitions, ownership, cancellation, retries, +repair, observations, and configuration inspection have substantial behavioral +coverage. This is a targeted improvement list, not a recommendation to replace +the test architecture or pursue a percentage threshold. + +Both required analyses were performed: review of missing behavioral protection, +and review of assertion/fixture cost and coupling. Evidence includes source and +call-graph review, existing GoDoc and integration contracts, full-suite coverage, +repeated race-enabled execution, and reuse of the earlier error audit's temporary +offline diagnostic. No live provider, paid API, new repository test, production +mutation, or external mutation-testing service was needed. + +Validation used the local Go 1.27.0 toolchain: + +```sh +go test -count=1 -covermode=atomic -coverpkg=./... -coverprofile=/tmp/promptkit-testing-audit-LHAyNm/coverage.out ./... +go tool cover -func=/tmp/promptkit-testing-audit-LHAyNm/coverage.out +go test -race -vet=off -shuffle=42 -count=2 ./... +``` + +Both suite runs passed, including the examples. The race run exercised two +shuffled passes; this is evidence against readily reproducible order/race +problems, not proof that no flakes exist. Coverage output remains outside the +repository. The commands are audit evidence, not additional release gates. + +Combined coverage is **86.4%**, or 4,782 of 5,536 instrumented statements. +Selected package totals below combine hits across all test binaries. The +per-binary percentages printed by `-coverpkg=./...` are not individual-package +coverage percentages; repeated profile spans were combined before calculating +these totals. + +| Component | Combined statement coverage | +| --- | ---: | +| Public facade | 81.0% | +| Configuration | 91.0% | +| Domain | 98.0% | +| Capacity | 98.0% | +| Generation coordination | 91.2% | +| HTTP model client | 94.2% | +| Execution use cases | 90.5% | +| Rendering and requirement inspection | 78.4% | +| Output/schema validation | 75.8% | + +Coverage located candidates; the findings below concern specific risks and +assertions, not the percentages themselves. Testing recommendations associated +with already recorded implementation defects should accompany those fixes, +rather than becoming duplicate remediation projects. + +## 1. RunRequest's Privacy Formatting Contract Is Not Exercised + +**Priority:** P2. Missing protection for a documented confidentiality boundary. + +**Relevant code:** [RunRequest formatting](../../../formatting.go:5) and the +analogous existing [SmokeTestRequest privacy test](../../../smoke_test_contract_test.go:135). + +**Deficiency:** `RunRequest.String`, `GoString`, and their shared formatter all +have 0% combined statement coverage. The suite protects formatting of generated +client requests, smoke-test requests, and opaque prepared handles, but does not +exercise the ordinary run request's own formatting contract. + +**Unverified behavior:** A change that prints the direct key, input bodies, or +variable values from `RunRequest` can pass the complete suite. This is an +assertion gap, not evidence that current formatting actually leaks those values. + +**Potential improvement:** Add one small public-boundary test with distinct +synthetic private markers and exercise the supported String/GoString formatting +paths on values and pointers. Assert absence of the data covered by the GoDoc; +do not snapshot the complete summary or freeze incidental field order/count +formatting. If adding a JSON check, assert only the documented direct-key +exclusion, not a stable JSON shape or omission of ordinary request data. +Reflection-based disclosure is explicitly outside this formatter guarantee. + +## 2. The Public Single-Schema-File Option Has No Execution Coverage + +**Priority:** P2. An exported source-selection path is entirely unprotected. + +**Relevant code:** [WithSchemaFile](../../../engine.go:212), +[SetSchemaFile](../../../internal/engine/engine.go:75), and +[fileSource](../../../internal/engine/engine.go:211). + +**Deficiency:** All three functions have 0% combined coverage. The existing +[single-file validator test](../../../internal/validate/standard_validator_test.go:596) +constructs `FSValidator` directly with `fstest.MapFS`; it does not test the public +option, host-file selection, or its engine wiring. + +**Unverified behavior:** The public option could ignore its argument, fail to +replace `SchemaDir`, select the wrong root/base name, or mishandle invalid files +without this validator test noticing. Direct validator success does not establish +that a consumer can reach that behavior through the supported API. + +**Potential improvement:** Use `t.TempDir` and a real schema file in a small +public integration test. Exercise construction plus preparation/validation +through that file's base name, and representative invalid selections such as a +missing file or directory. Include source-option replacement where it protects +the documented last-valid-option behavior. Assert public error identity and +observable schema selection, not private helper calls. This does not require a +helper-by-helper test matrix or model-provider access. + +## 3. Programmatic Provider-Value Conversion Lacks Representative Value Cases + +**Priority:** P2. Conversion and ownership coverage misses known data-integrity +and deferred-error risks. + +**Relevant tests:** [programmatic source ownership/order](../../../sources_contract_test.go:14) +and [programmatic construction](../../../construction_contract_test.go:226). +**Implementation:** [providerValueForMapping](../../../provider_values.go:31). + +**Deficiency:** The converter has only 11.9% combined coverage. The principal +source-ownership test mutates a flat string-valued map. It does not establish +preservation and ownership for representative nested/typed containers or the +`json.Number` branch, nor deferred failure for unsupported provider values. +The more extensive `internal/jsonvalue` tests protect a different traversal and +do not execute this pre-ConfigKit convenience conversion. + +**Unverified behavior:** The suite passes with the exact-number rounding defect +already recorded in [the correctness audit](01-correctness.md#1-provider-source-helpers-silently-change-exact-json-numbers). +It also would not detect many regressions in nested source capture or the +documented source-creation-versus-loading failure boundary. + +**Potential improvement:** Exercise both `ProfileSource` and `BackendSource` +through `NewEngine` with a compact set of representative supported nested +values, mutations after source creation, and unsupported/cyclic values that +should fail during loading. Add the numeric regression when addressing the +existing precision finding. Compare meaningful final values or encoding, not Go +container identity, numeric spelling, or a required float representation. +Keep tests at the final handoff/public source-helper boundary; do not duplicate +ConfigKit's raw-source validation or its entire value-limit suite. Work-bound +coverage for the separately identified shared-graph issue belongs with that fix. + +## 4. Error Tests Miss Composed Causes And Pre-Dispatch Diagnostic Failures + +**Priority:** P2. Covered code paths lack important boundary-case assertions. + +**Relevant tests:** [public generation failures](../../../generation_error_contract_test.go:111), +[direct mapper cases](../../../errors_internal_test.go:13), +[body cancellation](../../../internal/llm/prepared_generation_test.go:161), and +[inference diagnostic cases](../../../internal/usecase/smoke_inference_test.go:14). + +**Deficiency and demonstrated blind spots:** + +- Injected failures are mostly plain marker errors or a single explicit + category. They do not check a real nested PromptKit error returned through a + different operation. The suite passes although an injected client/reader can + lose the outer public generation/artifact category. +- Tests cover `http.Client.Do` failures and cancellation during successful-body + reading, but not an ordinary non-EOF read error during initial decoding or + final EOF verification. Such errors currently lose their cause and become + malformed-response failures. +- Diagnostic tests cover invalid selection/settings and concluded generation + failures separately, but miss a runtime setting rejected by built-in request + construction before dispatch. A reserved `ExtraParams["model"]` override + currently returns a completed report with nil Go error. + +These are the three implementation findings in the error-handling audit, not +new duplicate defects. Re-running its temporary offline public-API diagnostic +against the current tree confirmed all three while the complete suite passed. + +**Potential improvement:** Add narrowly owned regressions with those fixes: +real nested public causes through the injection boundary; a controlled body +reader failing at each decode/framing phase; and a public `SmokeTest` call with +a reserved runtime field and counted transport. Assert relevant error identities, +cause preservation, factual failure classification, and whether dispatch occurred. +Avoid reproducing the internal error topology by hand or matching complete +messages. Preserve intentional nil-error diagnostic findings, cancellation +precedence, best-effort non-2xx detail parsing, and status-only retry policy. + +## 5. Multi-Resource Schema Tests Do Not Establish A Usable Provider Constraint + +**Priority:** P2. Missing assembled validation-to-wire assertion. + +**Relevant tests:** [prepared execution source freezing](../../../prepared_execution_contract_test.go:19), +its [referenced schema fixture](../../../prepared_execution_contract_test.go:809), +and [local reference validation](../../../internal/validate/standard_validator_test.go:627). + +**Deficiency:** Local tests establish that referenced schemas compile and +validate. The prepared-execution test checks that the injected client's schema +matches frozen details. Its opaque model client accepts that schema without +interpreting references. Equality between those two values does not establish +that the provider-facing constraint contains the resources it needs. + +**Unverified behavior:** Both sides can contain the same unresolved `value.json` +reference while the test passes. This is the packaging gap already identified +in [the correctness audit](01-correctness.md#2-external-schema-resources-do-not-reach-the-provider-constraint). +The freezing test's fake is appropriate for its stated ownership purpose; the +missing protection is a separate built-in-path integration case. + +**Potential improvement:** Once the schema-packaging policy is selected, add one +offline assembled test with a root and child schema, capture the actual built-in +HTTP payload, and assess the chosen provider-facing representation independently +of `Details`. For a bundled native schema, compile/evaluate the captured +constraint without granting access to PromptKit's original local files; if the +chosen policy supports a local-only fallback, assert that behavior instead. +Keep the existing freezing test. Do not require live provider acceptance or +promise support for arbitrary provider schema dialect features. + +## 6. Artifact Tests And Benchmarks Pin An Explicitly Opaque Hash Algorithm + +**Priority:** P3. Remove or rewrite change-detector assertions, retaining the +substantive file-reading tests. + +**Relevant assertions:** [size-hint test](../../../internal/artifact/reader_test.go:287) +and [benchmark correctness check](../../../internal/artifact/reader_benchmark_test.go:156). + +**Deficiency:** Both independently calculate a hex SHA-256 digest and require +that exact value. The [public Artifact.Hash contract](../../../types.go:290) +expressly leaves its format and algorithm unspecified. A legitimate algorithm +or encoding change would fail tests unrelated to the requested behavior. + +**Potential improvement:** Replace those algorithm-specific checks with the +opaque equality relationships needed for each test. For the under/overreported +size cases, comparing with the inline reader for the same complete body still +detects hashing only a reported-size prefix. The existing +[source-parity test](../../../internal/artifact/reader_test.go:27) already owns +repeatability, same-content equality, and changed-content inequality. Retain +body, actual-size, metadata, and read-boundary assertions. The benchmark may +reuse an opaque expected hash prepared outside measurement or rely on the +owning behavioral test; it need not maintain its own SHA-256 contract. + +## 7. Bundled Catalog Expectations Have Multiple Test Owners + +**Priority:** P3. Consolidation/removal candidate for duplicated assertions. + +**Relevant tests:** [internal complete catalog table](../../../internal/configuration/bundled_test.go:12), +[public option/inventory table](../../../bundled_openrouter_contract_test.go:54), +[HTTP payload matrix](../../../bundled_execution_contract_test.go:51), and +[bundled smoke defaults](../../../bundled_smoke_contract_test.go:16). + +**Deficiency:** The current count of 17, complete ID inventory, selected +ID-to-model mappings, tier-plugin data, and diagnostic default model recur +across configuration, facade, wire, and diagnostic tests. A catalog update can +require expectation edits in several layers even when loading, option handling, +routing, and serialization are unchanged. The internal plugin assertion also +searches `fmt.Sprint` output instead of comparing the plugin's structure. + +**Potential improvement:** Choose one owner for the complete maintained asset +inventory and expected model/plugin/default data. Remove duplicate full-catalog +counts/lists from option permutations and keep those tests focused on enable, +disable, last-option selection, isolation, and override behavior. Retain a small +assembled wire/diagnostic set for distinct integration risks; controlled +overrides can isolate routing assertions from a changing catalog default. +Use semantic structural assertions for plugin data. Keep independent expected +asset values in the selected owner rather than deriving every expectation +from the production loader, which could conceal an omitted or incorrect asset. +This is consolidation, not a proposal to stop checking bundled configuration. + +## Findings Not Supported And Work To Preserve + +- No general test-flakiness, default-suite live-provider dependency, or race + failure was reproduced. Bounded timeout guards and channel synchronization + are not classified as flaky merely because a test involves concurrency. +- The public API/type-boundary checks and their checker fixtures protect an + explicit architectural rule; they are not automatically obsolete because + they inspect structure. Stable JSON shape tests protect selected contracts. +- Fakes at model/HTTP boundaries, counted paid-effect calls, and no-work-after- + rejection checks are appropriate. A fake is not deficient merely because it + does not model unrelated provider behavior. +- Exhaustive domain-rule tables plus representative boundary wiring tests are + generally sound. No wholesale deletion of error, cancellation, capacity, + ownership, or prepared-lifecycle coverage is recommended. +- Low coverage of thin wrappers, example `main` functions, defensive impossible + branches, and unexercised reflection variants is not independently a finding. + Interface-only packages do not need artificial tests to obtain a package + percentage. Conservative requirement inspection is not obliged to fully + evaluate unsupported template expressions just to cover every AST branch. +- The standalone release-wiring fixture remains scoped to local disposable + repositories under the documented testing model. This audit does not propose + moving it into every normal test run or retesting ReleaseKit's algorithms. + +Prioritize the missing public privacy/source-option protection and regressions +for the already identified defects. Make the two assertion-cleanup changes as +focused rewrites, retaining their useful behavioral coverage. None of these +recommendations requires new public guarantees or a coverage-percentage gate.