ldraney/godaddy-tofu
1
0
Fork 0
Code Issues 2 Pull requests 1 Projects Releases Packages Wiki Activity Actions

Add acceptance tests for DNS record resource and data source #35

Merged
ldraney merged 1 commit from 13-acceptance-tests into main 2026-06-14 19:47:15 +00:00
Conversation 1 Commits 1 Files changed 5 +389 -1
ldraney commented 2026-06-14 19:25:21 +00:00
Owner
Copy link

Summary

Adds provider-level acceptance tests using terraform-plugin-testing to verify the full plan/apply/destroy lifecycle for godaddy_dns_record resource and godaddy_dns_records data source against the live GoDaddy API.

Changes

  • provider_test.go -- shared test infrastructure: ProtoV6 provider factory map, testAccPreCheck and testAccPreCheckWithDomain helpers checking required env vars
  • resource_dns_record_test.go -- DNS record resource CRUD lifecycle test: create A record with _test-basic prefix, verify all attributes via statecheck.ExpectKnownValue, update TTL and verify, import via composite ID domain:A:name, CheckDestroy calling GoDaddy API directly to confirm deletion
  • data_source_dns_records_test.go -- DNS records data source test: creates a test record then reads it back via data source filtered by type and name, verifies computed ID and records list size
  • go.mod / go.sum -- adds github.com/hashicorp/terraform-plugin-testing v1.16.0 dependency

Test Plan

  • go build ./... -- passes (compile check)
  • go vet ./... -- passes (static analysis)
  • TF_ACC=1 GODADDY_API_KEY=... GODADDY_API_SECRET=... GODADDY_TEST_DOMAIN=... go test ./... -v -- runs full acceptance suite (requires live API credentials)
  • Tests use _test- prefix names for isolation from production records
  • CheckDestroy confirms records are deleted after each test

Review Checklist

  • go build ./... passes
  • go vet ./... passes
  • Tests follow conventions.md patterns (factory, statecheck, CheckDestroy)
  • Test isolation via _test- prefix names
  • No live API tests run without TF_ACC=1

Related Notes

  • Closes #13
  • Follows #4 (client integration tests)
  • Depends on #11 (DNS record resource) and #12 (DNS data source)
## Summary Adds provider-level acceptance tests using `terraform-plugin-testing` to verify the full plan/apply/destroy lifecycle for `godaddy_dns_record` resource and `godaddy_dns_records` data source against the live GoDaddy API. ## Changes - `provider_test.go` -- shared test infrastructure: ProtoV6 provider factory map, `testAccPreCheck` and `testAccPreCheckWithDomain` helpers checking required env vars - `resource_dns_record_test.go` -- DNS record resource CRUD lifecycle test: create A record with `_test-basic` prefix, verify all attributes via `statecheck.ExpectKnownValue`, update TTL and verify, import via composite ID `domain:A:name`, `CheckDestroy` calling GoDaddy API directly to confirm deletion - `data_source_dns_records_test.go` -- DNS records data source test: creates a test record then reads it back via data source filtered by type and name, verifies computed ID and records list size - `go.mod` / `go.sum` -- adds `github.com/hashicorp/terraform-plugin-testing` v1.16.0 dependency ## Test Plan - `go build ./...` -- passes (compile check) - `go vet ./...` -- passes (static analysis) - `TF_ACC=1 GODADDY_API_KEY=... GODADDY_API_SECRET=... GODADDY_TEST_DOMAIN=... go test ./... -v` -- runs full acceptance suite (requires live API credentials) - Tests use `_test-` prefix names for isolation from production records - `CheckDestroy` confirms records are deleted after each test ## Review Checklist - [x] `go build ./...` passes - [x] `go vet ./...` passes - [x] Tests follow conventions.md patterns (factory, statecheck, CheckDestroy) - [x] Test isolation via `_test-` prefix names - [x] No live API tests run without `TF_ACC=1` ## Related Notes - Closes #13 - Follows #4 (client integration tests) - Depends on #11 (DNS record resource) and #12 (DNS data source)
Implements provider-level acceptance tests using terraform-plugin-testing
to verify the full plan/apply/destroy lifecycle against the live GoDaddy
API. Tests cover CRUD operations, TTL updates, import by composite ID,
and data source reads with self-contained record creation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-14 19:26:53 +00:00
Author
Owner
Copy link

PR #35 Review

DOMAIN REVIEW

Tech stack: Go / Terraform Plugin Framework / terraform-plugin-testing v1.16.0

This PR adds acceptance tests for the godaddy_dns_record resource and godaddy_dns_records data source. Three new test files follow the canonical pattern for Plugin Framework provider acceptance testing.

Provider factory (provider_test.go:15):

