tidy up code

helper/resource/testing.go

@@ -574,7 +574,7 @@ func Test(t TestT, c TestCase) {
 
 	if acctest.TestHelper != nil && c.DisableBinaryDriver == false {
 		// inject providers for ImportStateVerify
-		RunLegacyTest(t.(*testing.T), c, providers)
+		RunNewTest(t.(*testing.T), c, providers)
 		return
 	}
 

helper/resource/testing_new.go

@@ -23,15 +23,12 @@ func getState(t *testing.T, wd *tftest.WorkingDir) *terraform.State {
 	return state
 }
 
-func RunLegacyTest(t *testing.T, c TestCase, providers map[string]terraform.ResourceProvider) {
+func RunNewTest(t *testing.T, c TestCase, providers map[string]terraform.ResourceProvider) {
 	spewConf := spew.NewDefaultConfig()
 	spewConf.SortKeys = true
 	wd := acctest.TestHelper.RequireNewWorkingDir(t)
 
 	defer func() {
-		// destroy step
-		// TODO probably better to combine this with TestStep.Destroy implementation as in old framework
-
 		wd.RequireDestroy(t)
 
 		if c.CheckDestroy != nil {
@@ -59,11 +56,6 @@ func RunLegacyTest(t *testing.T, c TestCase, providers map[string]terraform.Reso
 			step.PreConfig()
 		}
 
-		// TODOs
-		if step.Destroy {
-			t.Fatal("TODO: TestStep.Destroy")
-		}
-
 		if step.SkipFunc != nil {
 			skip, err := step.SkipFunc()
 			if err != nil {
@@ -104,13 +96,6 @@ func RunLegacyTest(t *testing.T, c TestCase, providers map[string]terraform.Reso
 
 		t.Fatal("Unsupported test mode")
 	}
-	// If we never checked an id-only refresh, it is a failure.
-	// TODO KEM: why is this here? does this ever happen?
-	// if idRefresh {
-	// 	if len(c.Steps) > 0 && idRefreshCheck == nil {
-	// 		t.Error("ID-only refresh check never ran.")
-	// 	}
-	// }
 }
 
 func planIsEmpty(plan *tfjson.Plan) bool {

helper/resource/testing_new_config.go

@@ -49,7 +49,7 @@ func testStepNewConfig(t *testing.T, c TestCase, wd *tftest.WorkingDir, step Tes
 			t.Log("[INFO] Got non-empty plan, as expected")
 		} else {
 
-			t.Fatalf("After applying this step, the plan was not empty. %s", spewConf.Sdump(plan)) // TODO error message
+			t.Fatalf("After applying this test step, the plan was not empty. %s", spewConf.Sdump(plan))
 		}
 	}
 
@@ -58,8 +58,6 @@ func testStepNewConfig(t *testing.T, c TestCase, wd *tftest.WorkingDir, step Tes
 		wd.RequireRefresh(t)
 	}
 
-	// TODO deal with the data resources instantiated during refresh
-
 	// do another plan
 	wd.RequireCreatePlan(t)
 	plan = wd.RequireSavedPlan(t)
@@ -70,7 +68,7 @@ func testStepNewConfig(t *testing.T, c TestCase, wd *tftest.WorkingDir, step Tes
 			t.Log("[INFO] Got non-empty plan, as expected")
 		} else {
 
-			t.Fatalf("After applying this step, the plan was not empty. %s", spewConf.Sdump(plan)) // TODO error message
+			t.Fatalf("After applying this test step and performing a `terraform refresh`, the plan was not empty. %s", spewConf.Sdump(plan))
 		}
 	}
 

helper/resource/testing_new_import_state.go

@@ -44,25 +44,8 @@ func testStepNewImportState(t *testing.T, c TestCase, wd *tftest.WorkingDir, ste
 	}
 	importId = step.ImportStateIdPrefix + importId
 
-	// create working directory for import tests
+	// Create working directory for import tests
 	if step.Config == "" {
-		// I can't understand how the previous framework
-		// managed to set this to just an empty provider block cfg
-		// it must have somehow piggy backed the last non import step config??
-
-		/*
-			if step.ImportState {
-				if step.Config == "" {
-					step.Config = testProviderConfig(c)
-				}
-
-				// Can optionally set step.Config in addition to
-				// step.ImportState, to provide config for the import.
-				state, err = testStepImportState(opts, state, step)
-			}
-		*/
-
-		// this is what I think should be done
 		step.Config = cfg
 		if step.Config == "" {
 			t.Fatal("Cannot import state with no specified config")