Document repository-wide test strategy audit
This commit is contained in:
233
audit.md
233
audit.md
@@ -3853,3 +3853,236 @@ go run ./examples/go-library/prepare
|
|||||||
Any future change to fairness or allocation strategy should first preserve
|
Any future change to fairness or allocation strategy should first preserve
|
||||||
the current linearization points and be justified by representative
|
the current linearization points and be justified by representative
|
||||||
contention measurements.
|
contention measurements.
|
||||||
|
|
||||||
|
## Stage 16: Repository-Wide Test Strategy And Maintained Examples
|
||||||
|
|
||||||
|
### Scope Reviewed
|
||||||
|
|
||||||
|
The suite-level review covered all 25 `_test.go` files and their 281 top-level
|
||||||
|
tests, every package-local and repository-level fixture, the external-package
|
||||||
|
root contracts, `architecture_test.go`, and both maintained programs and prompt
|
||||||
|
files under `examples/go-library`. The testing policy supplied the
|
||||||
|
risk-oriented sufficiency standard. The architecture, documentation, consumer,
|
||||||
|
format, release, and internal-overview documents supplied the repository and
|
||||||
|
example boundaries. Component behavior was not reopened: the Stage 1 through
|
||||||
|
15 coverage ledgers were used to assign owners and identify already accepted
|
||||||
|
gaps.
|
||||||
|
|
||||||
|
The refreshed code graph mapped test symbols and `TESTS` edges across the
|
||||||
|
public facade and internal packages. Source inspection then checked the suite
|
||||||
|
for exact error strings, private-helper and constant coupling, fixed ports,
|
||||||
|
live dependencies, environment and filesystem state, timers, sleeps, fixture
|
||||||
|
mutation, golden updates, and helper complexity. Coverage and timing directed
|
||||||
|
attention only; findings below are based on the maintained workflow contract
|
||||||
|
and one reproduced environment-dependent failure.
|
||||||
|
|
||||||
|
### Accepted Findings
|
||||||
|
|
||||||
|
#### S16-F01: Maintainer validation never executes the maintained Run example
|
||||||
|
|
||||||
|
- **Category:** testing
|
||||||
|
- **Severity:** low
|
||||||
|
- **Confidence:** confirmed
|
||||||
|
- **Status:** accepted
|
||||||
|
- **Affected code:** `docs/policy/testing.md` and `docs/release.md` (maintainer
|
||||||
|
validation commands), `docs/development.md` (the assigned contributor-command
|
||||||
|
owner), and `examples/go-library/run/main.go` plus its `prompt.yaml`
|
||||||
|
- **Contract at issue:** Both Go-library examples are maintained, deterministic,
|
||||||
|
offline downstream workflows. The execution example is intentionally
|
||||||
|
separate because it protects assembled `Run`, injected-client, validation,
|
||||||
|
usage, and result behavior that the preparation example does not exercise.
|
||||||
|
Maintainer validation should execute each distinct maintained consumer
|
||||||
|
artifact whose fixture is otherwise only compiled or ignored.
|
||||||
|
- **Evidence:** The internal overview and consumer guide name both examples as
|
||||||
|
maintained offline workflows, but the testing-policy and release command
|
||||||
|
lists invoke only `go run ./examples/go-library/prepare`. `go test ./...`
|
||||||
|
reports both example packages as having no test files; diagnostic coverage
|
||||||
|
reports 0.0% for each, so it compiles `run/main.go` but never opens
|
||||||
|
`run/prompt.yaml` or invokes the engine. Directly running
|
||||||
|
`go run ./examples/go-library/run` from the repository root completed in
|
||||||
|
under a second and returned the documented deterministic result. The
|
||||||
|
preparation command also passed, confirming that the two programs are
|
||||||
|
independently runnable rather than alternate entry points to one fixture.
|
||||||
|
- **Failure mode:** The execution example's relative path, prompt format,
|
||||||
|
in-memory profile, injected-client adaptation, runtime validation, output
|
||||||
|
projection, or fixture can drift while every required maintainer command
|
||||||
|
still passes. A consumer copying the advertised complete example discovers
|
||||||
|
the breakage after release. Root contracts continue to protect library
|
||||||
|
behavior, but they do not execute this maintained artifact or its file.
|
||||||
|
- **Recommended direction:** Add the execution example to the canonical local
|
||||||
|
maintainer-validation workflow and synchronize documents that currently
|
||||||
|
present that workflow without creating a second independent command owner.
|
||||||
|
Keep both examples separate: their preparation and execution outcomes are
|
||||||
|
distinct and their small duplicated prompt fixtures make each program
|
||||||
|
independently copyable.
|
||||||
|
- **Required verification:** Run both example commands from the repository
|
||||||
|
root without credentials or network access, require successful exit and
|
||||||
|
stable semantic fields for their respective prepared and executed outputs,
|
||||||
|
and demonstrate that an invalid or missing execution prompt fixture makes
|
||||||
|
the maintained validation fail.
|
||||||
|
|
||||||
|
#### S16-F02: A transport test depends on fixed port 9999 being unused
|
||||||
|
|
||||||
|
- **Category:** testing
|
||||||
|
- **Severity:** low
|
||||||
|
- **Confidence:** confirmed
|
||||||
|
- **Status:** accepted
|
||||||
|
- **Affected code:** `internal/llm/openai_compatible_client_test.go`
|
||||||
|
(`TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL`)
|
||||||
|
- **Contract at issue:** Default tests must be deterministic, offline,
|
||||||
|
independent of live infrastructure, and free of fixed-port assumptions. A
|
||||||
|
test of configured-versus-request endpoint selection should control its
|
||||||
|
transport outcome rather than infer selection from an environmental
|
||||||
|
connection refusal.
|
||||||
|
- **Evidence:** The test sends a real HTTP request to
|
||||||
|
`http://localhost:9999/v1` and requires `ErrRequestFailed`, assuming nothing
|
||||||
|
is listening. With a temporary standard-library HTTP server bound to
|
||||||
|
`127.0.0.1:9999`, the focused test failed immediately: the selected endpoint
|
||||||
|
returned status 501, producing `ErrNonSuccessStatus` instead of the required
|
||||||
|
connection error. The listener was then stopped and the ordinary suite
|
||||||
|
passed. Search across all tests found no other contacted fixed port;
|
||||||
|
`localhost:8000` occurrences are inert fixture values or use injected
|
||||||
|
clients.
|
||||||
|
- **Failure mode:** A developer, build host, or shared runner with any service
|
||||||
|
on port 9999 gets a false test failure whose error category depends on that
|
||||||
|
unrelated service. The test also performs a real socket operation even
|
||||||
|
though both constructor acceptance and endpoint-override selection can be
|
||||||
|
established deterministically.
|
||||||
|
- **Recommended direction:** Supply a controlled `httptest` endpoint or
|
||||||
|
injected round tripper that records the selected URL and returns a deliberate
|
||||||
|
transport result. Retain separate assertions that an empty configured base
|
||||||
|
is valid and that the request endpoint takes precedence; do not preserve
|
||||||
|
connection-refusal behavior as a contract.
|
||||||
|
- **Required verification:** Run the focused case normally, repeatedly, and
|
||||||
|
while an unrelated listener occupies port 9999. It must make no request to
|
||||||
|
that listener, must prove that the target endpoint was selected, and must
|
||||||
|
retain the intended constructor and error identities without relying on the
|
||||||
|
host network state.
|
||||||
|
|
||||||
|
### Unresolved Observations
|
||||||
|
|
||||||
|
None. The documentation policy assigns local validation commands to the
|
||||||
|
development guide, while that guide currently has no command list and the
|
||||||
|
release procedure says its own list comes from the guide. Comparing and
|
||||||
|
baselining those three command owners is explicitly Stage 0 work, which remains
|
||||||
|
absent; this stage did not backfill that broader audit or promote a duplicate
|
||||||
|
finding beyond S16-F01's concrete missing workflow.
|
||||||
|
|
||||||
|
Large files were not treated as defects by size. `engine_test.go`,
|
||||||
|
`internal/usecase/runner_test.go`, and the external prepared contracts organize
|
||||||
|
many distinct public or orchestration behaviors, while S14-F06 already records
|
||||||
|
the one confirmed case where repeated transport scaffolding obscures its
|
||||||
|
behavior matrix. The 25 ms prepared-timing sleep is unnecessary for ordering
|
||||||
|
but passed shuffled repetition and race runs, adds negligible suite time, and
|
||||||
|
does not establish a consequential flaky boundary by itself.
|
||||||
|
|
||||||
|
### Risk-To-Owner Matrix
|
||||||
|
|
||||||
|
| Risk | Credible current owner | Accepted gap or retained protection |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| Public compatibility and error identity | External-package root tests in `engine_test.go`, `public_contract_test.go`, `prepared_execution_contract_test.go`, and `capacity_contract_test.go`; focused internal mapping tests own internal-type containment. | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, and S13-F01 already identify the consequential missing public cases. Other internal/root overlap asserts different stable boundaries and is retained. |
|
||||||
|
| Parsing, validation, and serialization | `internal/promptdef`, `internal/profile`, `internal/filecatalog`, `internal/jsonvalue`, `internal/validate`, and domain tests own focused rules; root corpus and JSON contracts own assembled public compatibility. | S05-F04 and S07-F06 own material missing rule coverage. The reproduced parser, profile, schema, and numeric defects already specify regression cases in their component findings; no second suite-level test owner is needed. |
|
||||||
|
| Immutability and data integrity | Domain and JSON-value tests own shared transforms; backend, repository, use-case, prepared, and root tests own snapshots at each ownership transfer. | S02-F03 owns injected-client mutation isolation. S05-F03 owns unbounded tree work. S05-F05 identifies unused internal prepared JSON tests whose stable public serialization remains protected by external root contracts. |
|
||||||
|
| External wire behavior | `internal/llm` owns realistic `httptest` request/response behavior; root tests own resolved settings reaching the built-in client and public error mapping. | S14-F01 through S14-F05 own missing cancellation, timeout, URL, size, and framing boundaries. S14-F06 retains protocol assertions while consolidating repeated scaffolding. S16-F02 is the only fixed-port/live-environment case. |
|
||||||
|
| Cancellation, failure propagation, and recovery | Artifact, validator, model-client, capacity, use-case, prepared-lifecycle, and root tests each own failures at their narrow boundary; root tests own public identities and partial-result rules. | S04-F01, S09-F02, S10-F04, and S14-F01 identify missing or defective cancellation paths. Existing failure tests use `errors.Is`/`errors.As`; error-text checks are limited to required path association, redaction, or diagnostic fragments. |
|
||||||
|
| Concurrency and resource lifecycle | Capacity package and public capacity contracts own relational limits and FIFO release; prepared use-case and public contracts own claim/discard races; race-enabled validation owns frozen-plan safety. | Stage 15 found no scheduling defect or duplicate owner. S05-F03 and S14-F04 own the two unbounded resource paths. Shuffled repeated race runs found no shared-state or ordering failure. |
|
||||||
|
| Representative assembled consumer workflows | Root external tests own broad assembled behavior; the framework corpus owns a realistic prepare fixture; the two examples own independently copyable offline preparation and execution programs. | S16-F01 records that only the preparation example is executed by maintainer validation. The examples' small parallel fixtures are retained because each program demonstrates a distinct workflow and remains standalone. |
|
||||||
|
|
||||||
|
### Suite And Fixture Ledger
|
||||||
|
|
||||||
|
- **Inventory and boundaries:** The graph and source inventory found 25 test
|
||||||
|
files with 281 top-level tests. Five root files use package
|
||||||
|
`promptkit_test` for public contracts, while the two small root internal
|
||||||
|
files directly own adapter copying and error translation unavailable through
|
||||||
|
an external surface. Internal packages test their own boundaries. No test
|
||||||
|
imports a consumer, and the AST-based architecture guard recursively rejects
|
||||||
|
imports of the former consumer module while a focused self-test proves that
|
||||||
|
its detector finds nested imports.
|
||||||
|
- **Fixtures and secrets:** Parser and profile fixtures are minimal malformed
|
||||||
|
and valid YAML cases; schema and prompt content use synthetic values. The
|
||||||
|
framework corpus is a shared root integration fixture rather than a second
|
||||||
|
parser truth table. Tests use `t.TempDir`, temporary directories, `fstest`,
|
||||||
|
and `httptest` where mutable or external behavior matters. Environment
|
||||||
|
credentials are synthetic and controlled with `t.Setenv` or deliberate
|
||||||
|
execution-time removal; no real secret or mutable external service is
|
||||||
|
required. S16-F02 is the sole contacted fixed port.
|
||||||
|
- **Assertions and coupling:** Public and collaborator errors are normally
|
||||||
|
asserted with `errors.Is` or `errors.As`. Remaining text assertions select
|
||||||
|
structural paths, schema/compiler causes, status codes, redaction sentinels,
|
||||||
|
or the smallest diagnostic fragment needed to distinguish a case; none
|
||||||
|
snapshots a complete noncontractual message. Exact collaborator call counts
|
||||||
|
in runner and prepared tests protect ordering, bounded generation, no partial
|
||||||
|
work, and one-attempt lifecycle requirements rather than private call graphs.
|
||||||
|
No golden files or automatic update mode exist.
|
||||||
|
- **Consolidation:** Component ledgers found intentional relational overlap
|
||||||
|
between package, use-case, and public boundaries. The accepted deletion or
|
||||||
|
consolidation candidates already state their remaining protection:
|
||||||
|
S05-F05 can remove unused internal prepared serialization cases while root
|
||||||
|
JSON contracts remain; S09-F05 can stop pinning a hash algorithm while
|
||||||
|
retaining determinism and mutation sensitivity; and S14-F06 can share HTTP
|
||||||
|
setup while retaining every wire-visible assertion. No additional
|
||||||
|
evidence-backed deletion emerged at suite level.
|
||||||
|
- **Determinism and cost:** Twenty shuffled ordinary runs and three shuffled
|
||||||
|
race-enabled runs passed across every package. No test-order, global-state,
|
||||||
|
data-race, deadlock, or leaked-waiter symptom appeared. A diagnostic timing
|
||||||
|
run found no individual test above 50 ms in that sample and no package pass
|
||||||
|
event above 205 ms. Package coverage ranged from 69.2% to 100% among
|
||||||
|
packages containing substantive tests; the lower percentages align with
|
||||||
|
the already recorded JSON-value and validation decisions and were not used
|
||||||
|
as independent findings. `internal/defaults`
|
||||||
|
remains untested because its trivial baseline is exercised relationally by
|
||||||
|
higher owners rather than pinned as private constants.
|
||||||
|
- **Maintained examples:** Preparation resolves and renders without a model;
|
||||||
|
execution injects a deterministic client and validates a result. Preparation
|
||||||
|
never calls its configured endpoint, and execution's injected client bypasses
|
||||||
|
its endpoint, so both remain offline and credential-free.
|
||||||
|
Their similar prompt files are intentionally local so either complete
|
||||||
|
program can be copied without a hidden cross-example dependency. Both ran
|
||||||
|
successfully during this review; only validation ownership is deficient as
|
||||||
|
S16-F01 records.
|
||||||
|
|
||||||
|
### Verification Performed
|
||||||
|
|
||||||
|
The code knowledge graph inventoried test files and symbols, associated tests
|
||||||
|
with production hotspots, and confirmed the external and internal package
|
||||||
|
boundaries. Source and the prior component ledgers supplied the behavioral
|
||||||
|
ownership analysis. Diagnostic coverage and timing were used only to direct
|
||||||
|
the review:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
go test -cover ./... -count=1
|
||||||
|
go test -json ./... -count=1
|
||||||
|
```
|
||||||
|
|
||||||
|
The suite passed shuffled repetition, shuffled race execution, vet, build, Go
|
||||||
|
formatting, and both offline example runs:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
go test ./... -shuffle=on -count=20
|
||||||
|
go test -race ./... -shuffle=on -count=3
|
||||||
|
go vet ./...
|
||||||
|
go build ./...
|
||||||
|
go run ./examples/go-library/prepare
|
||||||
|
go run ./examples/go-library/run
|
||||||
|
```
|
||||||
|
|
||||||
|
A temporary Python standard-library server was bound to `127.0.0.1:9999` only
|
||||||
|
for the focused probe. With it active, this command failed with status 501
|
||||||
|
instead of the expected `ErrRequestFailed`, confirming S16-F02; the server was
|
||||||
|
stopped immediately afterward:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
go test ./internal/llm -run '^TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL$' -count=1
|
||||||
|
```
|
||||||
|
|
||||||
|
### Handoff
|
||||||
|
|
||||||
|
- The Stage 0 baseline and its assigned comparison of validation-command
|
||||||
|
owners remain absent and were not backfilled.
|
||||||
|
- Stage 17 should use the risk matrix and existing S02-F02, S02-F05, S07-F05,
|
||||||
|
S12-F01, and S14-F06 duplication evidence without treating file size, helper
|
||||||
|
counts, or intentional cross-boundary tests as new consolidation findings.
|
||||||
|
- Stage 18 should retain both S16 findings unless the final audited tree has
|
||||||
|
gained deterministic execution-example validation or removed the fixed-port
|
||||||
|
assumption. Its synthesis should count earlier test gaps once at their
|
||||||
|
owning component rather than cloning them from this matrix.
|
||||||
|
|||||||
Reference in New Issue
Block a user