"godaddy": providerserver.NewProtocol6WithError(New("test")()),

The PR uses New("test")() which is correct -- New returns func() provider.Provider, so calling it twice (once to get the factory, once to invoke it) produces the provider.Provider value that NewProtocol6WithError expects. Note that docs/conventions.md line 129 shows New("test") (without the second invocation), which would fail to compile due to type mismatch. The conventions doc should be corrected in a follow-up, but the PR code is right.

Test coverage analysis:

CRUD phase Covered? How
Create Yes Step 1: creates A record, verifies all 6 attributes
Read Yes Implicit in state checks after create/update; also exercised by data source test
Update Yes Step 2: updates TTL from 600 to 3600, confirms change + stability of other fields
Delete Yes CheckDestroy calls GoDaddy API directly to confirm deletion
Import Yes Step 3: imports by composite ID domain:A:_test-basic, verifies full state round-trip

statecheck usage: All assertions use the modern statecheck.ExpectKnownValue + knownvalue + tfjsonpath API per conventions. No deprecated resource.TestCheckResourceAttr usage.

CheckDestroy (resource_dns_record_test.go:74-101): Correctly creates a direct API client with env var credentials and queries GoDaddy for each godaddy_dns_record resource in state. API errors (including 404) are treated as successful destruction. Non-empty record list returns an error. This matches the conventions doc pattern exactly.

Data source test (data_source_dns_records_test.go): Self-contained -- creates a resource first, then reads it back via the data source with depends_on. Verifies computed ID format and list size. Good isolation.

Test record naming: _test-basic and _test-ds prefixes follow the _test- naming convention for test isolation.

HCL configs: All configs use empty provider "godaddy" {} blocks (auth via env vars) per conventions.

IP addresses: Tests use 192.0.2.1 and 192.0.2.2 which are in RFC 5737 TEST-NET-1 range -- good practice for test fixtures.

go.mod: terraform-plugin-testing v1.16.0 added as a direct dependency. terraform-plugin-go v0.31.0 promoted from indirect to direct (needed for tfprotov6 type in provider_test.go). All other additions are transitive indirect dependencies.

BLOCKERS

None.

NITS

  1. Conventions doc drift (non-blocking, track separately): docs/conventions.md line 129 shows providerserver.NewProtocol6WithError(New("test")) but this would not compile. The correct invocation is New("test")() as the PR implements. Consider a follow-up fix to the conventions doc so future contributors are not confused.

  2. Data source test does not verify individual record attributes: TestAccDNSRecords_basic checks the id format and records list size, but does not assert on the actual record content within the list (e.g., data, TTL). Consider adding a knownvalue.ListExact or index-based path check on records[0].data to verify the data source maps fields correctly. This is not a blocker because the resource test already validates the underlying CRUD, but it would strengthen data source-specific coverage.

  3. No negative/edge-case test for invalid domain: An optional improvement would be a test that verifies provider error handling when given a nonexistent domain. Not required for the initial acceptance test suite.

  4. PreCheck message wording inconsistency: testAccPreCheck uses "must be set for acceptance tests" while the conventions doc example shows just "must be set". The PR version is actually more informative -- the conventions doc could be updated to match.

SOP COMPLIANCE

  • PR body has Summary, Changes, Test Plan, Related sections
  • PR body includes Review Checklist
  • No secrets, .env files, or credentials committed
  • No hardcoded API keys or tokens (env vars used throughout)
  • Test domain parameterized via GODADDY_TEST_DOMAIN env var
  • No unnecessary file changes (all 5 changed files are directly relevant)
  • Commit message is descriptive (PR title matches purpose)
  • Tests gated by TF_ACC=1 (handled automatically by resource.Test)

PROCESS OBSERVATIONS

  • This PR closes issue #13 and completes the acceptance test layer for the DNS record resource, which was the primary gap after PRs #30 (resource implementation) and #28 (data source). The provider now has both unit-level client tests (pkg/godaddy/*_test.go) and provider-level acceptance tests.
  • The acceptance test coverage map (create, read, update, delete, import) is comprehensive for a first pass. Future PRs could add edge cases (MX/SRV record types with priority/weight/port, CNAME trailing dot normalization) but those belong in separate issues.
  • The conventions doc discrepancy (New("test") vs New("test")()) should be tracked as a follow-up to prevent confusion.

VERDICT: APPROVED

