From 91d26b6ea66a1e7ec89a8ffd090dd8bbc433a16c Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Fri, 8 Sep 2023 19:43:18 +0200 Subject: [PATCH] Refactor file and run block feedback frequency (#33840) * Refactor file and run block feedback frequency * fix consistency checks * make generate --- internal/backend/local/test.go | 15 ++-- internal/command/test_test.go | 56 ++++++++++--- internal/command/views/json/test.go | 11 ++- internal/command/views/test.go | 83 ++++++++++++++++--- internal/command/views/test_test.go | 105 ++++++++++++++++++++----- internal/moduletest/progress.go | 18 +++++ internal/moduletest/progress_string.go | 25 ++++++ 7 files changed, 263 insertions(+), 50 deletions(-) create mode 100644 internal/moduletest/progress.go create mode 100644 internal/moduletest/progress_string.go diff --git a/internal/backend/local/test.go b/internal/backend/local/test.go index 85c38c69ce..6744c82d03 100644 --- a/internal/backend/local/test.go +++ b/internal/backend/local/test.go @@ -108,8 +108,11 @@ func (runner *TestSuiteRunner) Test() (moduletest.Status, tfdiags.Diagnostics) { PriorStates: make(map[string]*terraform.TestContext), } + runner.View.File(file, moduletest.Starting) fileRunner.Test(file) + runner.View.File(file, moduletest.TearDown) fileRunner.cleanup(file) + runner.View.File(file, moduletest.Complete) suite.Status = suite.Status.Merge(file.Status) } @@ -235,16 +238,18 @@ func (runner *TestFileRunner) Test(file *moduletest.File) { if runner.Suite.Stopped { // Then the test was requested to be stopped, so we just mark each - // following test as skipped and move on. + // following test as skipped, print the status, and move on. run.Status = moduletest.Skip + runner.Suite.View.Run(run, file) continue } if file.Status == moduletest.Error { // If the overall test file has errored, we don't keep trying to // execute tests. Instead, we mark all remaining run blocks as - // skipped. + // skipped, print the status, and move on. run.Status = moduletest.Skip + runner.Suite.View.Run(run, file) continue } @@ -289,12 +294,8 @@ func (runner *TestFileRunner) Test(file *moduletest.File) { runner.RelevantStates[key].Run = run } - file.Status = file.Status.Merge(run.Status) - } - - runner.Suite.View.File(file) - for _, run := range file.Runs { runner.Suite.View.Run(run, file) + file.Status = file.Status.Merge(run.Status) } } diff --git a/internal/command/test_test.go b/internal/command/test_test.go index e583920a33..95c6aee577 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -476,8 +476,10 @@ func TestTest_CatchesErrorsBeforeDestroy(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expectedOut := `main.tftest.hcl... fail + expectedOut := `main.tftest.hcl... in progress run "test"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 0 passed, 1 failed. ` @@ -531,7 +533,7 @@ func TestTest_Verbose(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expected := `main.tftest.hcl... pass + expected := `main.tftest.hcl... in progress run "validate_test_resource"... pass Terraform used the selected providers to generate the following execution @@ -547,13 +549,18 @@ Terraform will perform the following actions: } Plan: 1 to add, 0 to change, 0 to destroy. + run "validate_test_resource"... pass + # test_resource.foo: resource "test_resource" "foo" { id = "constant_value" value = "bar" } +main.tftest.hcl... tearing down +main.tftest.hcl... pass + Success! 2 passed, 0 failed. ` @@ -574,8 +581,10 @@ func TestTest_ValidatesBeforeExecution(t *testing.T) { expectedErr string }{ "invalid": { - expectedOut: `main.tftest.hcl... fail + expectedOut: `main.tftest.hcl... in progress run "invalid"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 0 passed, 1 failed. `, @@ -591,9 +600,11 @@ managed resources and data sources. `, }, "invalid-module": { - expectedOut: `main.tftest.hcl... fail + expectedOut: `main.tftest.hcl... in progress run "invalid"... fail run "test"... skip +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 0 passed, 1 failed, 1 skipped. `, @@ -608,8 +619,10 @@ variable can be declared with a variable "not_real" {} block. `, }, "missing-provider": { - expectedOut: `main.tftest.hcl... fail + expectedOut: `main.tftest.hcl... in progress run "passes_validation"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 0 passed, 1 failed. `, @@ -625,8 +638,10 @@ can remove the provider configuration again. `, }, "missing-provider-in-run-block": { - expectedOut: `main.tftest.hcl... fail + expectedOut: `main.tftest.hcl... in progress run "passes_validation"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 0 passed, 1 failed. `, @@ -642,9 +657,11 @@ can remove the provider configuration again. `, }, "missing-provider-in-test-module": { - expectedOut: `main.tftest.hcl... fail + expectedOut: `main.tftest.hcl... in progress run "passes_validation_primary"... pass run "passes_validation_secondary"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 1 passed, 1 failed. `, @@ -822,19 +839,23 @@ func TestTest_StatePropagation(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expected := `main.tftest.hcl... pass + expected := `main.tftest.hcl... in progress run "initial_apply_example"... pass + # test_resource.module_resource: resource "test_resource" "module_resource" { id = "df6h8as9" value = "start" } + run "initial_apply"... pass + # test_resource.resource: resource "test_resource" "resource" { id = "598318e0" value = "start" } + run "plan_second_example"... pass Terraform used the selected providers to generate the following execution @@ -850,6 +871,7 @@ Terraform will perform the following actions: } Plan: 1 to add, 0 to change, 0 to destroy. + run "plan_update"... pass Terraform used the selected providers to generate the following execution @@ -865,6 +887,7 @@ Terraform will perform the following actions: } Plan: 0 to add, 1 to change, 0 to destroy. + run "plan_update_example"... pass Terraform used the selected providers to generate the following execution @@ -881,6 +904,9 @@ Terraform will perform the following actions: Plan: 0 to add, 1 to change, 0 to destroy. +main.tftest.hcl... tearing down +main.tftest.hcl... pass + Success! 5 passed, 0 failed. ` @@ -938,9 +964,11 @@ func TestTest_OnlyExternalModules(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expected := `main.tftest.hcl... pass + expected := `main.tftest.hcl... in progress run "first"... pass run "second"... pass +main.tftest.hcl... tearing down +main.tftest.hcl... pass Success! 2 passed, 0 failed. ` @@ -978,7 +1006,7 @@ func TestTest_PartialUpdates(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expected := `main.tftest.hcl... pass + expected := `main.tftest.hcl... in progress run "first"... pass Warning: Resource targeting is in effect @@ -1003,7 +1031,10 @@ Note that the -target option is not suitable for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message. + run "second"... pass +main.tftest.hcl... tearing down +main.tftest.hcl... pass Success! 2 passed, 0 failed. ` @@ -1041,7 +1072,7 @@ func TestTest_BadReferences(t *testing.T) { t.Errorf("expected status code 0 but got %d", code) } - expectedOut := `main.tftest.hcl... fail + expectedOut := `main.tftest.hcl... in progress run "setup"... pass run "test"... fail @@ -1052,7 +1083,10 @@ Warning: Value for undeclared variable The module under test does not declare a variable named "input_three", but it is declared in run block "test". + run "finalise"... skip +main.tftest.hcl... tearing down +main.tftest.hcl... fail Failure! 1 passed, 1 failed, 1 skipped. ` diff --git a/internal/command/views/json/test.go b/internal/command/views/json/test.go index 666a0a95f0..fc5a8b6693 100644 --- a/internal/command/views/json/test.go +++ b/internal/command/views/json/test.go @@ -13,9 +13,12 @@ type TestSuiteAbstract map[string][]string type TestStatus string +type TestProgress string + type TestFileStatus struct { - Path string `json:"path"` - Status TestStatus `json:"status"` + Path string `json:"path"` + Progress TestProgress `json:"progress"` + Status TestStatus `json:"status,omitempty"` } type TestRunStatus struct { @@ -50,3 +53,7 @@ type TestFatalInterrupt struct { func ToTestStatus(status moduletest.Status) TestStatus { return TestStatus(strings.ToLower(status.String())) } + +func ToTestProgress(progress moduletest.Progress) TestProgress { + return TestProgress(strings.ToLower(progress.String())) +} diff --git a/internal/command/views/test.go b/internal/command/views/test.go index a044487b78..d1edeb8a55 100644 --- a/internal/command/views/test.go +++ b/internal/command/views/test.go @@ -39,7 +39,7 @@ type Test interface { Conclusion(suite *moduletest.Suite) // File prints out the summary for an entire test file. - File(file *moduletest.File) + File(file *moduletest.File, progress moduletest.Progress) // Run prints out the summary for a single test run block. Run(run *moduletest.Run, file *moduletest.File) @@ -131,9 +131,18 @@ func (t *TestHuman) Conclusion(suite *moduletest.Suite) { } } -func (t *TestHuman) File(file *moduletest.File) { - t.view.streams.Printf("%s... %s\n", file.Name, colorizeTestStatus(file.Status, t.view.colorize)) - t.Diagnostics(nil, file, file.Diagnostics) +func (t *TestHuman) File(file *moduletest.File, progress moduletest.Progress) { + switch progress { + case moduletest.Starting: + t.view.streams.Printf(t.view.colorize.Color("%s... [light_gray]in progress[reset]\n"), file.Name) + case moduletest.TearDown: + t.view.streams.Printf(t.view.colorize.Color("%s... [light_gray]tearing down[reset]\n"), file.Name) + case moduletest.Complete: + t.view.streams.Printf("%s... %s\n", file.Name, colorizeTestStatus(file.Status, t.view.colorize)) + t.Diagnostics(nil, file, file.Diagnostics) + default: + panic("unrecognized test progress: " + progress.String()) + } } func (t *TestHuman) Run(run *moduletest.Run, file *moduletest.File) { @@ -171,7 +180,9 @@ func (t *TestHuman) Run(run *moduletest.Run, file *moduletest.File) { ProviderSchemas: jsonprovider.MarshalForRenderer(schemas), } + t.view.streams.Println() // Separate the state from any previous statements. renderer.RenderHumanState(state) + t.view.streams.Println() // Separate the state from any future statements. } } else { // We'll print the plan. @@ -201,12 +212,35 @@ func (t *TestHuman) Run(run *moduletest.Run, file *moduletest.File) { } renderer.RenderHumanPlan(plan, run.Verbose.Plan.UIMode, opts...) + t.view.streams.Println() // Separate the plan from any future statements. } } } // Finally we'll print out a summary of the diagnostics from the run. t.Diagnostics(run, file, run.Diagnostics) + + var warnings bool + for _, diag := range run.Diagnostics { + switch diag.Severity() { + case tfdiags.Error: + // do nothing + case tfdiags.Warning: + warnings = true + } + + if warnings { + // We only care about checking if we printed any warnings in the + // previous output. + break + } + } + + if warnings { + // warnings are printed to stdout, so we'll put a new line into stdout + // to separate any future statements info statements. + t.view.streams.Println() + } } func (t *TestHuman) DestroySummary(diags tfdiags.Diagnostics, run *moduletest.Run, file *moduletest.File, state *states.State) { @@ -386,13 +420,40 @@ func (t *TestJSON) Conclusion(suite *moduletest.Suite) { json.MessageTestSummary, summary) } -func (t *TestJSON) File(file *moduletest.File) { - t.view.log.Info( - fmt.Sprintf("%s... %s", file.Name, testStatus(file.Status)), - "type", json.MessageTestFile, - json.MessageTestFile, json.TestFileStatus{file.Name, json.ToTestStatus(file.Status)}, - "@testfile", file.Name) - t.Diagnostics(nil, file, file.Diagnostics) +func (t *TestJSON) File(file *moduletest.File, progress moduletest.Progress) { + switch progress { + case moduletest.Starting: + t.view.log.Info( + fmt.Sprintf("%s... in progress", file.Name), + "type", json.MessageTestFile, + json.MessageTestFile, json.TestFileStatus{ + Path: file.Name, + Progress: json.ToTestProgress(moduletest.Starting), + }, + "@testfile", file.Name) + case moduletest.TearDown: + t.view.log.Info( + fmt.Sprintf("%s... tearing down", file.Name), + "type", json.MessageTestFile, + json.MessageTestFile, json.TestFileStatus{ + Path: file.Name, + Progress: json.ToTestProgress(moduletest.TearDown), + }, + "@testfile", file.Name) + case moduletest.Complete: + t.view.log.Info( + fmt.Sprintf("%s... %s", file.Name, testStatus(file.Status)), + "type", json.MessageTestFile, + json.MessageTestFile, json.TestFileStatus{ + Path: file.Name, + Progress: json.ToTestProgress(moduletest.Complete), + Status: json.ToTestStatus(file.Status), + }, + "@testfile", file.Name) + t.Diagnostics(nil, file, file.Diagnostics) + default: + panic("unrecognized test progress: " + progress.String()) + } } func (t *TestJSON) Run(run *moduletest.Run, file *moduletest.File) { diff --git a/internal/command/views/test_test.go b/internal/command/views/test_test.go index 581064e47b..e737e14fb6 100644 --- a/internal/command/views/test_test.go +++ b/internal/command/views/test_test.go @@ -412,32 +412,48 @@ func TestTestHuman_Conclusion(t *testing.T) { func TestTestHuman_File(t *testing.T) { tcs := map[string]struct { File *moduletest.File + Progress moduletest.Progress Expected string }{ "pass": { File: &moduletest.File{Name: "main.tf", Status: moduletest.Pass}, + Progress: moduletest.Complete, Expected: "main.tf... pass\n", }, "pending": { File: &moduletest.File{Name: "main.tf", Status: moduletest.Pending}, + Progress: moduletest.Complete, Expected: "main.tf... pending\n", }, "skip": { File: &moduletest.File{Name: "main.tf", Status: moduletest.Skip}, + Progress: moduletest.Complete, Expected: "main.tf... skip\n", }, "fail": { File: &moduletest.File{Name: "main.tf", Status: moduletest.Fail}, + Progress: moduletest.Complete, Expected: "main.tf... fail\n", }, "error": { File: &moduletest.File{Name: "main.tf", Status: moduletest.Error}, + Progress: moduletest.Complete, Expected: "main.tf... fail\n", }, + "starting": { + File: &moduletest.File{Name: "main.tftest.hcl", Status: moduletest.Pending}, + Progress: moduletest.Starting, + Expected: "main.tftest.hcl... in progress\n", + }, + "tear_down": { + File: &moduletest.File{Name: "main.tftest.hcl", Status: moduletest.Pending}, + Progress: moduletest.TearDown, + Expected: "main.tftest.hcl... tearing down\n", + }, } for name, tc := range tcs { t.Run(name, func(t *testing.T) { @@ -445,7 +461,7 @@ func TestTestHuman_File(t *testing.T) { streams, done := terminal.StreamsForTesting(t) view := NewTest(arguments.ViewHuman, NewView(streams)) - view.File(tc.File) + view.File(tc.File, tc.Progress) actual := done(t).Stdout() expected := tc.Expected @@ -478,6 +494,7 @@ func TestTestHuman_Run(t *testing.T) { Warning: a warning occurred some warning happened during this test + `, }, @@ -627,6 +644,7 @@ Terraform will perform the following actions: } Plan: 1 to add, 0 to change, 0 to destroy. + `, }, "verbose_apply": { @@ -685,10 +703,12 @@ Plan: 1 to add, 0 to change, 0 to destroy. }, }, StdOut: ` run "run_block"... pass + # test_resource.creating: resource "test_resource" "creating" { value = "foobar" } + `, }, } @@ -2181,11 +2201,13 @@ func TestTestJSON_DestroySummary(t *testing.T) { func TestTestJSON_File(t *testing.T) { tcs := map[string]struct { - file *moduletest.File - want []map[string]interface{} + file *moduletest.File + progress moduletest.Progress + want []map[string]interface{} }{ "pass": { - file: &moduletest.File{Name: "main.tf", Status: moduletest.Pass}, + file: &moduletest.File{Name: "main.tf", Status: moduletest.Pass}, + progress: moduletest.Complete, want: []map[string]interface{}{ { "@level": "info", @@ -2193,8 +2215,9 @@ func TestTestJSON_File(t *testing.T) { "@module": "terraform.ui", "@testfile": "main.tf", "test_file": map[string]interface{}{ - "path": "main.tf", - "status": "pass", + "path": "main.tf", + "progress": "complete", + "status": "pass", }, "type": "test_file", }, @@ -2202,7 +2225,8 @@ func TestTestJSON_File(t *testing.T) { }, "pending": { - file: &moduletest.File{Name: "main.tf", Status: moduletest.Pending}, + file: &moduletest.File{Name: "main.tf", Status: moduletest.Pending}, + progress: moduletest.Complete, want: []map[string]interface{}{ { "@level": "info", @@ -2210,8 +2234,9 @@ func TestTestJSON_File(t *testing.T) { "@module": "terraform.ui", "@testfile": "main.tf", "test_file": map[string]interface{}{ - "path": "main.tf", - "status": "pending", + "path": "main.tf", + "progress": "complete", + "status": "pending", }, "type": "test_file", }, @@ -2219,7 +2244,8 @@ func TestTestJSON_File(t *testing.T) { }, "skip": { - file: &moduletest.File{Name: "main.tf", Status: moduletest.Skip}, + file: &moduletest.File{Name: "main.tf", Status: moduletest.Skip}, + progress: moduletest.Complete, want: []map[string]interface{}{ { "@level": "info", @@ -2227,8 +2253,9 @@ func TestTestJSON_File(t *testing.T) { "@module": "terraform.ui", "@testfile": "main.tf", "test_file": map[string]interface{}{ - "path": "main.tf", - "status": "skip", + "path": "main.tf", + "progress": "complete", + "status": "skip", }, "type": "test_file", }, @@ -2236,7 +2263,8 @@ func TestTestJSON_File(t *testing.T) { }, "fail": { - file: &moduletest.File{Name: "main.tf", Status: moduletest.Fail}, + file: &moduletest.File{Name: "main.tf", Status: moduletest.Fail}, + progress: moduletest.Complete, want: []map[string]interface{}{ { "@level": "info", @@ -2244,8 +2272,9 @@ func TestTestJSON_File(t *testing.T) { "@module": "terraform.ui", "@testfile": "main.tf", "test_file": map[string]interface{}{ - "path": "main.tf", - "status": "fail", + "path": "main.tf", + "progress": "complete", + "status": "fail", }, "type": "test_file", }, @@ -2253,7 +2282,8 @@ func TestTestJSON_File(t *testing.T) { }, "error": { - file: &moduletest.File{Name: "main.tf", Status: moduletest.Error}, + file: &moduletest.File{Name: "main.tf", Status: moduletest.Error}, + progress: moduletest.Complete, want: []map[string]interface{}{ { "@level": "info", @@ -2261,8 +2291,45 @@ func TestTestJSON_File(t *testing.T) { "@module": "terraform.ui", "@testfile": "main.tf", "test_file": map[string]interface{}{ - "path": "main.tf", - "status": "error", + "path": "main.tf", + "progress": "complete", + "status": "error", + }, + "type": "test_file", + }, + }, + }, + + "starting": { + file: &moduletest.File{Name: "main.tftest.hcl", Status: moduletest.Pending}, + progress: moduletest.Starting, + want: []map[string]interface{}{ + { + "@level": "info", + "@message": "main.tftest.hcl... in progress", + "@module": "terraform.ui", + "@testfile": "main.tftest.hcl", + "test_file": map[string]interface{}{ + "path": "main.tftest.hcl", + "progress": "starting", + }, + "type": "test_file", + }, + }, + }, + + "tear_down": { + file: &moduletest.File{Name: "main.tftest.hcl", Status: moduletest.Pending}, + progress: moduletest.TearDown, + want: []map[string]interface{}{ + { + "@level": "info", + "@message": "main.tftest.hcl... tearing down", + "@module": "terraform.ui", + "@testfile": "main.tftest.hcl", + "test_file": map[string]interface{}{ + "path": "main.tftest.hcl", + "progress": "teardown", }, "type": "test_file", }, @@ -2274,7 +2341,7 @@ func TestTestJSON_File(t *testing.T) { streams, done := terminal.StreamsForTesting(t) view := NewTest(arguments.ViewJSON, NewView(streams)) - view.File(tc.file) + view.File(tc.file, tc.progress) testJSONViewOutputEquals(t, done(t).All(), tc.want) }) } diff --git a/internal/moduletest/progress.go b/internal/moduletest/progress.go new file mode 100644 index 0000000000..98da39ceb2 --- /dev/null +++ b/internal/moduletest/progress.go @@ -0,0 +1,18 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package moduletest + +// Progress represents the status of a test file as it executes. +// +// We will include the progress markers to provide feedback as each test file +// executes. +// +//go:generate go run golang.org/x/tools/cmd/stringer -type=Progress progress.go +type Progress int + +const ( + Starting Progress = iota + TearDown + Complete +) diff --git a/internal/moduletest/progress_string.go b/internal/moduletest/progress_string.go new file mode 100644 index 0000000000..7764621e35 --- /dev/null +++ b/internal/moduletest/progress_string.go @@ -0,0 +1,25 @@ +// Code generated by "stringer -type=Progress progress.go"; DO NOT EDIT. + +package moduletest + +import "strconv" + +func _() { + // An "invalid array index" compiler error signifies that the constant values have changed. + // Re-run the stringer command to generate them again. + var x [1]struct{} + _ = x[Starting-0] + _ = x[TearDown-1] + _ = x[Complete-2] +} + +const _Progress_name = "StartingTearDownComplete" + +var _Progress_index = [...]uint8{0, 8, 16, 24} + +func (i Progress) String() string { + if i < 0 || i >= Progress(len(_Progress_index)-1) { + return "Progress(" + strconv.FormatInt(int64(i), 10) + ")" + } + return _Progress_name[_Progress_index[i]:_Progress_index[i+1]] +}