test: os.Unsetenv affects global state

This change adds a helper function that guarantees the cleanup of
environment variables.

For each applicable test function, we run this at the start, and at the
end. Previously, we were unsetting the environment variable within the
test, though there is a risk that since global state is being
manipulated, we may run into a flakey test where an environment variable
is inadvertently lingering.

src/plugin/config_test.go

@@ -15,47 +15,43 @@ func TestFailOnMissingRequiredEnvironment(t *testing.T) {
 	fetcher := plugin.EnvironmentConfigFetcher{}
 
 	tests := []struct {
-		name            string
-		enabledEnvVars  map[string]string
-		disabledEnvVars []string
-		expectedErr     string
+		name           string
+		enabledEnvVars map[string]string
+		expectedErr    string
 	}{
 		{
-			name:            "all required parameters are unset",
-			enabledEnvVars:  map[string]string{},
-			disabledEnvVars: []string{"BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME", "BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT"},
-			expectedErr:     "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME missing value",
+			name:           "all required parameters are unset",
+			enabledEnvVars: map[string]string{},
+			expectedErr:    "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME missing value",
 		},
 		{
 			name: "variable PARAMETER_NAME set",
 			enabledEnvVars: map[string]string{
 				"BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME": "test-parameter",
 			},
-			disabledEnvVars: []string{"BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT"},
-			expectedErr:     "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT missing value",
+			expectedErr: "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT missing value",
 		},
 		{
 			name: "variable SCRIPT set",
 			enabledEnvVars: map[string]string{
 				"BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT": "bin/script",
 			},
-			disabledEnvVars: []string{"BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME"},
-			expectedErr:     "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME missing value",
+			expectedErr: "required key BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME missing value",
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
+			// clear the environment variables *before* each test case is run
+			unsetEnvironmentVariables()
+			// clear the environment variables *after* each test case is run
+			defer unsetEnvironmentVariables()
+
 			// set the environment variables
 			for key, value := range tc.enabledEnvVars {
 				t.Setenv(key, value)
 			}
 
-			// unset the environment variables
-			for _, key := range tc.disabledEnvVars {
-				os.Unsetenv(key)
-			}
-
 			// verify the fetcher throws the error specific to missing environment variable
 			err := fetcher.Fetch(&config)
 			assert.EqualError(t, err, tc.expectedErr, "fetch should error on missing environment variable")
@@ -64,6 +60,9 @@ func TestFailOnMissingRequiredEnvironment(t *testing.T) {
 }
 
 func TestFetchConfigFromEnvironment(t *testing.T) {
+	unsetEnvironmentVariables()
+	defer unsetEnvironmentVariables()
+
 	var config plugin.Config
 	fetcher := plugin.EnvironmentConfigFetcher{}
 
@@ -76,3 +75,10 @@ func TestFetchConfigFromEnvironment(t *testing.T) {
 	assert.Equal(t, "test-parameter", config.ParameterName, "fetched message should match environment")
 	assert.Equal(t, "hello-world", config.Script, "fetched message should match environment")
 }
+
+// Unsets environment variables through an all-in-one function. Extend this with additional environment variables as
+// needed.
+func unsetEnvironmentVariables() {
+	os.Unsetenv("BUILDKITE_PLUGIN_ECS_TASK_RUNNER_PARAMETER_NAME")
+	os.Unsetenv("BUILDKITE_PLUGIN_ECS_TASK_RUNNER_SCRIPT")
+}