## PR #35 Review ### DOMAIN REVIEW **Tech stack:** Go / Terraform Plugin Framework / terraform-plugin-testing v1.16.0 This PR adds acceptance tests for the `godaddy_dns_record` resource and `godaddy_dns_records` data source. Three new test files follow the canonical pattern for Plugin Framework provider acceptance testing. **Provider factory (`provider_test.go:15`):** ```go "godaddy": providerserver.NewProtocol6WithError(New("test")()), ``` The PR uses `New("test")()` which is **correct** -- `New` returns `func() provider.Provider`, so calling it twice (once to get the factory, once to invoke it) produces the `provider.Provider` value that `NewProtocol6WithError` expects. Note that `docs/conventions.md` line 129 shows `New("test")` (without the second invocation), which would fail to compile due to type mismatch. The conventions doc should be corrected in a follow-up, but the PR code is right. **Test coverage analysis:** | CRUD phase | Covered? | How | |------------|----------|-----| | Create | Yes | Step 1: creates A record, verifies all 6 attributes | | Read | Yes | Implicit in state checks after create/update; also exercised by data source test | | Update | Yes | Step 2: updates TTL from 600 to 3600, confirms change + stability of other fields | | Delete | Yes | `CheckDestroy` calls GoDaddy API directly to confirm deletion | | Import | Yes | Step 3: imports by composite ID `domain:A:_test-basic`, verifies full state round-trip | **statecheck usage:** All assertions use the modern `statecheck.ExpectKnownValue` + `knownvalue` + `tfjsonpath` API per conventions. No deprecated `resource.TestCheckResourceAttr` usage. **CheckDestroy (`resource_dns_record_test.go:74-101`):** Correctly creates a direct API client with env var credentials and queries GoDaddy for each `godaddy_dns_record` resource in state. API errors (including 404) are treated as successful destruction. Non-empty record list returns an error. This matches the conventions doc pattern exactly. **Data source test (`data_source_dns_records_test.go`):** Self-contained -- creates a resource first, then reads it back via the data source with `depends_on`. Verifies computed ID format and list size. Good isolation. **Test record naming:** `_test-basic` and `_test-ds` prefixes follow the `_test-` naming convention for test isolation. **HCL configs:** All configs use empty `provider "godaddy" {}` blocks (auth via env vars) per conventions. **IP addresses:** Tests use `192.0.2.1` and `192.0.2.2` which are in RFC 5737 TEST-NET-1 range -- good practice for test fixtures. **go.mod:** `terraform-plugin-testing` v1.16.0 added as a direct dependency. `terraform-plugin-go` v0.31.0 promoted from indirect to direct (needed for `tfprotov6` type in `provider_test.go`). All other additions are transitive indirect dependencies. ### BLOCKERS None. ### NITS 1. **Conventions doc drift (non-blocking, track separately):** `docs/conventions.md` line 129 shows `providerserver.NewProtocol6WithError(New("test"))` but this would not compile. The correct invocation is `New("test")()` as the PR implements. Consider a follow-up fix to the conventions doc so future contributors are not confused. 2. **Data source test does not verify individual record attributes:** `TestAccDNSRecords_basic` checks the `id` format and `records` list size, but does not assert on the actual record content within the list (e.g., data, TTL). Consider adding a `knownvalue.ListExact` or index-based path check on `records[0].data` to verify the data source maps fields correctly. This is not a blocker because the resource test already validates the underlying CRUD, but it would strengthen data source-specific coverage. 3. **No negative/edge-case test for invalid domain:** An optional improvement would be a test that verifies provider error handling when given a nonexistent domain. Not required for the initial acceptance test suite. 4. **PreCheck message wording inconsistency:** `testAccPreCheck` uses "must be set for acceptance tests" while the conventions doc example shows just "must be set". The PR version is actually more informative -- the conventions doc could be updated to match. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] PR body includes Review Checklist - [x] No secrets, `.env` files, or credentials committed - [x] No hardcoded API keys or tokens (env vars used throughout) - [x] Test domain parameterized via `GODADDY_TEST_DOMAIN` env var - [x] No unnecessary file changes (all 5 changed files are directly relevant) - [x] Commit message is descriptive (PR title matches purpose) - [x] Tests gated by `TF_ACC=1` (handled automatically by `resource.Test`) ### PROCESS OBSERVATIONS - This PR closes issue #13 and completes the acceptance test layer for the DNS record resource, which was the primary gap after PRs #30 (resource implementation) and #28 (data source). The provider now has both unit-level client tests (`pkg/godaddy/*_test.go`) and provider-level acceptance tests. - The acceptance test coverage map (create, read, update, delete, import) is comprehensive for a first pass. Future PRs could add edge cases (MX/SRV record types with priority/weight/port, CNAME trailing dot normalization) but those belong in separate issues. - The conventions doc discrepancy (`New("test")` vs `New("test")()`) should be tracked as a follow-up to prevent confusion. ### VERDICT: APPROVED
ldraney deleted branch 13-acceptance-tests 2026-06-14 19:47:15 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/godaddy-tofu!35
No description provided.