Fix QA nits from PR #8

ldraney commented

2026-06-14 17:51:57 +00:00

Owner

Summary

Replace direct (*APIError) type assertions with errors.As for consistent error unwrapping across all test files
Remove dead otherTXT variable and no-op retryOn429 call in TestReplaceRecordsByType
Fix slice append gotcha in TestReplaceAllRecords — use explicit make/copy to avoid mutating the backing array
Add nil guard before dereferencing domain in TestGetDomain
Document TXT preservation gap: ReplaceRecordsByType test only preserves @ TXT records, non-@ names will be dropped

Changes

File	Fix
`pkg/godaddy/dns_test.go`	`errors.As` consistency (3 sites), dead code removal, slice safety, TXT gap comment
`pkg/godaddy/domains_test.go`	Nil dereference guard

Test Plan

go build ./... passes
go vet ./... passes
go test ./pkg/godaddy/ -v with credentials (live API — same as PR #8)

Review Checklist

No behavioral changes — all fixes are defensive/hygienic
errors.As pattern matches client_test.go retryOn429 helper
Dead code fully removed (no leftover references)
Slice copy pattern prevents subtle mutation bugs

Follows up on QA review of PR #8 (Go scaffold with provider skeleton and pkg/godaddy DNS client)

Closes #10

## Summary - Replace direct `(*APIError)` type assertions with `errors.As` for consistent error unwrapping across all test files - Remove dead `otherTXT` variable and no-op `retryOn429` call in `TestReplaceRecordsByType` - Fix slice append gotcha in `TestReplaceAllRecords` — use explicit `make`/`copy` to avoid mutating the backing array - Add nil guard before dereferencing `domain` in `TestGetDomain` - Document TXT preservation gap: `ReplaceRecordsByType` test only preserves `@` TXT records, non-`@` names will be dropped ## Changes | File | Fix | |------|-----| | `pkg/godaddy/dns_test.go` | `errors.As` consistency (3 sites), dead code removal, slice safety, TXT gap comment | | `pkg/godaddy/domains_test.go` | Nil dereference guard | ## Test Plan - [x] `go build ./...` passes - [x] `go vet ./...` passes - [ ] `go test ./pkg/godaddy/ -v` with credentials (live API — same as PR #8) ## Review Checklist - [x] No behavioral changes — all fixes are defensive/hygienic - [x] `errors.As` pattern matches `client_test.go` `retryOn429` helper - [x] Dead code fully removed (no leftover references) - [x] Slice copy pattern prevents subtle mutation bugs ## Related Notes - Follows up on QA review of PR #8 (Go scaffold with provider skeleton and pkg/godaddy DNS client) Closes #10

ldraney added 1 commit

2026-06-14 17:51:57 +00:00

Fix QA nits: errors.As consistency, dead code, nil guard, slice safety b4a49b8c28

Closes #10

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented

2026-06-14 17:53:19 +00:00

Author

Owner

PR #29 Review

DOMAIN REVIEW

Stack: Go test files for a GoDaddy API client (pkg/godaddy/). Applying Go idiom, error handling, and test quality checks.

1. errors.As migration (dns_test.go, 3 sites) -- Correct and necessary

This is not merely a style preference. The APIError is returned as *APIError from client.do(), but GetRecords (and other client methods) wrap it via fmt.Errorf("getting records for %s %s.%s: %w", ...). The %w verb wraps the error in a chain. A direct type assertion err.(*APIError) would fail on wrapped errors; errors.As unwraps the chain. This fix prevents a class of silent test bugs where a 404 would not be recognized and the test would fail spuriously.

The pattern now matches retryOn429 in client_test.go (line 55), which already used errors.As. Consistency restored.

2. Dead code removal (dns_test.go) -- Clean

The otherTXT variable, its no-op retryOn429 call, and the _ = otherTXT suppression line are all removed together. No dangling references remain. The block was literally a no-op (the closure set otherTXT = nil and returned nil).

3. TXT preservation gap comment (dns_test.go) -- Accurate

The comment correctly documents that TestReplaceRecordsByType only preserves @ TXT records because it calls GetRecords(ctx, testDomain, "TXT", "@"). Non-@ TXT names (e.g., _dmarc, _domainconnect) are silently dropped. This is a real production risk and the comment is a useful artifact for the future fix.

4. Slice safety fix (dns_test.go, TestReplaceAllRecords) -- Correct

Old code: replacement := append(existing, ...) -- if existing has spare backing-array capacity, append mutates the original slice's memory. The new make/copy/append pattern is the standard Go idiom for this. Note: this test is behind t.Skip so the risk was low, but the fix is still correct for when the skip is removed.

5. Nil guard (domains_test.go, TestGetDomain) -- Correct

GetDomain returns (*Domain, error). If both are nil (unexpected but possible), the original code would panic on domain.Domain. The nil guard with t.Fatal fails fast with a clear message.

errors import addition (dns_test.go) -- Correct

The "errors" import is added at line 4 in sorted position within the stdlib block. Matches goimports conventions.

BLOCKERS

None.

NITS

Test Plan checkbox: The PR body shows go test ./pkg/godaddy/ -v as unchecked. This is expected since it requires live API credentials, but worth noting it was not run as part of this PR.
TXT gap is a known tech-debt item: The comment documents the problem well. Consider tracking it as a separate issue so it does not get lost (low priority, not blocking this PR).

SOP COMPLIANCE

PR body has ## Summary -- yes
PR body has ## Changes -- yes, with a table
PR body has ## Test Plan -- yes, with checkboxes
PR body has ## Related -- yes (Related Notes section references PR #8, closes #10)
No secrets, .env files, or credentials committed
No unnecessary file changes (2 files changed, all within scope of issue #10)
Commit messages are descriptive (title matches issue)
Changes are purely defensive/hygienic -- no behavioral changes

PROCESS OBSERVATIONS

Clean follow-up from QA review of PR #8. The review-fix loop is working as intended.
The errors.As fix is the most impactful change here. Without it, wrapped errors from GetRecords would silently bypass the 404 check, causing test flakiness against the live API. Good catch in the original review.
Change failure risk: minimal. All fixes are in test files only; no production code is touched.

VERDICT: APPROVED

## PR #29 Review ### DOMAIN REVIEW **Stack:** Go test files for a GoDaddy API client (`pkg/godaddy/`). Applying Go idiom, error handling, and test quality checks. **1. `errors.As` migration (dns_test.go, 3 sites) -- Correct and necessary** This is not merely a style preference. The `APIError` is returned as `*APIError` from `client.do()`, but `GetRecords` (and other client methods) wrap it via `fmt.Errorf("getting records for %s %s.%s: %w", ...)`. The `%w` verb wraps the error in a chain. A direct type assertion `err.(*APIError)` would fail on wrapped errors; `errors.As` unwraps the chain. This fix prevents a class of silent test bugs where a 404 would not be recognized and the test would fail spuriously. The pattern now matches `retryOn429` in `client_test.go` (line 55), which already used `errors.As`. Consistency restored. **2. Dead code removal (dns_test.go) -- Clean** The `otherTXT` variable, its no-op `retryOn429` call, and the `_ = otherTXT` suppression line are all removed together. No dangling references remain. The block was literally a no-op (the closure set `otherTXT = nil` and returned `nil`). **3. TXT preservation gap comment (dns_test.go) -- Accurate** The comment correctly documents that `TestReplaceRecordsByType` only preserves `@` TXT records because it calls `GetRecords(ctx, testDomain, "TXT", "@")`. Non-`@` TXT names (e.g., `_dmarc`, `_domainconnect`) are silently dropped. This is a real production risk and the comment is a useful artifact for the future fix. **4. Slice safety fix (dns_test.go, `TestReplaceAllRecords`) -- Correct** Old code: `replacement := append(existing, ...)` -- if `existing` has spare backing-array capacity, `append` mutates the original slice's memory. The new `make`/`copy`/`append` pattern is the standard Go idiom for this. Note: this test is behind `t.Skip` so the risk was low, but the fix is still correct for when the skip is removed. **5. Nil guard (domains_test.go, `TestGetDomain`) -- Correct** `GetDomain` returns `(*Domain, error)`. If both are nil (unexpected but possible), the original code would panic on `domain.Domain`. The nil guard with `t.Fatal` fails fast with a clear message. **`errors` import addition (dns_test.go) -- Correct** The `"errors"` import is added at line 4 in sorted position within the stdlib block. Matches `goimports` conventions. ### BLOCKERS None. ### NITS 1. **Test Plan checkbox:** The PR body shows `go test ./pkg/godaddy/ -v` as unchecked. This is expected since it requires live API credentials, but worth noting it was not run as part of this PR. 2. **TXT gap is a known tech-debt item:** The comment documents the problem well. Consider tracking it as a separate issue so it does not get lost (low priority, not blocking this PR). ### SOP COMPLIANCE - [x] PR body has `## Summary` -- yes - [x] PR body has `## Changes` -- yes, with a table - [x] PR body has `## Test Plan` -- yes, with checkboxes - [x] PR body has `## Related` -- yes (`Related Notes` section references PR #8, closes #10) - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (2 files changed, all within scope of issue #10) - [x] Commit messages are descriptive (title matches issue) - [x] Changes are purely defensive/hygienic -- no behavioral changes ### PROCESS OBSERVATIONS - Clean follow-up from QA review of PR #8. The review-fix loop is working as intended. - The `errors.As` fix is the most impactful change here. Without it, wrapped errors from `GetRecords` would silently bypass the 404 check, causing test flakiness against the live API. Good catch in the original review. - Change failure risk: minimal. All fixes are in test files only; no production code is touched. ### VERDICT: APPROVED

ldraney added 1 commit

2026-06-14 17:57:54 +00:00