From 0c3cbfc2e698841231a1b86f78d8645c06af2643 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Thu, 29 Mar 2018 13:15:08 -0700 Subject: [PATCH 1/2] Fix organization policy tests. This updates the organization policy tests to be run sequentially, instead of in parallel, as they share a resource that they're modifying. It also updates them to use a separate organization than the one all our other tests are running in, which prevents other tests from failing because they're run in parallel to the organization policy changing under them. --- google/provider_test.go | 9 +++ ...esource_google_organization_policy_test.go | 62 ++++++++++++------- 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/google/provider_test.go b/google/provider_test.go index 5f5ee347..cc469600 100644 --- a/google/provider_test.go +++ b/google/provider_test.go @@ -37,6 +37,10 @@ var orgEnvVars = []string{ "GOOGLE_ORG", } +var orgTargetEnvVars = []string{ + "GOOGLE_ORG_2", +} + var billingAccountEnvVars = []string{ "GOOGLE_BILLING_ACCOUNT", } @@ -154,6 +158,11 @@ func getTestOrgFromEnv(t *testing.T) string { return multiEnvSearch(orgEnvVars) } +func getTestOrgTargetFromEnv(t *testing.T) string { + skipIfEnvNotSet(t, orgTargetEnvVars...) + return multiEnvSearch(orgTargetEnvVars) +} + func getTestBillingAccountFromEnv(t *testing.T) string { skipIfEnvNotSet(t, billingAccountEnvVars...) return multiEnvSearch(billingAccountEnvVars) diff --git a/google/resource_google_organization_policy_test.go b/google/resource_google_organization_policy_test.go index df0a9f95..2a58504a 100644 --- a/google/resource_google_organization_policy_test.go +++ b/google/resource_google_organization_policy_test.go @@ -18,9 +18,25 @@ var DENIED_ORG_POLICIES = []string{ // Since each test here is acting on the same organization, run the tests serially to // avoid race conditions and aborted operations. +func TestAccOrganizationPolicy(t *testing.T) { + testCases := map[string]func(t *testing.T){ + "boolean": testAccOrganizationPolicy_boolean, + "list_allowAll": testAccOrganizationPolicy_list_allowAll, + "list_allowSome": testAccOrganizationPolicy_list_allowSome, + "list_denySome": testAccOrganizationPolicy_list_denySome, + "list_update": testAccOrganizationPolicy_list_update, + } -func TestAccOrganizationPolicy_boolean(t *testing.T) { - org := getTestOrgFromEnv(t) + for name, tc := range testCases { + tc := tc + t.Run(name, func(t *testing.T) { + tc(t) + }) + } +} + +func testAccOrganizationPolicy_boolean(t *testing.T) { + org := getTestOrgTargetFromEnv(t) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, @@ -28,12 +44,12 @@ func TestAccOrganizationPolicy_boolean(t *testing.T) { Steps: []resource.TestStep{ { // Test creation of an enforced boolean policy - Config: testAccOrganizationPolicy_boolean(org, true), + Config: testAccOrganizationPolicyConfig_boolean(org, true), Check: testAccCheckGoogleOrganizationBooleanPolicy("bool", true), }, { // Test update from enforced to not - Config: testAccOrganizationPolicy_boolean(org, false), + Config: testAccOrganizationPolicyConfig_boolean(org, false), Check: testAccCheckGoogleOrganizationBooleanPolicy("bool", false), }, { @@ -42,12 +58,12 @@ func TestAccOrganizationPolicy_boolean(t *testing.T) { }, { // Test creation of a not enforced boolean policy - Config: testAccOrganizationPolicy_boolean(org, false), + Config: testAccOrganizationPolicyConfig_boolean(org, false), Check: testAccCheckGoogleOrganizationBooleanPolicy("bool", false), }, { // Test update from not enforced to enforced - Config: testAccOrganizationPolicy_boolean(org, true), + Config: testAccOrganizationPolicyConfig_boolean(org, true), Check: testAccCheckGoogleOrganizationBooleanPolicy("bool", true), }, { @@ -60,15 +76,15 @@ func TestAccOrganizationPolicy_boolean(t *testing.T) { } -func TestAccOrganizationPolicy_list_allowAll(t *testing.T) { - org := getTestOrgFromEnv(t) +func testAccOrganizationPolicy_list_allowAll(t *testing.T) { + org := getTestOrgTargetFromEnv(t) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckGoogleOrganizationPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccOrganizationPolicy_list_allowAll(org), + Config: testAccOrganizationPolicyConfig_list_allowAll(org), Check: testAccCheckGoogleOrganizationListPolicyAll("list", "ALLOW"), }, { @@ -80,8 +96,8 @@ func TestAccOrganizationPolicy_list_allowAll(t *testing.T) { }) } -func TestAccOrganizationPolicy_list_allowSome(t *testing.T) { - org := getTestOrgFromEnv(t) +func testAccOrganizationPolicy_list_allowSome(t *testing.T) { + org := getTestOrgTargetFromEnv(t) project := getTestProjectFromEnv() resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -89,7 +105,7 @@ func TestAccOrganizationPolicy_list_allowSome(t *testing.T) { CheckDestroy: testAccCheckGoogleOrganizationPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccOrganizationPolicy_list_allowSome(org, project), + Config: testAccOrganizationPolicyConfig_list_allowSome(org, project), Check: testAccCheckGoogleOrganizationListPolicyAllowedValues("list", []string{"projects/" + project, "projects/debian-cloud"}), }, { @@ -101,15 +117,15 @@ func TestAccOrganizationPolicy_list_allowSome(t *testing.T) { }) } -func TestAccOrganizationPolicy_list_denySome(t *testing.T) { - org := getTestOrgFromEnv(t) +func testAccOrganizationPolicy_list_denySome(t *testing.T) { + org := getTestOrgTargetFromEnv(t) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckGoogleOrganizationPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccOrganizationPolicy_list_denySome(org), + Config: testAccOrganizationPolicyConfig_list_denySome(org), Check: testAccCheckGoogleOrganizationListPolicyDeniedValues("list", DENIED_ORG_POLICIES), }, { @@ -121,19 +137,19 @@ func TestAccOrganizationPolicy_list_denySome(t *testing.T) { }) } -func TestAccOrganizationPolicy_list_update(t *testing.T) { - org := getTestOrgFromEnv(t) +func testAccOrganizationPolicy_list_update(t *testing.T) { + org := getTestOrgTargetFromEnv(t) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckGoogleOrganizationPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccOrganizationPolicy_list_allowAll(org), + Config: testAccOrganizationPolicyConfig_list_allowAll(org), Check: testAccCheckGoogleOrganizationListPolicyAll("list", "ALLOW"), }, { - Config: testAccOrganizationPolicy_list_denySome(org), + Config: testAccOrganizationPolicyConfig_list_denySome(org), Check: testAccCheckGoogleOrganizationListPolicyDeniedValues("list", DENIED_ORG_POLICIES), }, { @@ -256,7 +272,7 @@ func getGoogleOrganizationPolicyTestResource(s *terraform.State, n string) (*clo }).Do() } -func testAccOrganizationPolicy_boolean(org string, enforced bool) string { +func testAccOrganizationPolicyConfig_boolean(org string, enforced bool) string { return fmt.Sprintf(` resource "google_organization_policy" "bool" { org_id = "%s" @@ -269,7 +285,7 @@ resource "google_organization_policy" "bool" { `, org, enforced) } -func testAccOrganizationPolicy_list_allowAll(org string) string { +func testAccOrganizationPolicyConfig_list_allowAll(org string) string { return fmt.Sprintf(` resource "google_organization_policy" "list" { org_id = "%s" @@ -284,7 +300,7 @@ resource "google_organization_policy" "list" { `, org) } -func testAccOrganizationPolicy_list_allowSome(org, project string) string { +func testAccOrganizationPolicyConfig_list_allowSome(org, project string) string { return fmt.Sprintf(` resource "google_organization_policy" "list" { org_id = "%s" @@ -302,7 +318,7 @@ resource "google_organization_policy" "list" { `, org, project) } -func testAccOrganizationPolicy_list_denySome(org string) string { +func testAccOrganizationPolicyConfig_list_denySome(org string) string { return fmt.Sprintf(` resource "google_organization_policy" "list" { org_id = "%s" From dfa40af99384eb689856529fa15ef8d816ad3f46 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Thu, 29 Mar 2018 13:27:54 -0700 Subject: [PATCH 2/2] Comment on shadowing variable. --- google/resource_google_organization_policy_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/google/resource_google_organization_policy_test.go b/google/resource_google_organization_policy_test.go index 2a58504a..4a09f0c8 100644 --- a/google/resource_google_organization_policy_test.go +++ b/google/resource_google_organization_policy_test.go @@ -28,6 +28,10 @@ func TestAccOrganizationPolicy(t *testing.T) { } for name, tc := range testCases { + // shadow the tc variable into scope so that when + // the loop continues, if t.Run hasn't executed tc(t) + // yet, we don't have a race condition + // see https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables tc := tc t.Run(name, func(t *testing.T) { tc(t)