Vcr ignore tests (#3486) (#6375)

* Missed acctest.Randoms, add support for skipIfVcr * Add skip_vcr to tests, more fixes for failing vcr tests * Fix compile * Comment on why / path replace is needed * Rubocop line length * Ignores for instance template tests with generated ids, multiple fine-grained resources in single step * Add more ignores, rework VCR file writing to only write when test succeeds * Add notes on why specific tests fail VCR * Spaces... Signed-off-by: Modular Magician <[email protected]>
hashicorp · May 14, 2020 · 6b537c0 · 6b537c0
1 parent 0c45905
commit 6b537c0
Show file tree

Hide file tree

Showing 35 changed files with 155 additions and 39 deletions.
diff --git a/.changelog/3486.txt b/.changelog/3486.txt
@@ -0,0 +1,2 @@
+```release-note:none
+```
diff --git a/google/bootstrap_utils_test.go b/google/bootstrap_utils_test.go
@@ -8,7 +8,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/terraform-plugin-sdk/helper/acctest"
 	"google.golang.org/api/cloudkms/v1"
 	cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1"
 	"google.golang.org/api/iam/v1"
@@ -329,7 +328,7 @@ func BootstrapServicePerimeterProjects(t *testing.T, desiredProjects int) []*clo
 
 	projects := res.Projects
 	for len(projects) < desiredProjects {
-		pid := SharedServicePerimeterProjectPrefix + acctest.RandString(10)
+		pid := SharedServicePerimeterProjectPrefix + randString(t, 10)
 		project := &cloudresourcemanager.Project{
 			ProjectId: pid,
 			Name:      "TF Service Perimeter Test",

diff --git a/google/data_source_compute_network_endpoint_group_test.go b/google/data_source_compute_network_endpoint_group_test.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/hashicorp/terraform-plugin-sdk/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/terraform"
 )
@@ -13,7 +12,7 @@ func TestAccDataSourceComputeNetworkEndpointGroup(t *testing.T) {
 	t.Parallel()
 
 	context := map[string]interface{}{
-		"random_suffix": acctest.RandString(10),
+		"random_suffix": randString(t, 10),
 	}
 
 	vcrTest(t, resource.TestCase{

diff --git a/google/data_source_google_compute_region_instance_group_test.go b/google/data_source_google_compute_region_instance_group_test.go
@@ -8,6 +8,8 @@ import (
 )
 
 func TestAccDataSourceRegionInstanceGroup(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 	name := "acctest-" + randString(t, 6)
 	vcrTest(t, resource.TestCase{

diff --git a/google/metadata.go b/google/metadata.go
@@ -102,9 +102,14 @@ func BetaMetadataUpdate(oldMDMap map[string]interface{}, newMDMap map[string]int
 
 func expandComputeMetadata(m map[string]interface{}) []*compute.MetadataItems {
 	metadata := make([]*compute.MetadataItems, len(m))
+	var keys []string
+	for key := range m {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
 	// Append new metadata to existing metadata
-	for key, val := range m {
-		v := val.(string)
+	for _, key := range keys {
+		v := m[key].(string)
 		metadata = append(metadata, &compute.MetadataItems{
 			Key:   key,
 			Value: &v,

diff --git a/google/provider.go b/google/provider.go
@@ -773,7 +773,7 @@ func ResourceMapWithErrors() (map[string]*schema.Resource, error) {
 			"google_dns_record_set":                        resourceDnsRecordSet(),
 			"google_endpoints_service":                     resourceEndpointsService(),
 			"google_folder":                                resourceGoogleFolder(),
-			"google_folder_iam_binding":                    ResourceIamBindingWithBatching(IamFolderSchema, NewFolderIamUpdater, FolderIdParseFunc, IamBatchingEnabled),
+			"google_folder_iam_binding":                    ResourceIamBinding(IamFolderSchema, NewFolderIamUpdater, FolderIdParseFunc),
 			"google_folder_iam_member":                     ResourceIamMember(IamFolderSchema, NewFolderIamUpdater, FolderIdParseFunc),
 			"google_folder_iam_policy":                     ResourceIamPolicy(IamFolderSchema, NewFolderIamUpdater, FolderIdParseFunc),
 			"google_folder_organization_policy":            resourceGoogleFolderOrganizationPolicy(),

diff --git a/google/provider_test.go b/google/provider_test.go
@@ -81,11 +81,18 @@ var billingAccountEnvVars = []string{
 }
 
 var configs map[string]*Config
-var sources map[string]rand.Source
+
+// A source for a given VCR test with the value that seeded it
+type VcrSource struct {
+	seed   int64
+	source rand.Source
+}
+
+var sources map[string]VcrSource
 
 func init() {
 	configs = make(map[string]*Config)
-	sources = make(map[string]rand.Source)
+	sources = make(map[string]VcrSource)
 	testAccProvider = Provider().(*schema.Provider)
 	testAccRandomProvider = random.Provider().(*schema.Provider)
 	testAccProviders = map[string]terraform.ResourceProvider{
@@ -127,7 +134,7 @@ func getCachedConfig(d *schema.ResourceData, configureFunc func(d *schema.Resour
 		log.Print("[DEBUG] No environment var set for VCR_PATH, skipping VCR")
 		return config, nil
 	}
-	path := filepath.Join(envPath, testName)
+	path := filepath.Join(envPath, vcrFileName(testName))
 
 	rec, err := recorder.NewAsMode(path, vcrMode, config.client.Transport)
 	if err != nil {
@@ -186,9 +193,19 @@ func getCachedConfig(d *schema.ResourceData, configureFunc func(d *schema.Resour
 func closeRecorder(t *testing.T) {
 	if config, ok := configs[t.Name()]; ok {
 		// We did not cache the config if it does not use VCR
-		err := config.client.Transport.(*recorder.Recorder).Stop()
-		if err != nil {
-			t.Error(err)
+		if !t.Failed() && isVcrEnabled() {
+			// If a test succeeds, write new seed/yaml to files
+			err := config.client.Transport.(*recorder.Recorder).Stop()
+			if err != nil {
+				t.Error(err)
+			}
+			envPath := os.Getenv("VCR_PATH")
+			if vcrSource, ok := sources[t.Name()]; ok {
+				err = writeSeedToFile(vcrSource.seed, vcrSeedFile(envPath, t.Name()))
+				if err != nil {
+					t.Error(err)
+				}
+			}
 		}
 		// Clean up test config
 		delete(configs, t.Name())
@@ -239,32 +256,40 @@ func vcrTest(t *testing.T, c resource.TestCase) {
 	resource.Test(t, c)
 }
 
+// Retrieves a unique test name used for writing files
+// replaces all `/` characters that would cause filepath issues
+// This matters during tests that dispatch multiple tests, for example TestAccLoggingFolderExclusion
+func vcrSeedFile(path, name string) string {
+	return filepath.Join(path, fmt.Sprintf("%s.seed", vcrFileName(name)))
+}
+
+func vcrFileName(name string) string {
+	return strings.ReplaceAll(name, "/", "_")
+}
+
 // Produces a rand.Source for VCR testing based on the given mode.
 // In RECORDING mode, generates a new seed and saves it to a file, using the seed for the source
 // In REPLAYING mode, reads a seed from a file and creates a source from it
-func vcrSource(t *testing.T, path, mode string) (rand.Source, error) {
+func vcrSource(t *testing.T, path, mode string) (*VcrSource, error) {
 	if s, ok := sources[t.Name()]; ok {
-		return s, nil
+		return &s, nil
 	}
-	fileName := filepath.Join(path, fmt.Sprintf("%s.seed", t.Name()))
 	switch mode {
 	case "RECORDING":
 		seed := rand.Int63()
 		s := rand.NewSource(seed)
-		err := writeSeedToFile(seed, fileName)
-		if err != nil {
-			return nil, err
-		}
-		sources[t.Name()] = s
-		return s, nil
+		vcrSource := VcrSource{seed: seed, source: s}
+		sources[t.Name()] = vcrSource
+		return &vcrSource, nil
 	case "REPLAYING":
-		seed, err := readSeedFromFile(fileName)
+		seed, err := readSeedFromFile(vcrSeedFile(path, t.Name()))
 		if err != nil {
 			return nil, err
 		}
 		s := rand.NewSource(seed)
-		sources[t.Name()] = s
-		return s, nil
+		vcrSource := VcrSource{seed: seed, source: s}
+		sources[t.Name()] = vcrSource
+		return &vcrSource, nil
 	default:
 		log.Printf("[DEBUG] No valid environment var set for VCR_MODE, expected RECORDING or REPLAYING, skipping VCR. VCR_MODE: %s", mode)
 		return nil, errors.New("No valid VCR_MODE set")
@@ -314,7 +339,7 @@ func randString(t *testing.T, length int) string {
 		t.Fatal(err)
 	}
 
-	r := rand.New(s)
+	r := rand.New(s.source)
 	result := make([]byte, length)
 	set := "abcdefghijklmnopqrstuvwxyz012346789"
 	for i := 0; i < length; i++ {
@@ -335,7 +360,7 @@ func randInt(t *testing.T) int {
 		t.Fatal(err)
 	}
 
-	return rand.New(s).Int()
+	return rand.New(s.source).Int()
 }
 
 func TestProvider(t *testing.T) {
@@ -422,7 +447,7 @@ func TestAccProviderBasePath_setBasePath(t *testing.T) {
 		CheckDestroy: testAccCheckComputeAddressDestroyProducer(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccProviderBasePath_setBasePath("https://www.googleapis.com/compute/beta/", acctest.RandString(10)),
+				Config: testAccProviderBasePath_setBasePath("https://www.googleapis.com/compute/beta/", randString(t, 10)),
 			},
 			{
 				ResourceName:      "google_compute_address.default",
@@ -442,7 +467,7 @@ func TestAccProviderBasePath_setInvalidBasePath(t *testing.T) {
 		CheckDestroy: testAccCheckComputeAddressDestroyProducer(t),
 		Steps: []resource.TestStep{
 			{
-				Config:      testAccProviderBasePath_setBasePath("https://www.example.com/compute/beta/", acctest.RandString(10)),
+				Config:      testAccProviderBasePath_setBasePath("https://www.example.com/compute/beta/", randString(t, 10)),
 				ExpectError: regexp.MustCompile("got HTTP response code 404 with body"),
 			},
 		},
@@ -454,9 +479,9 @@ func TestAccProviderUserProjectOverride(t *testing.T) {
 
 	org := getTestOrgFromEnv(t)
 	billing := getTestBillingAccountFromEnv(t)
-	pid := acctest.RandomWithPrefix("tf-test")
-	sa := acctest.RandomWithPrefix("tf-test")
-	topicName := "tf-test-topic-" + acctest.RandString(10)
+	pid := "tf-test-" + randString(t, 10)
+	sa := "tf-test-" + randString(t, 10)
+	topicName := "tf-test-topic-" + randString(t, 10)
 
 	vcrTest(t, resource.TestCase{
 		PreCheck:  func() { testAccPreCheck(t) },
@@ -498,8 +523,8 @@ func TestAccProviderIndirectUserProjectOverride(t *testing.T) {
 
 	org := getTestOrgFromEnv(t)
 	billing := getTestBillingAccountFromEnv(t)
-	pid := acctest.RandomWithPrefix("tf-test")
-	sa := acctest.RandomWithPrefix("tf-test")
+	pid := "tf-test-" + randString(t, 10)
+	sa := "tf-test-" + randString(t, 10)
 
 	vcrTest(t, resource.TestCase{
 		PreCheck:  func() { testAccPreCheck(t) },
@@ -839,3 +864,12 @@ func multiEnvSearch(ks []string) string {
 	}
 	return ""
 }
+
+// Some tests fail during VCR. One common case is race conditions when creating resources.
+// If a test config adds two fine-grained resources with the same parent it is undefined
+// which will be created first, causing VCR to fail ~50% of the time
+func skipIfVcr(t *testing.T) {
+	if isVcrEnabled() {
+		t.Skipf("VCR enabled, skipping test: %s", t.Name())
+	}
+}
diff --git a/google/resource_access_context_manager_service_perimeter_resource_test.go b/google/resource_access_context_manager_service_perimeter_resource_test.go
@@ -12,6 +12,8 @@ import (
 // can exist, they need to be ran serially. See AccessPolicy for the test runner.
 
 func testAccAccessContextManagerServicePerimeterResource_basicTest(t *testing.T) {
+	// Multiple fine-grained resources
+	skipIfVcr(t)
 	org := getTestOrgFromEnv(t)
 	projects := BootstrapServicePerimeterProjects(t, 2)
 	policyTitle := "my policy"

diff --git a/google/resource_bigquery_dataset_access_test.go b/google/resource_bigquery_dataset_access_test.go
@@ -69,6 +69,8 @@ func TestAccBigQueryDatasetAccess_view(t *testing.T) {
 }
 
 func TestAccBigQueryDatasetAccess_multiple(t *testing.T) {
+	// Multiple fine-grained resources
+	skipIfVcr(t)
 	t.Parallel()
 
 	datasetID := fmt.Sprintf("tf_test_%s", randString(t, 10))

diff --git a/google/resource_compute_disk_test.go b/google/resource_compute_disk_test.go
@@ -204,6 +204,8 @@ func TestAccComputeDisk_imageDiffSuppressPublicVendorsFamilyNames(t *testing.T)
 }
 
 func TestAccComputeDisk_timeout(t *testing.T) {
+	// Vcr speeds up test, so it doesn't time out
+	skipIfVcr(t)
 	t.Parallel()
 
 	diskName := fmt.Sprintf("tf-test-disk-%d", randInt(t))
@@ -374,6 +376,8 @@ func TestAccComputeDisk_deleteDetach(t *testing.T) {
 }
 
 func TestAccComputeDisk_deleteDetachIGM(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	diskName := fmt.Sprintf("tf-test-%s", randString(t, 10))

diff --git a/google/resource_compute_instance_group_manager_test.go b/google/resource_compute_instance_group_manager_test.go
@@ -96,6 +96,8 @@ func TestAccInstanceGroupManager_update(t *testing.T) {
 }
 
 func TestAccInstanceGroupManager_updateLifecycle(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	tag1 := "tag1"
@@ -128,6 +130,8 @@ func TestAccInstanceGroupManager_updateLifecycle(t *testing.T) {
 }
 
 func TestAccInstanceGroupManager_updatePolicy(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	igm := fmt.Sprintf("igm-test-%s", randString(t, 10))
@@ -175,6 +179,8 @@ func TestAccInstanceGroupManager_updatePolicy(t *testing.T) {
 }
 
 func TestAccInstanceGroupManager_separateRegions(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	igm1 := fmt.Sprintf("igm-test-%s", randString(t, 10))

diff --git a/google/resource_compute_instance_iam_test.go b/google/resource_compute_instance_iam_test.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/hashicorp/terraform-plugin-sdk/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
 )
 
@@ -16,7 +15,7 @@ func TestAccComputeInstanceIamPolicy(t *testing.T) {
 	project := getTestProjectFromEnv()
 	role := "roles/compute.osLogin"
 	zone := getTestZoneFromEnv()
-	instanceName := fmt.Sprintf("tf-test-instance-%s", acctest.RandString(10))
+	instanceName := fmt.Sprintf("tf-test-instance-%s", randString(t, 10))
 
 	vcrTest(t, resource.TestCase{
 		PreCheck:  func() { testAccPreCheck(t) },

diff --git a/google/resource_compute_instance_template_test.go b/google/resource_compute_instance_template_test.go
@@ -511,6 +511,8 @@ func TestAccComputeInstanceTemplate_subnet_custom(t *testing.T) {
 }
 
 func TestAccComputeInstanceTemplate_subnet_xpn(t *testing.T) {
+	// Randomness
+	skipIfVcr(t)
 	t.Parallel()
 
 	var instanceTemplate compute.InstanceTemplate
@@ -819,6 +821,8 @@ func TestAccComputeInstanceTemplate_invalidDiskType(t *testing.T) {
 }
 
 func TestAccComputeInstanceTemplate_imageResourceTest(t *testing.T) {
+	// Multiple fine-grained resources
+	skipIfVcr(t)
 	t.Parallel()
 	diskName := "tf-test-disk-" + randString(t, 10)
 	computeImage := "tf-test-image-" + randString(t, 10)

diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go
@@ -903,6 +903,8 @@ func TestAccComputeInstance_subnet_custom(t *testing.T) {
 }
 
 func TestAccComputeInstance_subnet_xpn(t *testing.T) {
+	// Multiple fine-grained resources
+	skipIfVcr(t)
 	t.Parallel()
 
 	var instance compute.Instance

diff --git a/google/resource_compute_network_endpoint_test.go b/google/resource_compute_network_endpoint_test.go
@@ -9,6 +9,8 @@ import (
 )
 
 func TestAccComputeNetworkEndpoint_networkEndpointsBasic(t *testing.T) {
+	// Multiple fine-grained resources
+	skipIfVcr(t)
 	t.Parallel()
 
 	context := map[string]interface{}{

diff --git a/google/resource_compute_region_instance_group_manager_test.go b/google/resource_compute_region_instance_group_manager_test.go
@@ -97,6 +97,8 @@ func TestAccRegionInstanceGroupManager_update(t *testing.T) {
 }
 
 func TestAccRegionInstanceGroupManager_updateLifecycle(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	tag1 := "tag1"
@@ -129,6 +131,8 @@ func TestAccRegionInstanceGroupManager_updateLifecycle(t *testing.T) {
 }
 
 func TestAccRegionInstanceGroupManager_rollingUpdatePolicy(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	igm := fmt.Sprintf("igm-test-%s", randString(t, 10))
@@ -164,6 +168,8 @@ func TestAccRegionInstanceGroupManager_rollingUpdatePolicy(t *testing.T) {
 }
 
 func TestAccRegionInstanceGroupManager_separateRegions(t *testing.T) {
+	// Randomness in instance template
+	skipIfVcr(t)
 	t.Parallel()
 
 	igm1 := fmt.Sprintf("igm-test-%s", randString(t, 10))