1.8 KiB
Request Validation Review Findings
Reviewed commits f315620, f3148bf, and 60784e7 against the finalized
feature roadmap and implementation plan.
1. Reuse The Existing Preparation-Parity Test Helper
Priority: P3 (test maintainability). Status: open.
In validate_request_contract_test.go,
TestValidateRequestMatchesPrepareContract defines a local testParity closure
at lines 25–45 that duplicates the package-level
assertValidateRequestParity helper at lines 87–108. Both invoke Prepare and
ValidateRequest and assert the same success/result and public-error behavior;
the package-level helper additionally returns the successful prepared value.
The nil-engine subtest repeats the same failure assertions again.
Retaining separate copies makes future parity-assertion improvements require multiple edits and increases the risk that related tests diverge unnecessarily. This is a small cleanup, not a demonstrated functional or release-blocking bug.
Requested Remediation
- Replace calls to the local closure with
assertValidateRequestParityand remove the redundant closure. - Reuse that helper for the nil-engine subtest as well; retain its
ErrInvalidConfigexpectation. - Preserve all existing cases, public-error assertions, and no-model-call checks. No production or public-contract changes are needed.
Verify with go test -race . -run '^TestValidateRequest' and
git diff --check, formatting the changed Go file.
Review Outcome
The public method and shared facade helper match the intended architecture. No implementation gaps or unintended strengthening of contracts were found. The uncached full race-enabled suite, vet, maintained consumer examples, and implementation diff whitespace checks passed during review.