tests/provider: Fixes for testing provider re-configuration

aws/provider_test.go

@@ -9,6 +9,7 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -76,14 +77,25 @@ var testAccProviderFactories map[string]func() (*schema.Provider, error)
 // testAccPreCheck(t) must be called before using this provider instance.
 var testAccProvider *schema.Provider
 
+// testAccProviderConfigure ensures testAccProvider is only configured once
+//
+// The testAccPreCheck(t) function is invoked for every test and this prevents
+// extraneous reconfiguration to the same values each time. However, this does
+// not prevent reconfiguration that may happen should the address of
+// testAccProvider be errantly reused in ProviderFactories.
+var testAccProviderConfigure sync.Once
+
 func init() {
 	testAccProvider = Provider()
 
 	testAccProviders = map[string]*schema.Provider{
 		ProviderNameAws: testAccProvider,
 	}
+
+	// Always allocate a new provider instance each invocation, otherwise gRPC
+	// ProviderConfigure() can overwrite configuration during concurrent testing.
 	testAccProviderFactories = map[string]func() (*schema.Provider, error){
-		ProviderNameAws: func() (*schema.Provider, error) { return testAccProvider, nil },
+		ProviderNameAws: func() (*schema.Provider, error) { return Provider(), nil }, //nolint:unparam
 	}
 }
 
@@ -166,23 +178,41 @@ func TestProvider_impl(t *testing.T) {
 	var _ *schema.Provider = Provider()
 }
 
+// testAccPreCheck verifies and sets required provider testing configuration
+//
+// This PreCheck function should be present in every acceptance test. It allows
+// test configurations to omit a provider configuration with region and ensures
+// testing functions that attempt to call AWS APIs are previously configured.
+//
+// These verifications and configuration are preferred at this level to prevent
+// provider developers from experiencing less clear errors for every test.
 func testAccPreCheck(t *testing.T) {
-	if os.Getenv("AWS_PROFILE") == "" && os.Getenv("AWS_ACCESS_KEY_ID") == "" {
-		t.Fatal("AWS_ACCESS_KEY_ID or AWS_PROFILE must be set for acceptance tests")
-	}
-
-	if os.Getenv("AWS_ACCESS_KEY_ID") != "" && os.Getenv("AWS_SECRET_ACCESS_KEY") == "" {
-		t.Fatal("AWS_SECRET_ACCESS_KEY must be set for acceptance tests")
-	}
+	// Since we are outside the scope of the Terraform configuration we must
+	// call Configure() to properly initialize the provider configuration.
+	testAccProviderConfigure.Do(func() {
+		if os.Getenv("AWS_PROFILE") == "" && os.Getenv("AWS_ACCESS_KEY_ID") == "" {
+			t.Fatal("AWS_ACCESS_KEY_ID or AWS_PROFILE must be set for acceptance tests")
+		}
 
-	region := testAccGetRegion()
-	log.Printf("[INFO] Test: Using %s as test region", region)
-	os.Setenv("AWS_DEFAULT_REGION", region)
+		if os.Getenv("AWS_ACCESS_KEY_ID") != "" && os.Getenv("AWS_SECRET_ACCESS_KEY") == "" {
+			t.Fatal("AWS_SECRET_ACCESS_KEY must be set for acceptance tests")
+		}
 
-	err := testAccProvider.Configure(context.Background(), terraform.NewResourceConfigRaw(nil))
-	if err != nil {
-		t.Fatal(err)
-	}
+		// Setting the AWS_DEFAULT_REGION environment variable here allows all tests to omit
+		// a provider configuration with a region. This defaults to us-west-2 for provider
+		// developer simplicity and has been in the codebase for a very long time.
+		//
+		// This handling must be preserved until either:
+		//   * AWS_DEFAULT_REGION is required and checked above (should mention us-west-2 default)
+		//   * Region is automatically handled via shared AWS configuration file and still verified
+		region := testAccGetRegion()
+		os.Setenv("AWS_DEFAULT_REGION", region)
+
+		err := testAccProvider.Configure(context.Background(), terraform.NewResourceConfigRaw(nil))
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
 }
 
 // testAccAwsProviderAccountID returns the account ID of an AWS provider