Complete the Stage 21 test durability audit
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# Repository Audit Ledger
|
||||
|
||||
Status: In progress; Stages 1-20 complete.
|
||||
Status: In progress; Stages 1-21 complete.
|
||||
|
||||
This temporary roadmap document is the evidence ledger for the staged audit
|
||||
defined by [the audit plan](audit-plan.md). It records audit evidence and
|
||||
@@ -165,6 +165,18 @@ but no additional consequential unowned failure mode. Representative app and
|
||||
CLI tests appropriately prove cross-package ordering and result projection
|
||||
without repeating every lower-level malformed-input case. Detailed test
|
||||
durability, fixture cost, and consolidation remain Stage 21.
|
||||
Stage 21 found four low-severity test-maintenance issues: a direct assertion of
|
||||
private SPC policy constants is already subsumed by stronger observable risk-
|
||||
digest cases; collection and Distributor polling tests unnecessarily pay 1.00
|
||||
second and 0.25 second production wait intervals; generated-text render-context
|
||||
tests repeat reporttemplate's report-specific Markdown assertions; and a shared
|
||||
comparison concurrency helper releases blocked workers on one diagnostic path
|
||||
but not on its timeout, early-completion, or duplicate-start exits. Exact error
|
||||
checks are otherwise limited and generally protect user-visible or safety
|
||||
semantics; dense configuration/domain tables, realistic filesystem tests,
|
||||
small external-boundary fakes, and representative app/CLI workflows earn their
|
||||
cost. No complete-output golden snapshots or oversized fixture framework was
|
||||
found.
|
||||
Subsystem conclusions and final disposition remain pending the later stages.
|
||||
|
||||
## Baseline Metadata
|
||||
@@ -312,7 +324,7 @@ inventory commands, graph index refresh, and graph architecture inspection.
|
||||
| 18 | Audit comparison execution and CLI integration | Complete |
|
||||
| 19 | Audit test hermeticity and execution hygiene | Complete |
|
||||
| 20 | Audit test risk coverage and ownership | Complete |
|
||||
| 21 | Audit test durability, duplication, and maintenance cost | Pending |
|
||||
| 21 | Audit test durability, duplication, and maintenance cost | Complete |
|
||||
| 22 | Audit cross-cutting efficiency and complexity | Pending |
|
||||
| 23 | Audit cross-cutting refactoring and deduplication opportunities | Pending |
|
||||
| 24 | Audit documentation coherence and executable contracts | Pending |
|
||||
@@ -346,7 +358,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
||||
| Comparison concurrency and CLI behavior | `internal/app`, `internal/cli` | 18 | Partly insufficient. Explicit profiles are preflighted sequentially, one immutable prepared package feeds concurrent profile-local executions, goroutines join, slice positions lock durable/CLI order, partial bundles publish coherently, and safe summaries preserve committed paths and fail the action on any error. `AUD-058` records a missing mixed failure/cancellation case and the resulting overwrite of an already completed profile failure. |
|
||||
| Test hermeticity and execution hygiene | All Go tests, fixtures, test assets, and helper mechanisms | 19 | Partly insufficient. The suite is offline, uses controlled clocks/roots/environment for almost every stateful case, has no subprocesses or automatic golden updates, passes race and shuffled-repeat checks, and succeeds with a scrubbed environment and broken external proxies. `AUD-059` records one ambient credential assumption; `AUD-060` records unguarded symlink-capability assumptions. |
|
||||
| Test risk coverage and ownership | Repository-wide suite | 20 | Partly insufficient. Consequential contracts have narrow owners and representative cross-boundary workflows; every identified missing regression is already attached to the production or test-gap finding for the defect it would catch. Coverage diagnostics found no additional unowned critical branch. Accepted omissions are thin entry-point/delegation wrappers, defensive accessors, and dormant helpers already routed under `AUD-001`; broad duplication and durability remain Stage 21. |
|
||||
| Test durability, duplication, and maintenance cost | Repository-wide suite | 21 | Pending |
|
||||
| Test durability, duplication, and maintenance cost | Repository-wide suite | 21 | Partly insufficient. Most tests assert stable behavior with direct fixtures and appropriately narrow fakes. `AUD-039`, `AUD-061`, and `AUD-063` identify duplicated or implementation-coupled assertions; `AUD-062` identifies avoidable production waits; `AUD-064` identifies incomplete failure cleanup. Each consolidation retains a named behavioral owner. |
|
||||
| Cross-cutting efficiency and maintainability | Graph metrics plus focused tests | 22-23 | Pending |
|
||||
| Documentation and executable-contract coherence | Canonical documents, code, schemas, templates, examples | 24 | Pending |
|
||||
| Dynamic robustness and diagnostic checks | Repository-wide deterministic checks | 25 | Pending |
|
||||
@@ -2649,6 +2661,150 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
||||
- Related findings: `AUD-035`
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-061: SPC policy constants have a redundant implementation-coupled test
|
||||
|
||||
- Stage: 21
|
||||
- Status: candidate
|
||||
- Severity: low
|
||||
- Confidence: high
|
||||
- Category: test maintenance
|
||||
- Area: `internal/briefing.TestSPCRiskDigestDefaultPolicyConstants`
|
||||
- Evidence: The test directly asserts that two unexported constants equal
|
||||
`"categorical"` and `3`. The adjacent
|
||||
`TestSPCConvectiveOutlooksRiskDigestFilters` already exercises the stronger
|
||||
observable policy: categorical rank 3 is included, rank 2 is excluded, and a
|
||||
non-categorical rank 30 product is excluded. Any consequential change to
|
||||
either constant therefore fails the behavioral table without the direct
|
||||
constant assertion.
|
||||
- Contract at risk: Tests should protect the risk-digest selection policy, not
|
||||
the private representation used to supply it.
|
||||
- Impact: A semantics-preserving change that derives or configures the same
|
||||
policy can require editing a test that provides no additional confidence.
|
||||
The current cost is small but wholly redundant.
|
||||
- Recommendation: Delete the constant test and retain the observable filter
|
||||
table as the sole default-policy owner. Add relational boundary rows there
|
||||
if the policy gains more dimensions rather than asserting its private fields.
|
||||
- Test implications: Confidence remains in the module-level included/excluded
|
||||
outputs, including both threshold and product-type boundaries; only eight
|
||||
lines coupled to private names and storage disappear.
|
||||
- Validation: Mutating either effective default changes the filter-table
|
||||
outcome, while replacing the constants with an equivalent implementation
|
||||
requires no test edit.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-062: Two integration tests pay production retry and polling intervals
|
||||
|
||||
- Stage: 21
|
||||
- Status: candidate
|
||||
- Severity: low
|
||||
- Confidence: high
|
||||
- Category: test maintenance
|
||||
- Area: `internal/collect.TestRunWrapsFetchError` and
|
||||
`internal/adapters/distributor.TestUploadPollsUntilTerminalStatus`
|
||||
- Evidence: An uncached JSON-timed suite reports these tests at 1.00 second and
|
||||
0.25 second, respectively; every other top-level test completes in at most
|
||||
0.04 second on the audit host. The collection test returns a retryable 502
|
||||
from `/observations`, so production's one-second fetch retry delay dominates
|
||||
a test whose assertions concern collection error wrapping. The Distributor
|
||||
test deliberately returns accepted then succeeded and therefore pays the
|
||||
fixed 250-millisecond production poll interval to assert two status calls.
|
||||
Focused adapter tests separately own retry, cancellation, and timeout policy.
|
||||
- Contract at risk: Tests should wait on events, not operational delays, unless
|
||||
elapsed time is the contract under test.
|
||||
- Impact: Every uncached suite spends avoidable time waiting; repeated and
|
||||
shuffled runs multiply it. The waits also couple feedback cost to future
|
||||
production timing changes without adding timing confidence.
|
||||
- Recommendation: Make the collection wrapper case use a non-retryable source
|
||||
failure, preserving its production-adapter traversal and error context. Give
|
||||
the Distributor client a narrow test-controlled wait mechanism or interval
|
||||
so accepted-to-terminal polling and call count remain real while elapsed
|
||||
wall time approaches zero.
|
||||
- Test implications: Collection still proves adapter construction, source
|
||||
failure propagation, and collection context; Distributor still proves
|
||||
accepted-to-terminal polling, report retention, and exactly two status
|
||||
calls. Retry timing/cancellation remains in its focused adapter cases.
|
||||
- Validation: Both tests preserve their current behavioral assertions, take no
|
||||
production-sized sleep, and the suite's cancellation/timeout tests still
|
||||
detect a wait mechanism that ignores context.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-063: Render-context tests duplicate template output ownership
|
||||
|
||||
- Stage: 21
|
||||
- Status: candidate
|
||||
- Severity: low
|
||||
- Confidence: high
|
||||
- Category: test maintenance
|
||||
- Area: `internal/generatedtext/render_context_test.go` and
|
||||
`internal/reporttemplate/reporttemplate_test.go`
|
||||
- Evidence: Each of the four full generated-text context tests first asserts
|
||||
typed report/module projection, then calls `reporttemplate.Render` and checks
|
||||
substantial report-specific Markdown fragments and ordering. The
|
||||
reporttemplate suite independently owns those same four reports, partials,
|
||||
absent-section behavior, ordering, and semantic output. The files are 1,397
|
||||
and 1,183 lines, have changed 15 and 19 times in graph history, and changed
|
||||
together 11 times with a 0.73 coupling score. The context suite also contains
|
||||
a 185-line shared day-style projection test and four 126-147-line snapshot
|
||||
fixtures, amplifying cross-owner edits when template wording changes.
|
||||
- Contract at risk: Generated-text tests should own typed context construction;
|
||||
reporttemplate tests should own Markdown semantics; only a representative
|
||||
integration needs to prove the two contracts compose.
|
||||
- Impact: A legitimate template-only change can fail two large suites and
|
||||
require updating expectations outside the template owner. The duplicate
|
||||
output checks add noisy diagnostics while offering little protection beyond
|
||||
the focused template suite.
|
||||
- Recommendation: Keep all report-specific typed context, optional-module, and
|
||||
extraction assertions in generatedtext, and all report-specific Markdown
|
||||
semantics in reporttemplate. Retain one small real-typed-context render smoke
|
||||
case to protect integration. Consolidate only fixture fields common to the
|
||||
day-style reports; keep report-specific planning and ordering values explicit.
|
||||
- Test implications: Confidence remains in every context mapping and every
|
||||
template output through their current focused owners, plus one cross-package
|
||||
composition proof. Removing the other rendered-fragment blocks reduces
|
||||
duplicated expectations and fixture-driven change amplification.
|
||||
- Validation: Breaking any typed projection fails generatedtext; breaking any
|
||||
report template or partial fails reporttemplate; making the two boundaries
|
||||
incompatible fails the retained integration case.
|
||||
- Related findings: `AUD-039`
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-064: Comparison test failure exits can leave workers blocked
|
||||
|
||||
- Stage: 21
|
||||
- Status: candidate
|
||||
- Severity: low
|
||||
- Confidence: high
|
||||
- Category: test diagnostics
|
||||
- Area: `internal/app.waitForProfileStarts` and its four comparison-execution
|
||||
test callers
|
||||
- Evidence: The helper waits up to five seconds for every profile to reach a
|
||||
barrier. It calls `executor.releaseAll()` only when a debug callback reports
|
||||
failure. An early aggregate result, timeout, or duplicate start calls
|
||||
`t.Fatal` without releasing profiles already blocked in the executor, even
|
||||
though the helper is shared by four tests. Normal paths release or cancel all
|
||||
work and the race suite passes, so the defect appears only while diagnosing a
|
||||
production regression or broken fixture.
|
||||
- Contract at risk: Concurrency tests should clean up test-owned workers on
|
||||
both success and failure so their primary diagnostic remains trustworthy.
|
||||
- Impact: A regression that starts too few profiles incurs the full timeout and
|
||||
leaves goroutines blocked until the package test process exits. Those leaks
|
||||
can add secondary noise or interfere with later tests, obscuring the original
|
||||
ordering/concurrency failure.
|
||||
- Recommendation: Register idempotent `executor.releaseAll` cleanup before the
|
||||
execution goroutine starts, and ensure the result goroutine is drainable or
|
||||
joined on every helper exit. Keep the five-second timer only as a deadlock
|
||||
diagnostic rather than weakening the concurrency requirement.
|
||||
- Test implications: All four current ordering, failure isolation,
|
||||
cancellation, and debug-reference assertions remain unchanged; only
|
||||
failure-path cleanup is added.
|
||||
- Validation: Deliberately suppressing one start produces the intended timeout
|
||||
diagnosis and leaves no blocked comparison workers under the race detector
|
||||
or a goroutine-leak probe.
|
||||
- Related findings: `AUD-058`
|
||||
- Remediation reference: pending
|
||||
|
||||
## Retained Decisions
|
||||
|
||||
### RET-001: Keep the application package as the explicit composition owner
|
||||
@@ -3196,6 +3352,36 @@ locate surprising weak spots, while requiring each added regression to name a
|
||||
realistic consequential defect and its stable owner. The currently important
|
||||
weak spots are already recorded by defect rather than by uncovered line.
|
||||
|
||||
### RET-047: Keep dense contract tables explicit
|
||||
|
||||
The largest current files concentrate configuration fields, generated/rendered
|
||||
report shapes, briefing rules, HTTP products, and filesystem transaction
|
||||
states. Their size alone is not a defect. Configuration defaults and retired-
|
||||
field rejections are user/compatibility contracts; meteorological threshold
|
||||
tables make boundary cases reviewable; comparison tables enumerate destructive
|
||||
destination and recovery states. Retain these explicit cases while removing
|
||||
only the named semantic duplication in `AUD-039`, `AUD-061`, and `AUD-063`.
|
||||
|
||||
### RET-048: Keep small interaction fakes at external and orchestration seams
|
||||
|
||||
Promptkit, Distributor, collection, app, and CLI fakes record calls, results,
|
||||
and ordering only where those interactions are requirements: one executor,
|
||||
publish before notify, exact upload mappings, profile concurrency, cancellation
|
||||
propagation, and no work after preflight failure. They do not reproduce full
|
||||
provider implementations or general mock choreography. Retain these local
|
||||
doubles; replacing them with a shared framework would increase indirection and
|
||||
maintenance cost without improving confidence.
|
||||
|
||||
### RET-049: Keep semantic assertions instead of complete-output snapshots
|
||||
|
||||
The suite has no golden or full Markdown snapshot files. Exact whole-byte
|
||||
assertions are confined to canonical compatibility encodings, normalized JSON,
|
||||
atomic replacement payloads, and copy-isolation sentinels. Markdown tests use
|
||||
ordered semantic fragments and explicit omissions, which make intentional
|
||||
template changes reviewable without memorializing incidental whitespace.
|
||||
Retain that style while consolidating the cross-owner render checks in
|
||||
`AUD-063`.
|
||||
|
||||
## Open Questions
|
||||
|
||||
No Stage 1 open questions or unexplained baseline failures remain.
|
||||
@@ -3551,6 +3737,26 @@ Stage 20 routed these investigation leads to their assigned later stages:
|
||||
concerns remain execution/durability questions for Stages 21-22. They do not
|
||||
leave a consequential production invariant without a test owner.
|
||||
|
||||
Stage 21 routed these investigation leads to their assigned later stages:
|
||||
|
||||
- The collection and Distributor wait controls in `AUD-062` are test-cost
|
||||
seams, not evidence that production retry or polling intervals are globally
|
||||
inefficient. Stage 22 should assess runtime waits only in its broader
|
||||
operational context; Stage 23 may choose the smallest ownership-preserving
|
||||
injection shape during remediation planning.
|
||||
- `config_test.go` and the generated-text/template suites are physically large,
|
||||
but their explicit contract tables remain valuable under `RET-047`.
|
||||
Stage 23 may consider file organization or narrow fixture builders only where
|
||||
it reduces navigation/change cost without hiding report-specific values.
|
||||
- Historical rejection tests for removed workspace, report, Distributor, and
|
||||
execution settings remain plausible regression guards for statelessness and
|
||||
strict compatibility. Stage 26 should consolidate them only if a stronger
|
||||
executable forbidden-surface invariant demonstrably retains those named
|
||||
protections.
|
||||
- The absence of fuzz targets remains the Stage 25 adversarial-validation lead
|
||||
recorded by Stage 20; Stage 21 did not treat it as a durability or test-count
|
||||
issue.
|
||||
|
||||
## Stage Log
|
||||
|
||||
### Stage 1: Establish The Baseline And Audit Ledger
|
||||
@@ -4664,3 +4870,64 @@ Stage 20 routed these investigation leads to their assigned later stages:
|
||||
- Retained decisions: `RET-045` and `RET-046`.
|
||||
- Open questions: the four leads recorded above are routed to their assigned
|
||||
later stages.
|
||||
|
||||
### Stage 21: Audit Test Durability, Duplication, And Maintenance Cost
|
||||
|
||||
- Status: Complete.
|
||||
- Scope reviewed: all 50 Go test files and 418 top-level tests, totaling 16,876
|
||||
lines; the largest and highest-change test files; exact and fragment error
|
||||
assertions; defaults and thresholds; direct private-symbol assertions;
|
||||
external-boundary and orchestration fakes; repeated report/workflow fixtures;
|
||||
canonical byte assertions and rendered-output checks; table ownership;
|
||||
helper size; historical regression guards; concurrent failure cleanup; fresh
|
||||
test timing; graph test/call/co-change relationships; and test-file history.
|
||||
- Exclusions: production runtime/algorithmic efficiency remains Stage 22;
|
||||
implementation and package refactoring remain Stage 23; documentation
|
||||
coherence remains Stage 24; fuzz/adversarial execution remains Stage 25; and
|
||||
remediation remains Stages 26-27. No production code or tests were changed.
|
||||
|
||||
#### Durability And Maintenance Accounting
|
||||
|
||||
| Candidate | Protected behavior and maintenance evidence | Disposition |
|
||||
| --- | --- | --- |
|
||||
| Exact error strings | Six direct equality sites cover one CLI unknown-command diagnostic, three shared generated-text validation messages, and two comparison aggregate summaries. Most other cases use error identity/category, structured fields, safe omissions, or the smallest actionable fragment. | Retain CLI and comparison boundary checks. Generated-text equality participates in the broader duplicated policy finding `AUD-039`; no repository-wide exact-string smell. |
|
||||
| Defaults and thresholds | Config tests assert documented defaults/examples; adapter mapping tests use synthetic nondefault values; forecast/briefing tests mostly compare outputs relationally or reference the production threshold where the literal is not itself the contract. One test directly repeats private SPC constants despite an adjacent stronger behavior table. | Retain contractual defaults and relational mechanisms. Remove only the redundant private assertion under `AUD-061`. |
|
||||
| Private-helper coupling | Direct tests of batch planning, render projection, classification, and path helpers express dense stable package rules more clearly than assembled workflows. The SPC constant case instead protects representation with no marginal behavior. | Focused helper tests are justified; `AUD-061` is the narrow exception. |
|
||||
| Mock choreography | Promptkit and Distributor fakes, Weather API local servers, and app/CLI doubles observe calls only for exact external mapping, one construction/upload, publish-before-notify, no-work-on-preflight, cancellation, capacity, and ordering contracts. | Retain under `RET-048`; no oversized mocking framework or noncontractual call graph was found. |
|
||||
| Repeated workflow fixtures | Small `generationBundle`/`generationTime` helpers have 28/30 graph callers and keep representative app/CLI workflows concise. The 156-line briefing context supplies many independent module cases but is a plain deterministic value, not a behavioral framework. | Retain. Shared fixtures reduce rather than amplify incidental setup, and callers assert distinct outcomes. |
|
||||
| Render and report-family fixtures | Typed context tests own projection but also render all four reports and repeat reporttemplate's semantic fragments. The two suites changed together 11 times with 0.73 graph coupling; large day-style fixtures amplify that overlap. | Consolidate only the cross-owner Markdown assertions under `AUD-063`; retain report-specific typed projections, template semantics, and one real composition proof. |
|
||||
| Complete-output snapshots | No goldens or approval snapshots exist. Exact bytes protect canonical manifest/JSON/YAML encodings, atomic payload preservation, or copy isolation. Markdown assertions use selected fragments, ordering, and omissions. | Retain under `RET-049`; these exact values are compatibility/state contracts rather than incidental output dumps. |
|
||||
| Redundant tables | Generated-text day-style/schema duplication is already `AUD-039`. The SPC output filter table subsumes its constant test. Config unknown/retired fields, module compatibility, weather source policy, and comparison destination tables describe distinct inputs or states. | `AUD-039` and `AUD-061` own the proven redundancies. Keep the remaining explicit matrices under `RET-047`. |
|
||||
| Helper frameworks | Sixty-seven test helper functions/types matching common helper roles are package-local and mostly small; the largest are literal context/snapshot builders. No shared test package, fluent fixture API, auto-updater, or reflection-heavy assertion framework exists. | No finding. Prefer local direct helpers; do not create a common framework merely to reduce line count. |
|
||||
| Regression guards | Removed workspace, recent-change, report-alias, Distributor-path, Scriptorium, and prompt-runtime cases protect strict configuration, statelessness, or migration diagnostics. Destructive publication and comparison regressions retain distinct state outcomes. | Retain. The removed surfaces remain plausible to reintroduce and are not all implied by one generic unknown-field case. |
|
||||
| Execution waits | Fresh JSON timing found a 1.00-second collection wrapper test and 0.25-second Distributor polling test; no other top-level test exceeded 0.04 second. Neither case is intended to measure elapsed time. | Avoidable cost under `AUD-062`; preserve the same error/polling behavior with nonretryable input or a controlled wait seam. |
|
||||
| Concurrent failure diagnostics | Normal comparison/debug concurrency completes, joins, and passes the race suite. `waitForProfileStarts` has four callers and releases its barrier only for callback failure, not all fatal exits. Prompt-debug filesystem workers have no artificial barrier to release. | Add comparison cleanup under `AUD-064`; retain bounded five-second liveness diagnostics. |
|
||||
|
||||
#### Commands And Evidence
|
||||
|
||||
- Used graph search, symbol snippets, and relationship traces to inspect the
|
||||
largest tests/helpers, their production targets, call counts, and direct
|
||||
behavioral owners. Graph history ranked current test-file change counts and
|
||||
identified 11 co-changes (0.73 coupling) between generated-text context and
|
||||
reporttemplate tests.
|
||||
- Used targeted text search for exact error equality, whole-byte comparisons,
|
||||
defaults/durations, liveness timers, legacy regression names, and test-only
|
||||
helper declarations. Counted 50 test files, 418 top-level tests, 16,876 test
|
||||
lines, six exact error equality sites, and 67 package-local helper
|
||||
declarations matching common fixture/assertion roles.
|
||||
- Reviewed Git history for dense/current test files rather than inferring cost
|
||||
from size alone. The graph records 37 changes to `config_test.go`, 19 to
|
||||
`reporttemplate_test.go`, and 15 to `render_context_test.go`; the first
|
||||
remains primarily explicit user-contract coverage, while the latter pair has
|
||||
the proven cross-owner overlap in `AUD-063`.
|
||||
- Ran an uncached `go test -count=1 -json ./...` timing pass. All packages
|
||||
passed; `TestRunWrapsFetchError` took 1.00 second and
|
||||
`TestUploadPollsUntilTerminalStatus` took 0.25 second, while every other
|
||||
top-level test completed in at most 0.04 second on the audit host.
|
||||
- Ran `go test ./...`, `go vet ./...`,
|
||||
`go run ./cmd/weatherreporter --help`, and `git diff --check`; all passed.
|
||||
- Findings: `AUD-061` through `AUD-064`; existing duplication finding
|
||||
`AUD-039` remains applicable.
|
||||
- Retained decisions: `RET-047` through `RET-049`.
|
||||
- Open questions: the four leads recorded above are routed to their assigned
|
||||
later stages.
|
||||
|
||||
Reference in New Issue
Block a user