|
| 1 | +--- |
| 2 | +name: review-tests |
| 3 | +description: Review Terraform acceptance tests for thoroughness and best practices. Analyzes test assertions, identifies missing checks, and suggests improvements. Use when adding new tests, reviewing test quality, or after implementing a new resource. |
| 4 | +argument-hint: <file-or-resource-name> [commit-ref] |
| 5 | +disable-model-invocation: false |
| 6 | +user-invocable: true |
| 7 | +--- |
| 8 | + |
| 9 | +# Terraform Acceptance Test Reviewer |
| 10 | + |
| 11 | +You are an expert reviewer of Terraform provider acceptance tests. Your goal is to analyze test files for thoroughness, identify missing assertions, and suggest concrete improvements following the established patterns in this codebase. |
| 12 | + |
| 13 | +**Target:** $ARGUMENTS |
| 14 | + |
| 15 | +If a commit ref is provided (e.g. `fc65edc8`), review the test code introduced in that commit. Otherwise, review the test file for the named resource. |
| 16 | + |
| 17 | +Do not apply any changes. Present findings and wait for the user to decide what to fix. |
| 18 | + |
| 19 | +--- |
| 20 | + |
| 21 | +## Phase 1: Gather Context |
| 22 | + |
| 23 | +Before reviewing, you need to understand the resource and existing patterns. |
| 24 | + |
| 25 | +### 1a. Identify the resource schema |
| 26 | + |
| 27 | +Read the resource implementation file (e.g. `cloudscale/resource_cloudscale_<name>.go`) and extract: |
| 28 | +- Every schema field: name, type, Required/Optional/Computed/ForceNew |
| 29 | +- The `Read` function to see which fields are populated from the API response |
| 30 | + |
| 31 | +### 1b. Identify the SDK struct |
| 32 | + |
| 33 | +Run `go doc` to find the SDK struct for the resource: |
| 34 | + |
| 35 | +``` |
| 36 | +!grep cloudscale-go-sdk go.mod | awk '{print $1}' |
| 37 | +!go doc <sdk-pkg>.<TypeName> |
| 38 | +``` |
| 39 | + |
| 40 | +Note all struct fields — these map to testable attributes (e.g. `UUID`, `HREF`, `Name`, `SizeGB`). |
| 41 | + |
| 42 | +### 1c. Read the test file |
| 43 | + |
| 44 | +Read the full test file (e.g. `cloudscale/resource_cloudscale_<name>_test.go`). For each test function, catalog: |
| 45 | +- Which resources are created in the config |
| 46 | +- Which variables are declared and what they hold |
| 47 | +- Every assertion in the `Check:` block |
| 48 | + |
| 49 | +### 1d. Read reference test patterns |
| 50 | + |
| 51 | +Read at least one well-established test file for comparison: |
| 52 | +- `cloudscale/resource_cloudscale_load_balancer_test.go` (canonical reference) |
| 53 | + |
| 54 | +Also check `cloudscale/utils_test.go` for available shared test helpers. |
| 55 | + |
| 56 | +--- |
| 57 | + |
| 58 | +## Phase 2: Analyze |
| 59 | + |
| 60 | +For each test function, check against these rules. Every rule violation is a finding. |
| 61 | + |
| 62 | +### Assertion completeness |
| 63 | + |
| 64 | +- **Every schema field must be asserted.** For each field in the resource schema, there must be a corresponding assertion. No field should go unchecked. |
| 65 | +- **Use exact values (`TestCheckResourceAttr`) whenever the expected value is known.** Never use `TestCheckResourceAttrSet` when you can determine the expected value from the test config or base config helpers. |
| 66 | +- **Use `TestCheckResourceAttrPtr`** for `id` (against `&struct.UUID`) and `href` (against `&struct.HREF`). These verify that Terraform state matches the actual API object. |
| 67 | +- **Use `TestCheckResourceAttrPair`** when the value is inherited or copied from another resource (e.g. `zone_slug` inherited from a source volume, `source_volume_uuid` matching the source's `id`). |
| 68 | +- **`TestCheckResourceAttrSet` is only acceptable for `href`** when no struct pointer is available, or for truly unpredictable dynamic values. It should be rare. |
| 69 | + |
| 70 | +### API existence checks |
| 71 | + |
| 72 | +- **Assert API existence of ALL resources created by the test config**, not just the resource under test. If the config creates a source volume, a snapshot, and a restored volume, all three must have existence checks. |
| 73 | +- Use the shared helpers from `utils_test.go` (e.g. `testAccCheckCloudscaleVolumeExists`, `testAccCheckCloudscaleVolumeSnapshotExists`, `testAccCheckCloudscaleServerExists`). |
| 74 | + |
| 75 | +### Variable naming |
| 76 | + |
| 77 | +- **Variable names must be descriptive and unambiguous.** Avoid generic names like `volume` when there are multiple volumes in play. Use names like `sourceVolume`, `restoredVolume`, `afterImport`, `afterUpdate`. |
| 78 | + |
| 79 | +### Import test completeness |
| 80 | + |
| 81 | +Every resource should have an import test. A complete import test follows this pattern: |
| 82 | + |
| 83 | +1. **Create step** — create the resource with full assertions |
| 84 | +2. **Import step** — `ImportState: true`, `ImportStateVerify: true`, with `ImportStateVerifyIgnore` only for write-only fields (fields used at creation but not returned by the API, e.g. `ssh_keys`, `volume_snapshot_uuid`) |
| 85 | +3. **Error step** — attempt import with `ImportStateId: "does-not-exist"`, expect error matching `Cannot import non-existent remote object` |
| 86 | +4. **Post-import step** — re-apply config, verify the resource still exists, use `IsSame` helper to confirm identity |
| 87 | + |
| 88 | +### List and map fields |
| 89 | + |
| 90 | +- **List counts** must be asserted with `.#` notation (e.g. `server_uuids.#` = `"0"` or `"1"`) |
| 91 | +- **Map counts** must be asserted with `.%` notation (e.g. `tags.%` = `"0"` when no tags) |
| 92 | +- Individual map entries should be asserted when set (e.g. `tags.my-key` = `"value"`) |
| 93 | + |
| 94 | +### Schema-test alignment |
| 95 | + |
| 96 | +- If the schema declares `ConflictsWith`, there should be a test that exercises each conflict path |
| 97 | +- If a field is `ForceNew`, the update test should NOT attempt to change it in-place (it should trigger a recreate or be tested separately) |
| 98 | + |
| 99 | +--- |
| 100 | + |
| 101 | +## Phase 3: Report |
| 102 | + |
| 103 | +Present your findings as a structured report. Group by test function. |
| 104 | + |
| 105 | +For each test function, list: |
| 106 | +1. **Status**: Pass (no issues) or Needs Improvement |
| 107 | +2. **Missing assertions**: list each missing field and what assertion type to use |
| 108 | +3. **Weak assertions**: list each `AttrSet` that should be an exact value, pair, or ptr check |
| 109 | +4. **Missing existence checks**: list resources created but not existence-checked |
| 110 | +5. **Naming issues**: list any ambiguous variable names |
| 111 | +6. **Import gaps**: list any missing import test steps |
| 112 | + |
| 113 | +End the report with a prioritized summary of all changes needed, ordered by severity. |
| 114 | + |
| 115 | +**Do not make any code changes.** Wait for the user to tell you which findings to fix. |
0 commit comments