Reduce technical debt of code duplication in automated tests (#69)

* makefile refactor for help. Cleanup. Seeing a panic

* group host inventory job test refactoring

lint

* refactoring: group host inventory job test. Makefile Adjustments

* remove changes to client.go and other review comments.

Co-authored-by: Dan Leehr <[email protected]>

---------

Co-authored-by: Dan Leehr <[email protected]>

Author: arrestle

.gitignore

@@ -38,3 +38,5 @@ config.tfrc
 
 .env
 .DS_Store
+.tools
+.vscode

Makefile

@@ -1,32 +1,33 @@
+include makefiles/golangci.mk
+
 .PHONY: build test lint
 
+
 default: build
 
-build:
+build: ## Compile the Go package. This is the default.
 	@echo "==> Building package..."
 	go build
 
-lint:
-	@echo "==> Checking source code against linters..."
-	golangci-lint run -v ./...
+clean: ## Remove the .tools folder and binaries.
+	@rm -rf .tools
+	@rm terraform-provider-aap
 
-test:
+test: ## Execute all unit tests with verbose output.
 	@echo "==> Running unit tests..."
 	go test -v ./...
 
-testacc:
-	@echo "==> Running acceptance tests..."
-	TF_ACC=1 AAP_HOST="https://localhost:8043" AAP_INSECURE_SKIP_VERIFY=true go test -count=1 -v ./...
-
-testacc-aapdev:
+testacc: ## Run Acceptance tests against aap instance (See README.md for env variables)
 	@echo "==> Running acceptance tests..."
-	TF_ACC=1 AAP_HOST="http://localhost:9080" go test -count=1 -v ./...
-
-gofmt:
-	@echo "==> Format code using gofmt..."
-	gofmt -s -w internal/provider
+	TF_ACC=1 go test -count=1 -v ./...
 
-generatedocs:
+generatedocs: ## Format example Terraform configurations and generate plugin documentation.
 	@echo "==> Formatting examples and generating docs..."
 	terraform fmt -recursive ./examples/
 	go run github.com/hashicorp/terraform-plugin-docs/cmd/tfplugindocs generate
+
+.PHONY: help
+help: ## Show this help message
+	@grep -hE '^[a-zA-Z0-9._-]+:.*?##' $(MAKEFILE_LIST) | \
+	awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-16s\033[0m %s\n", $$1, $$2}' | \
+	sort

README.md

@@ -44,9 +44,13 @@ Acceptance tests apply test terraform configurations to a running AAP instance a
 
 To run acceptance tests locally, start a local AAP instance following the [docker-compose instructions for local AWX development](https://github.com/ansible/awx/blob/devel/tools/docker-compose/README.md). Create an admin user for the AAP instance and save the credentials to these environment variables:
 
+Create an admin user for the AAP instance and set the following environment variables:
+
 ```bash
 export AAP_USERNAME=<your admin username>
 export AAP_PASSWORD=<your admin password>
+export AAP_INSECURE_SKIP_VERIFY=true 
+export AAP_HOST=<your aap instance host url> # "http://localhost:9080" or "https://localhost:8043" 
 ```
 
 In order to run the acceptance tests for the job resource, you must have a working job template already in your AAP instance. The job template must be set to require an inventory on launch. Export the id of this job template:

internal/provider/group_resource_test.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
-	"regexp"
 	"strings"
 	"testing"
 
@@ -35,14 +34,13 @@ func TestGroupResourceSchema(t *testing.T) {
 
 	// Validate the schema
 	diagnostics := schemaResponse.Schema.ValidateImplementation(ctx)
-
 	if diagnostics.HasError() {
 		t.Fatalf("Schema validation diagnostics: %+v", diagnostics)
 	}
 }
 
 func TestGroupResourceCreateRequestBody(t *testing.T) {
-	var testTable = []struct {
+	testCases := []struct {
 		name     string
 		input    GroupResourceModel
 		expected []byte
@@ -97,14 +95,14 @@ func TestGroupResourceCreateRequestBody(t *testing.T) {
 		},
 	}
 
-	for _, test := range testTable {
-		t.Run(test.name, func(t *testing.T) {
-			actual, diags := test.input.CreateRequestBody()
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			actual, diags := testCase.input.CreateRequestBody()
 			if diags.HasError() {
 				t.Fatal(diags.Errors())
 			}
-			if !bytes.Equal(test.expected, actual) {
-				t.Errorf("Expected (%s) not equal to actual (%s)", test.expected, actual)
+			if !bytes.Equal(testCase.expected, actual) {
+				t.Errorf("Expected (%s) not equal to actual (%s)", testCase.expected, actual)
 			}
 		})
 	}
@@ -114,7 +112,15 @@ func TestGroupResourceParseHttpResponse(t *testing.T) {
 	jsonError := diag.Diagnostics{}
 	jsonError.AddError("Error parsing JSON response from AAP", "invalid character 'N' looking for beginning of value")
 
-	var testTable = []struct {
+	const groupJSON = `{
+		"inventory": 1,
+		"description": "A basic test group",
+		"name": "group1",
+		"url": "/api/v2/groups/1/",
+		"variables": "{\"foo\":\"bar\",\"nested\":{\"foobar\":\"baz\"}}"
+	}`
+
+	testCases := []struct {
 		name     string
 		input    []byte
 		expected GroupResourceModel
@@ -139,9 +145,8 @@ func TestGroupResourceParseHttpResponse(t *testing.T) {
 			errors: diag.Diagnostics{},
 		},
 		{
-			name: "test with all values",
-			input: []byte(`{"inventory":1,"description":"A basic test group","name":"group1","url":"/api/v2/groups/1/",` +
-				`"variables":"{\"foo\":\"bar\",\"nested\":{\"foobar\":\"baz\"}}"}`),
+			name:  "test with all values",
+			input: []byte(groupJSON),
 			expected: GroupResourceModel{
 				InventoryId: types.Int64Value(1),
 				Id:          types.Int64Value(0),
@@ -154,15 +159,15 @@ func TestGroupResourceParseHttpResponse(t *testing.T) {
 		},
 	}
 
-	for _, test := range testTable {
-		t.Run(test.name, func(t *testing.T) {
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
 			resource := GroupResourceModel{}
-			diags := resource.ParseHttpResponse(test.input)
-			if !test.errors.Equal(diags) {
-				t.Errorf("Expected error diagnostics (%s), actual was (%s)", test.errors, diags)
+			diags := resource.ParseHttpResponse(testCase.input)
+			if !testCase.errors.Equal(diags) {
+				t.Errorf("Expected error diagnostics (%s), actual was (%s)", testCase.errors, diags)
 			}
-			if !reflect.DeepEqual(test.expected, resource) {
-				t.Errorf("Expected (%s) not equal to actual (%s)", test.expected, resource)
+			if !reflect.DeepEqual(testCase.expected, resource) {
+				t.Errorf("Expected (%s) not equal to actual (%s)", testCase.expected, resource)
 			}
 		})
 	}
@@ -172,42 +177,38 @@ func TestGroupResourceParseHttpResponse(t *testing.T) {
 
 func TestAccGroupResource(t *testing.T) {
 	var groupApiModel GroupAPIModel
-	var description = "A test group"
-	var variables = "{\"foo\": \"bar\"}"
 	inventoryName := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
 	groupName := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
 	updatedName := "updated" + groupName
+	description := "A test group"
+	variables := "{\"foo\": \"bar\"}"
 
 	resource.Test(t, resource.TestCase{
 		PreCheck:                 func() { testAccPreCheck(t) },
 		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
 		Steps: []resource.TestStep{
-			// Invalid variables testing
+			// Invalid variables testing.
 			{
 				Config:      testAccGroupResourceBadVariables(inventoryName, updatedName),
-				ExpectError: regexp.MustCompile("Input type `str` is not a dictionary"),
+				ExpectError: reInvalidVars,
 			},
-			// Create and Read testing
 			{
 				Config: testAccGroupResourceMinimal(inventoryName, groupName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckGroupResourceExists("aap_group.test", &groupApiModel),
+					testAccCheckGroupResourceExists(resourceNameGroup, &groupApiModel),
 					testAccCheckGroupResourceValues(&groupApiModel, groupName, "", ""),
-					resource.TestCheckResourceAttr("aap_group.test", "name", groupName),
-					resource.TestCheckResourceAttrPair("aap_group.test", "inventory_id", "aap_inventory.test", "id"),
-					resource.TestMatchResourceAttr("aap_group.test", "url", regexp.MustCompile("^/api(/controller)?/v2/groups/[0-9]*/$")),
+					checkBasicGroupAttributes(t, resourceNameGroup, groupName),
 				),
 			},
 			{
 				Config: testAccGroupResourceComplete(inventoryName, updatedName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckGroupResourceExists("aap_group.test", &groupApiModel),
+					testAccCheckGroupResourceExists(resourceNameGroup, &groupApiModel),
 					testAccCheckGroupResourceValues(&groupApiModel, updatedName, description, variables),
-					resource.TestCheckResourceAttr("aap_group.test", "name", updatedName),
-					resource.TestCheckResourceAttrPair("aap_group.test", "inventory_id", "aap_inventory.test", "id"),
-					resource.TestCheckResourceAttr("aap_group.test", "description", description),
-					resource.TestCheckResourceAttr("aap_group.test", "variables", variables),
-					resource.TestMatchResourceAttr("aap_group.test", "url", regexp.MustCompile("^/api(/controller)?/v2/groups/[0-9]*/$")),
+					checkBasicGroupAttributes(t, resourceNameGroup, updatedName),
+					resource.TestCheckResourceAttr(resourceNameGroup, "name", updatedName),
+					resource.TestCheckResourceAttr(resourceNameGroup, "description", description),
+					resource.TestCheckResourceAttr(resourceNameGroup, "variables", variables),
 				),
 			},
 		},

internal/provider/host_resource_test.go

@@ -277,28 +277,20 @@ func TestAccHostResource(t *testing.T) {
 			{
 				Config: testAccHostResourceMinimal(inventoryName, hostName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckHostResourceExists("aap_host.test", &hostApiModel),
+					checkBasicHostAttributes(t, resourceNameHost, hostName),
+					testAccCheckHostResourceExists(resourceNameHost, &hostApiModel),
 					testAccCheckHostResourceValues(&hostApiModel, hostName, "", ""),
-					resource.TestCheckResourceAttr("aap_host.test", "name", hostName),
-					resource.TestCheckResourceAttrPair("aap_host.test", "inventory_id", "aap_inventory.test", "id"),
-					resource.TestCheckResourceAttr("aap_host.test", "enabled", "true"),
-					resource.TestMatchResourceAttr("aap_host.test", "url", regexp.MustCompile("^/api(/controller)?/v2/hosts/[0-9]*/$")),
-					resource.TestCheckResourceAttrSet("aap_host.test", "id"),
 				),
 			},
 			// Update and Read testing
 			{
 				Config: testAccHostResourceComplete(inventoryName, groupName, updatedName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckHostResourceExists("aap_host.test", &hostApiModel),
+					testAccCheckHostResourceExists(resourceNameHost, &hostApiModel),
 					testAccCheckHostResourceValues(&hostApiModel, updatedName, updatedDescription, updatedVariables),
-					resource.TestCheckResourceAttr("aap_host.test", "name", updatedName),
-					resource.TestCheckResourceAttrPair("aap_host.test", "inventory_id", "aap_inventory.test", "id"),
-					resource.TestCheckResourceAttr("aap_host.test", "description", updatedDescription),
-					resource.TestCheckResourceAttr("aap_host.test", "variables", updatedVariables),
-					resource.TestCheckResourceAttr("aap_host.test", "enabled", "true"),
-					resource.TestMatchResourceAttr("aap_host.test", "url", regexp.MustCompile("^/api(/controller)?/v2/hosts/[0-9]*/$")),
-					resource.TestCheckResourceAttrSet("aap_host.test", "id"),
+					checkBasicHostAttributes(t, resourceNameHost, updatedName),
+					resource.TestCheckResourceAttr(resourceNameHost, "description", updatedDescription),
+					resource.TestCheckResourceAttr(resourceNameHost, "variables", updatedVariables),
 				),
 			},
 		},

internal/provider/inventory_data_source_test.go

@@ -110,11 +110,11 @@ func TestAccInventoryDataSource(t *testing.T) {
 			{
 				Config: testAccInventoryDataSource(randomName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					resource.TestCheckResourceAttrPair("aap_inventory.test", "name", "data.aap_inventory.test", "name"),
-					resource.TestCheckResourceAttrPair("aap_inventory.test", "organization", "data.aap_inventory.test", "organization"),
-					resource.TestCheckResourceAttrPair("aap_inventory.test", "description", "data.aap_inventory.test", "description"),
-					resource.TestCheckResourceAttrPair("aap_inventory.test", "variables", "data.aap_inventory.test", "variables"),
-					resource.TestCheckResourceAttrPair("aap_inventory.test", "url", "data.aap_inventory.test", "url"),
+					resource.TestCheckResourceAttrPair(resourceNameInventory, "name", "data.aap_inventory.test", "name"),
+					resource.TestCheckResourceAttrPair(resourceNameInventory, "organization", "data.aap_inventory.test", "organization"),
+					resource.TestCheckResourceAttrPair(resourceNameInventory, "description", "data.aap_inventory.test", "description"),
+					resource.TestCheckResourceAttrPair(resourceNameInventory, "variables", "data.aap_inventory.test", "variables"),
+					resource.TestCheckResourceAttrPair(resourceNameInventory, "url", "data.aap_inventory.test", "url"),
 				),
 			},
 		},

internal/provider/inventory_resource_test.go

@@ -186,29 +186,20 @@ func TestAccInventoryResource(t *testing.T) {
 			{
 				Config: testAccInventoryResourceMinimal(randomName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckInventoryResourceExists("aap_inventory.test", &inventory),
+					testAccCheckInventoryResourceExists(resourceNameInventory, &inventory),
 					testAccCheckInventoryResourceValues(&inventory, randomName, "", ""),
-					resource.TestCheckResourceAttr("aap_inventory.test", "name", randomName),
-					resource.TestCheckResourceAttr("aap_inventory.test", "organization", "1"),
-					resource.TestCheckResourceAttr("aap_inventory.test", "organization_name", "Default"),
-					resource.TestCheckResourceAttrSet("aap_inventory.test", "named_url"),
-					resource.TestCheckResourceAttrSet("aap_inventory.test", "id"),
-					resource.TestCheckResourceAttrSet("aap_inventory.test", "url"),
+					checkBasicInventoryAttributes(t, resourceNameInventory, randomName, "1", "Default"),
 				),
 			},
 			// Update and Read testing
 			{
 				Config: testAccInventoryResourceComplete(updatedName),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckInventoryResourceExists("aap_inventory.test", &inventory),
+					testAccCheckInventoryResourceExists(resourceNameInventory, &inventory),
 					testAccCheckInventoryResourceValues(&inventory, updatedName, updatedDescription, updatedVariables),
-					resource.TestCheckResourceAttr("aap_inventory.test", "name", updatedName),
-					resource.TestCheckResourceAttr("aap_inventory.test", "organization", "1"),
-					resource.TestCheckResourceAttr("aap_inventory.test", "description", updatedDescription),
-					resource.TestCheckResourceAttr("aap_inventory.test", "variables", updatedVariables),
-					resource.TestCheckResourceAttr("aap_inventory.test", "named_url", fmt.Sprintf("/api/controller/v2/inventories/%s++%s/", updatedName, "Default")),
-					resource.TestCheckResourceAttrSet("aap_inventory.test", "id"),
-					resource.TestCheckResourceAttrSet("aap_inventory.test", "url"),
+					checkBasicInventoryAttributes(t, resourceNameInventory, updatedName, "1", "Default"),
+					resource.TestCheckResourceAttr(resourceNameInventory, "description", updatedDescription),
+					resource.TestCheckResourceAttr(resourceNameInventory, "variables", updatedVariables),
 				),
 			},
 		},