command: Fix most (but not all) "terraform plan" tests

backend/local/backend.go

@@ -355,7 +355,7 @@ func (b *Local) opWait(
 	select {
 	case <-stopCtx.Done():
 		if b.CLI != nil {
-			b.CLI.Output("stopping operation...")
+			b.CLI.Output("Stopping operation...")
 		}
 
 		// try to force a PersistState just in case the process is terminated
@@ -370,14 +370,16 @@ func (b *Local) opWait(
 		}
 
 		// Stop execution
+		log.Println("[TRACE] backend/local: waiting for the running operation to stop")
 		go tfCtx.Stop()
 
 		select {
 		case <-cancelCtx.Done():
-			log.Println("[WARN] running operation canceled")
+			log.Println("[WARN] running operation was forcefully canceled")
 			// if the operation was canceled, we need to return immediately
 			canceled = true
 		case <-doneCh:
+			log.Println("[TRACE] backend/local: graceful stop has completed")
 		}
 	case <-cancelCtx.Done():
 		// this should not be called without first attempting to stop the

backend/local/backend_plan.go

@@ -101,8 +101,13 @@ func (b *Local) opPlan(
 	}()
 
 	if b.opWait(doneCh, stopCtx, cancelCtx, tfCtx, opState) {
+		// If we get in here then the operation was cancelled, which is always
+		// considered to be a failure.
+		log.Printf("[INFO] backend/local: plan operation was force-cancelled by interrupt")
+		runningOp.Result = backend.OperationFailure
 		return
 	}
+	log.Printf("[INFO] backend/local: plan operation completed")
 
 	diags = diags.Append(planDiags)
 	if planDiags.HasErrors() {

command/command_test.go

@@ -215,18 +215,18 @@ func testPlanFileNoop(t *testing.T) string {
 	return testPlanFile(t, snap, state, plan)
 }
 
-func testReadPlan(t *testing.T, path string) *terraform.Plan {
+func testReadPlan(t *testing.T, path string) *plans.Plan {
 	t.Helper()
 
-	f, err := os.Open(path)
+	f, err := planfile.Open(path)
 	if err != nil {
-		t.Fatalf("err: %s", err)
+		t.Fatalf("error opening plan file %q: %s", path, err)
 	}
 	defer f.Close()
 
-	p, err := terraform.ReadPlan(f)
+	p, err := f.ReadPlan()
 	if err != nil {
-		t.Fatalf("err: %s", err)
+		t.Fatalf("error reading plan from plan file %q: %s", path, err)
 	}
 
 	return p

command/plan_test.go

@@ -11,12 +11,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/mitchellh/cli"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs/configschema"
 	"github.com/hashicorp/terraform/helper/copy"
+	"github.com/hashicorp/terraform/plans"
 	"github.com/hashicorp/terraform/providers"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/terraform"
@@ -101,8 +103,8 @@ func TestPlan_plan(t *testing.T) {
 	}
 
 	args := []string{planPath}
-	if code := c.Run(args); code != 0 {
+	if code := c.Run(args); code != 1 {
-		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+		t.Fatalf("wrong exit status %d; want 1\nstderr: %s", code, ui.ErrorWriter.String())
 	}
 
 	if p.ReadResourceCalled {
@@ -152,11 +154,9 @@ func TestPlan_destroy(t *testing.T) {
 	}
 
 	plan := testReadPlan(t, outPath)
-	for _, m := range plan.Diff.Modules {
+	for _, rc := range plan.Changes.Resources {
-		for _, r := range m.Resources {
+		if got, want := rc.Action, plans.Delete; got != want {
-			if !r.Destroy {
+			t.Fatalf("wrong action %s for %s; want %s\nplanned change: %s", got, rc.Addr, want, spew.Sdump(rc))
-				t.Fatalf("bad: %#v", r)
-			}
 		}
 	}
 }
@@ -222,15 +222,7 @@ func TestPlan_outPath(t *testing.T) {
 		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
 	}
 
-	f, err := os.Open(outPath)
+	testReadPlan(t, outPath) // will call t.Fatal itself if the file cannot be read
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	defer f.Close()
-
-	if _, err := terraform.ReadPlan(f); err != nil {
-		t.Fatalf("err: %s", err)
-	}
 }
 
 func TestPlan_outPathNoChange(t *testing.T) {
@@ -242,7 +234,10 @@ func TestPlan_outPathNoChange(t *testing.T) {
 				Name: "foo",
 			}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
 			&states.ResourceInstanceObjectSrc{
-				AttrsJSON: []byte(`{"id":"bar"}`),
+				// Aside from "id" (which is computed) the values here must
+				// exactly match the values in the "plan" test fixture in order
+				// to produce the empty plan we need for this test.
+				AttrsJSON: []byte(`{"id":"bar","ami":"bar","network_interface":[{"description":"Main network interface","device_index":"0"}]}`),
 				Status:    states.ObjectReady,
 			},
 			addrs.ProviderConfig{Type: "test"}.Absolute(addrs.RootModuleInstance),
@@ -272,8 +267,8 @@ func TestPlan_outPathNoChange(t *testing.T) {
 	}
 
 	plan := testReadPlan(t, outPath)
-	if !plan.Diff.Empty() {
+	if !plan.Changes.Empty() {
-		t.Fatalf("Expected empty plan to be written to plan file, got: %s", plan)
+		t.Fatalf("Expected empty plan to be written to plan file, got: %s", spew.Sdump(plan))
 	}
 }
 
@@ -310,7 +305,7 @@ func TestPlan_outBackend(t *testing.T) {
 
 	outPath := "foo"
 	p := testProvider()
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &PlanCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -322,19 +317,20 @@ func TestPlan_outBackend(t *testing.T) {
 		"-out", outPath,
 	}
 	if code := c.Run(args); code != 0 {
+		t.Logf("stdout: %s", ui.OutputWriter.String())
 		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
 	}
 
 	plan := testReadPlan(t, outPath)
-	if !plan.Diff.Empty() {
+	if !plan.Changes.Empty() {
-		t.Fatalf("Expected empty plan to be written to plan file, got: %s", plan)
+		t.Fatalf("Expected empty plan to be written to plan file, got: %s", spew.Sdump(plan))
 	}
 
-	if plan.Backend.Empty() {
+	if plan.Backend.Type == "" || plan.Backend.Config == nil {
 		t.Fatal("should have backend info")
 	}
 	if !reflect.DeepEqual(plan.Backend, dataState.Backend) {
-		t.Fatalf("bad: %#v", plan.Backend)
+		t.Fatalf("wrong backend config in plan\ngot:  %swant: %s", spew.Sdump(plan.Backend), spew.Sdump(dataState.Backend))
 	}
 }
 
@@ -489,8 +485,8 @@ func TestPlan_validate(t *testing.T) {
 	}
 
 	actual := ui.ErrorWriter.String()
-	if !strings.Contains(actual, "cannot be computed") {
+	if want := "Error: Invalid count argument"; !strings.Contains(actual, want) {
-		t.Fatalf("bad: %s", actual)
+		t.Fatalf("unexpected error output\ngot:\n%s\n\nshould contain: %s", actual, want)
 	}
 }
 
@@ -732,7 +728,7 @@ func TestPlan_shutdown(t *testing.T) {
 		})
 
 		// Because of the internal lock in the MockProvider, we can't
-		// coordiante directly with the calling of Stop, and making the
+		// coordinate directly with the calling of Stop, and making the
 		// MockProvider concurrent is disruptive to a lot of existing tests.
 		// Wait here a moment to help make sure the main goroutine gets to the
 		// Stop call before we exit, or the plan may finish before it can be
@@ -747,11 +743,28 @@ func TestPlan_shutdown(t *testing.T) {
 			},
 		}, nil
 	}
+	p.GetSchemaReturn = &terraform.ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_instance": {
+				Attributes: map[string]*configschema.Attribute{
+					"ami": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+	}
 
-	if code := c.Run([]string{testFixturePath("apply-shutdown")}); code != 1 {
+	code := c.Run([]string{
+		// Unfortunately it seems like this test can inadvertently pick up
+		// leftover state from other tests without this. Ideally we should
+		// find which test is leaving a terraform.tfstate behind and stop it
+		// doing that, but this will stop this test flapping for now.
+		"-state=nonexistent.tfstate",
+		testFixturePath("apply-shutdown"),
+	})
+	if code != 1 {
 		// FIXME: we should be able to avoid the error during evaluation
 		// the early exit isn't caught before the interpolation is evaluated
-		t.Fatal(ui.OutputWriter.String())
+		t.Fatalf("wrong exit code %d; want 1\noutput:\n%s", code, ui.OutputWriter.String())
 	}
 
 	select {

command/test-fixtures/plan-invalid/main.tf

@@ -1,7 +1,10 @@
 resource "test_instance" "foo" {
-    count = 5
+  count = 5
 }
 
 resource "test_instance" "bar" {
-    count = "${length(test_instance.foo.*.id)}"
+  # This is invalid because timestamp() returns an unknown value during plan,
+  # but the "count" argument in particular must always be known during plan
+  # so we can predict how many instances we will operate on.
+  count = timestamp()
 }