99 lines
6.0 KiB
Markdown
99 lines
6.0 KiB
Markdown
# Test coverage and effectiveness audit
|
|
|
|
Reviewed baseline: `3d7a691` on `main` against the
|
|
[testing policy](../../policy/testing.md). The review considered public and
|
|
package contracts, failure and recovery paths, fixture realism, and the cost
|
|
of implementation-coupled assertions. Coverage is a way to locate questions,
|
|
not evidence that every exercised behavior is correct. No production code or
|
|
tests were modified for this audit.
|
|
|
|
The offline instrumented suite passed with
|
|
`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -coverpkg=./... -coverprofile=/tmp/releasekit-audit-coverage.out ./...`.
|
|
`go tool cover -func` reported 86.0%
|
|
aggregate statement coverage across instrumented packages. Child executables
|
|
and platform-specific paths are not necessarily represented by that aggregate;
|
|
it does not measure branch coverage or assertion strength.
|
|
|
|
## 1. Publication fixtures omit Git settings that change what is published
|
|
|
|
The public [publication success test](../../../publish_test.go) checks the
|
|
remote tag object, peeled commit, message, and unrelated refs using a local
|
|
bare remote. However, its ordinary Git configuration does not exercise
|
|
`push.recurseSubmodules=only`, under which the
|
|
[correctness audit](01-correctness.md#1-high-publication-can-report-success-while-leaving-the-tag-unpublished)
|
|
reproduced a successful result with no remote tag. The same test checks object
|
|
IDs and tag-message bytes, but its fixture has no Git replacement objects;
|
|
the [second correctness finding](01-correctness.md#2-high-git-replacement-objects-can-separate-validated-content-from-published-source)
|
|
demonstrates that the validated contents and remote contents can differ while
|
|
the recorded commit ID remains the same.
|
|
|
|
When these defects are addressed, add focused public-boundary regressions with
|
|
a real local bare remote. For the inherited push mode, assert that a successful
|
|
result corresponds to the exact remotely present tag object. For replacement
|
|
objects, compare the actual validated and remotely published source and note
|
|
bytes, or assert a contextual rejection before publication. Preserve the
|
|
existing ordinary success case; it protects different important behavior.
|
|
|
|
## 2. Cancellation fixtures do not cover inherited subprocess descriptors
|
|
|
|
[process cancellation tests](../../../internal/process/process_test.go) and
|
|
[candidate reader lifecycle tests](../../../internal/validation/tree_lifecycle_test.go)
|
|
exercise cancellation of a ready child and check error identity and cleanup.
|
|
Their blocked children do not leave a descendant holding an output descriptor
|
|
after the direct child is killed. The independent probes recorded in the
|
|
[concurrency audit](04-concurrency.md#1-medium-inherited-descriptors-can-keep-canceled-operations-and-reader-shutdown-blocked)
|
|
show that both buffered execution and batch-reader shutdown can then remain
|
|
blocked past cancellation, despite those tests passing.
|
|
|
|
Add controlled regressions when repairing the shared shutdown path: establish
|
|
that a descendant holds a descriptor, cancel only after readiness, and verify
|
|
bounded return, preserved context identity, and cleanup. Exercise both the
|
|
buffered subprocess boundary and the explicitly owned batch-reader path; retain
|
|
one public preparation or publication-recovery check if needed to show the
|
|
shared behavior reaches callers. Avoid treating arbitrary descendant-process
|
|
supervision as an additional contract.
|
|
|
|
## 3. Local tag creation lacks a post-write inspection failure case
|
|
|
|
The [publication tests](../../../publish_test.go) cover successful exact tag
|
|
publication, a rejected push with the local tag retained, and a changed tag
|
|
before push. They do not cover failure of the inspection commands immediately
|
|
after successful local tag creation. The isolated probe in the
|
|
[error-handling audit](06-error-handling.md#a-post-creation-inspection-failure-hides-a-retained-local-tag)
|
|
shows that the tag can remain while the returned object ID is empty and the
|
|
error does not identify that partial local state.
|
|
|
|
When the recovery behavior is selected and fixed, use a controlled Git wrapper
|
|
and a real temporary repository to fail post-creation inspection. Verify both
|
|
the actual local ref and the reported recovery evidence, the original error,
|
|
and absence of a remote push. Do not rely solely on a mocked return value.
|
|
|
|
## 4. Some assertions lock in incidental internal mechanisms
|
|
|
|
[TestMarkdownCacheSharesParsedDocumentsByBlobObjectID](../../../internal/validation/markdown_test.go)
|
|
requires two parsed document pointers to be identical. A correct cache that
|
|
returns independent immutable values, or another memory strategy, would fail
|
|
without changing link validation. The separate
|
|
[shared-document link test](../../../internal/validation/markdown_check_test.go)
|
|
already checks that identical content resolves relative to each source path.
|
|
Consider removing the pointer-identity assertions or the cache-specific test
|
|
after retaining any unique parsing checks at the behavior boundary.
|
|
|
|
`TestDeliverReadsLocalBinaryOnceForVerificationAndOnceForUpload` in
|
|
`delivery_reads_linux_test.go` at audit baseline `3d7a691`
|
|
requires exactly two filesystem opens using Linux inotify. Verification before
|
|
hosted writes and correct uploaded bytes matter; the exact number of opens is
|
|
an implementation choice, and a harmless extra metadata open or a different
|
|
safe streaming approach would fail the test. Existing delivery and artifact
|
|
tests cover tampering and uploaded content. Consider removing this exact-count
|
|
test or moving a justified read-volume concern to a benchmark, while retaining
|
|
behavioral verification of artifact integrity and ordering.
|
|
|
|
The success case in [TestPublishPushesOnlyTheCapturedAnnotatedTag](../../../publish_test.go)
|
|
also asserts exactly three remote queries, including no post-success read-back.
|
|
That exact success-path count is not necessary to establish the public result,
|
|
and would reject an additional verification step even if it preserved the
|
|
contract. Consider removing that assertion while retaining the exact remote
|
|
tag, message, and unrelated-ref checks. The separately documented single
|
|
reconciliation query after a failed push remains a distinct contract.
|