From 21cafd70ea7298175a6a28e822b66d6aeeb212cb Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Thu, 16 Jan 2025 12:12:15 +0000 Subject: [PATCH] Add more details to JUnit `terraform test` output to describe why a test was skipped (#36315) * Add ability for TestJUnitXMLFile to access data about whether the test runner was Stopped * Add details to XML describing why a Run was skipped * Fix wording * Code consistency changes * Move all JUnit-related code down to where it's used Away from the Views section of the code where it was relevant before * Move JUnit-related error and warning diags to above where cancellable contexts are created * Fix wording of user feedback * Fix test to match updated skipped message text * Fix test --- internal/backend/local/test.go | 4 + internal/cloud/test.go | 4 + internal/command/junit/junit.go | 52 ++++- internal/command/junit/junit_internal_test.go | 153 ++++++++++++ internal/command/junit/junit_test.go | 217 ++++++------------ internal/command/test.go | 3 +- internal/moduletest/suite.go | 4 + 7 files changed, 280 insertions(+), 157 deletions(-) create mode 100644 internal/command/junit/junit_internal_test.go diff --git a/internal/backend/local/test.go b/internal/backend/local/test.go index 2d99379390..efbf371a22 100644 --- a/internal/backend/local/test.go +++ b/internal/backend/local/test.go @@ -86,6 +86,10 @@ func (runner *TestSuiteRunner) Stop() { runner.Stopped = true } +func (runner *TestSuiteRunner) IsStopped() bool { + return runner.Stopped +} + func (runner *TestSuiteRunner) Cancel() { runner.Cancelled = true } diff --git a/internal/cloud/test.go b/internal/cloud/test.go index 5259361f30..e81d07494f 100644 --- a/internal/cloud/test.go +++ b/internal/cloud/test.go @@ -109,6 +109,10 @@ func (runner *TestSuiteRunner) Stop() { runner.Stopped = true } +func (runner *TestSuiteRunner) IsStopped() bool { + return runner.Stopped +} + func (runner *TestSuiteRunner) Cancel() { runner.Cancelled = true } diff --git a/internal/command/junit/junit.go b/internal/command/junit/junit.go index 2aae55a91c..3ae7bfa815 100644 --- a/internal/command/junit/junit.go +++ b/internal/command/junit/junit.go @@ -40,6 +40,9 @@ type TestJUnitXMLFile struct { // A config loader is required to access sources, which are used with diagnostics to create XML content configLoader *configload.Loader + + // A pointer to the containing test suite runner is needed to monitor details like the command being stopped + testSuiteRunner moduletest.TestSuiteRunner } type JUnit interface { @@ -55,10 +58,11 @@ var _ JUnit = (*TestJUnitXMLFile)(nil) // point of being asked to write a conclusion. Otherwise it will create the // file at that time. If creating or overwriting the file fails, a subsequent // call to method Err will return information about the problem. -func NewTestJUnitXMLFile(filename string, configLoader *configload.Loader) *TestJUnitXMLFile { +func NewTestJUnitXMLFile(filename string, configLoader *configload.Loader, testSuiteRunner moduletest.TestSuiteRunner) *TestJUnitXMLFile { return &TestJUnitXMLFile{ - filename: filename, - configLoader: configLoader, + filename: filename, + configLoader: configLoader, + testSuiteRunner: testSuiteRunner, } } @@ -69,7 +73,7 @@ func (v *TestJUnitXMLFile) Save(suite *moduletest.Suite) tfdiags.Diagnostics { // Prepare XML content sources := v.configLoader.Parser().Sources() - xmlSrc, err := junitXMLTestReport(suite, sources) + xmlSrc, err := junitXMLTestReport(suite, v.testSuiteRunner.IsStopped(), sources) if err != nil { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -130,7 +134,7 @@ type testCase struct { Timestamp string `xml:"timestamp,attr,omitempty"` } -func junitXMLTestReport(suite *moduletest.Suite, sources map[string][]byte) ([]byte, error) { +func junitXMLTestReport(suite *moduletest.Suite, suiteRunnerStopped bool, sources map[string][]byte) ([]byte, error) { var buf bytes.Buffer enc := xml.NewEncoder(&buf) enc.EncodeToken(xml.ProcInst{ @@ -182,7 +186,7 @@ func junitXMLTestReport(suite *moduletest.Suite, sources map[string][]byte) ([]b }, }) - for _, run := range file.Runs { + for i, run := range file.Runs { // Each run is a "test case". testCase := testCase{ @@ -201,9 +205,10 @@ func junitXMLTestReport(suite *moduletest.Suite, sources map[string][]byte) ([]b } switch run.Status { case moduletest.Skip: + message, body := getSkipDetails(i, file, suiteRunnerStopped) testCase.Skipped = &withMessage{ - // FIXME: Is there something useful we could say here about - // why the test was skipped? + Message: message, + Body: body, } case moduletest.Fail: testCase.Failure = &withMessage{ @@ -248,6 +253,37 @@ func junitXMLTestReport(suite *moduletest.Suite, sources map[string][]byte) ([]b return buf.Bytes(), nil } +// getSkipDetails checks data about the test suite, file and runs to determine why a given run was skipped +// Test can be skipped due to: +// 1. terraform test recieving an interrupt from users; all unstarted tests will be skipped +// 2. A previous run in a file has failed, causing subsequent run blocks to be skipped +func getSkipDetails(runIndex int, file *moduletest.File, suiteStopped bool) (string, string) { + if suiteStopped { + // Test suite experienced an interrupt + // This block only handles graceful Stop interrupts, as Cancel interrupts will prevent a JUnit file being produced at all + message := "Testcase skipped due to an interrupt" + body := "Terraform received an interrupt and stopped gracefully. This caused all remaining testcases to be skipped" + + return message, body + } + + if file.Status == moduletest.Error { + // Overall test file marked as errored in the context of a skipped test means tests have been skipped after + // a previous test/run blocks has errored out + for i := runIndex; i >= 0; i-- { + if file.Runs[i].Status == moduletest.Error { + // Skipped due to error in previous run within the file + message := "Testcase skipped due to a previous testcase error" + body := fmt.Sprintf("Previous testcase %q ended in error, which caused the remaining tests in the file to be skipped", file.Runs[i].Name) + return message, body + } + } + } + + // Unhandled case: This results in with no attributes or body + return "", "" +} + func suiteFilesAsSortedList(files map[string]*moduletest.File) []*moduletest.File { fileNames := make([]string, len(files)) i := 0 diff --git a/internal/command/junit/junit_internal_test.go b/internal/command/junit/junit_internal_test.go new file mode 100644 index 0000000000..41ab23f82f --- /dev/null +++ b/internal/command/junit/junit_internal_test.go @@ -0,0 +1,153 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 +package junit + +import ( + "bytes" + "fmt" + "os" + "testing" + + "github.com/hashicorp/terraform/internal/moduletest" +) + +func Test_TestJUnitXMLFile_save(t *testing.T) { + + cases := map[string]struct { + filename string + expectError bool + }{ + "can save output to the specified filename": { + filename: func() string { + td := t.TempDir() + return fmt.Sprintf("%s/output.xml", td) + }(), + }, + "returns an error when given a filename that isn't absolute or relative": { + filename: "~/output.xml", + expectError: true, + }, + } + + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + j := TestJUnitXMLFile{ + filename: tc.filename, + } + + xml := []byte(` + + + +`) + + diags := j.save(xml) + + if diags.HasErrors() { + if !tc.expectError { + t.Fatalf("got unexpected error: %s", diags.Err()) + } + // return early if testing error case + return + } + + if !diags.HasErrors() && tc.expectError { + t.Fatalf("expected an error but got none") + } + + fileContent, err := os.ReadFile(tc.filename) + if err != nil { + t.Fatalf("unexpected error opening file") + } + + if !bytes.Equal(fileContent, xml) { + t.Fatalf("wanted XML:\n%s\n got XML:\n%s\n", string(xml), string(fileContent)) + } + }) + } +} + +func Test_suiteFilesAsSortedList(t *testing.T) { + cases := map[string]struct { + Suite *moduletest.Suite + ExpectedNames map[int]string + }{ + "no test files": { + Suite: &moduletest.Suite{}, + }, + "3 test files ordered in map": { + Suite: &moduletest.Suite{ + Status: moduletest.Skip, + Files: map[string]*moduletest.File{ + "test_file_1.tftest.hcl": { + Name: "test_file_1.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + "test_file_2.tftest.hcl": { + Name: "test_file_2.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + "test_file_3.tftest.hcl": { + Name: "test_file_3.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + }, + }, + ExpectedNames: map[int]string{ + 0: "test_file_1.tftest.hcl", + 1: "test_file_2.tftest.hcl", + 2: "test_file_3.tftest.hcl", + }, + }, + "3 test files unordered in map": { + Suite: &moduletest.Suite{ + Status: moduletest.Skip, + Files: map[string]*moduletest.File{ + "test_file_3.tftest.hcl": { + Name: "test_file_3.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + "test_file_1.tftest.hcl": { + Name: "test_file_1.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + "test_file_2.tftest.hcl": { + Name: "test_file_2.tftest.hcl", + Status: moduletest.Skip, + Runs: []*moduletest.Run{}, + }, + }, + }, + ExpectedNames: map[int]string{ + 0: "test_file_1.tftest.hcl", + 1: "test_file_2.tftest.hcl", + 2: "test_file_3.tftest.hcl", + }, + }, + } + + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + list := suiteFilesAsSortedList(tc.Suite.Files) + + if len(tc.ExpectedNames) != len(tc.Suite.Files) { + t.Fatalf("expected there to be %d items, got %d", len(tc.ExpectedNames), len(tc.Suite.Files)) + } + + if len(tc.ExpectedNames) == 0 { + return + } + + for k, v := range tc.ExpectedNames { + if list[k].Name != v { + t.Fatalf("expected element %d in sorted list to be named %s, got %s", k, v, list[k].Name) + } + } + }) + } +} diff --git a/internal/command/junit/junit_test.go b/internal/command/junit/junit_test.go index 6c8444a306..956571e561 100644 --- a/internal/command/junit/junit_test.go +++ b/internal/command/junit/junit_test.go @@ -1,6 +1,6 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: BUSL-1.1 -package junit +package junit_test import ( "bytes" @@ -8,6 +8,8 @@ import ( "os" "testing" + "github.com/hashicorp/terraform/internal/backend/local" + "github.com/hashicorp/terraform/internal/command/junit" "github.com/hashicorp/terraform/internal/configs/configload" "github.com/hashicorp/terraform/internal/moduletest" ) @@ -20,12 +22,16 @@ func Test_TestJUnitXMLFile_Save(t *testing.T) { cases := map[string]struct { filename string + runner *local.TestSuiteRunner suite moduletest.Suite expectedOuput []byte expectError bool }{ - "renders output indicating when tests are skipped": { + " element can explain when skip is due to the runner being stopped by an interrupt": { filename: "output.xml", + runner: &local.TestSuiteRunner{ + Stopped: true, + }, suite: moduletest.Suite{ Status: moduletest.Skip, Files: map[string]*moduletest.File{ @@ -42,6 +48,67 @@ func Test_TestJUnitXMLFile_Save(t *testing.T) { }, }, expectedOuput: []byte(` + + + + + +`), + }, + " element can explain when skip is due to the previously errored runs/testcases in the file": { + filename: "output.xml", + runner: &local.TestSuiteRunner{}, + suite: moduletest.Suite{ + Status: moduletest.Error, + Files: map[string]*moduletest.File{ + "file1.tftest.hcl": { + Name: "file1.tftest.hcl", + Status: moduletest.Error, + Runs: []*moduletest.Run{ + { + Name: "my_test_1", + Status: moduletest.Error, + }, + { + Name: "my_test_2", + Status: moduletest.Skip, + }, + }, + }, + }, + }, + expectedOuput: []byte(` + + + + + + + + +`), + }, + " element is present without additional details when contextual data is not available": { + filename: "output.xml", + runner: &local.TestSuiteRunner{ + // No data about being stopped + }, + suite: moduletest.Suite{ + Status: moduletest.Pending, + Files: map[string]*moduletest.File{ + "file1.tftest.hcl": { + Name: "file1.tftest.hcl", + Status: moduletest.Pending, + Runs: []*moduletest.Run{ + { + Name: "my_test", + Status: moduletest.Skip, // Only run present is skipped, no previous errors + }, + }, + }, + }, + }, + expectedOuput: []byte(` @@ -60,10 +127,7 @@ func Test_TestJUnitXMLFile_Save(t *testing.T) { loader, cleanup := configload.NewLoaderForTests(t) defer cleanup() - j := TestJUnitXMLFile{ - filename: path, - configLoader: loader, - } + j := junit.NewTestJUnitXMLFile(path, loader, tc.runner) // Process data & save file j.Save(&tc.suite) @@ -81,144 +145,3 @@ func Test_TestJUnitXMLFile_Save(t *testing.T) { } } - -func Test_TestJUnitXMLFile_save(t *testing.T) { - - cases := map[string]struct { - filename string - expectError bool - }{ - "can save output to the specified filename": { - filename: func() string { - td := t.TempDir() - return fmt.Sprintf("%s/output.xml", td) - }(), - }, - "returns an error when given a filename that isn't absolute or relative": { - filename: "~/output.xml", - expectError: true, - }, - } - - for tn, tc := range cases { - t.Run(tn, func(t *testing.T) { - j := TestJUnitXMLFile{ - filename: tc.filename, - } - - xml := []byte(` - - - -`) - - diags := j.save(xml) - - if diags.HasErrors() { - if !tc.expectError { - t.Fatalf("got unexpected error: %s", diags.Err()) - } - // return early if testing error case - return - } - - if !diags.HasErrors() && tc.expectError { - t.Fatalf("expected an error but got none") - } - - fileContent, err := os.ReadFile(tc.filename) - if err != nil { - t.Fatalf("unexpected error opening file") - } - - if !bytes.Equal(fileContent, xml) { - t.Fatalf("wanted XML:\n%s\n got XML:\n%s\n", string(xml), string(fileContent)) - } - }) - } -} - -func Test_suiteFilesAsSortedList(t *testing.T) { - cases := map[string]struct { - Suite *moduletest.Suite - ExpectedNames map[int]string - }{ - "no test files": { - Suite: &moduletest.Suite{}, - }, - "3 test files ordered in map": { - Suite: &moduletest.Suite{ - Status: moduletest.Skip, - Files: map[string]*moduletest.File{ - "test_file_1.tftest.hcl": { - Name: "test_file_1.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - "test_file_2.tftest.hcl": { - Name: "test_file_2.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - "test_file_3.tftest.hcl": { - Name: "test_file_3.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - }, - }, - ExpectedNames: map[int]string{ - 0: "test_file_1.tftest.hcl", - 1: "test_file_2.tftest.hcl", - 2: "test_file_3.tftest.hcl", - }, - }, - "3 test files unordered in map": { - Suite: &moduletest.Suite{ - Status: moduletest.Skip, - Files: map[string]*moduletest.File{ - "test_file_3.tftest.hcl": { - Name: "test_file_3.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - "test_file_1.tftest.hcl": { - Name: "test_file_1.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - "test_file_2.tftest.hcl": { - Name: "test_file_2.tftest.hcl", - Status: moduletest.Skip, - Runs: []*moduletest.Run{}, - }, - }, - }, - ExpectedNames: map[int]string{ - 0: "test_file_1.tftest.hcl", - 1: "test_file_2.tftest.hcl", - 2: "test_file_3.tftest.hcl", - }, - }, - } - - for tn, tc := range cases { - t.Run(tn, func(t *testing.T) { - list := suiteFilesAsSortedList(tc.Suite.Files) - - if len(tc.ExpectedNames) != len(tc.Suite.Files) { - t.Fatalf("expected there to be %d items, got %d", len(tc.ExpectedNames), len(tc.Suite.Files)) - } - - if len(tc.ExpectedNames) == 0 { - return - } - - for k, v := range tc.ExpectedNames { - if list[k].Name != v { - t.Fatalf("expected element %d in sorted list to be named %s, got %s", k, v, list[k].Name) - } - } - }) - } -} diff --git a/internal/command/test.go b/internal/command/test.go index f6759cd220..38ccf46f50 100644 --- a/internal/command/test.go +++ b/internal/command/test.go @@ -200,7 +200,6 @@ func (c *TestCommand) Run(rawArgs []string) int { Streams: c.Streams, } } else { - localRunner := &local.TestSuiteRunner{ Config: config, // The GlobalVariables are loaded from the @@ -223,7 +222,7 @@ func (c *TestCommand) Run(rawArgs []string) int { // JUnit output is only compatible with local test execution if args.JUnitXMLFile != "" { // Make sure TestCommand's calls loadConfigWithTests before this code, so configLoader is not nil - localRunner.JUnit = junit.NewTestJUnitXMLFile(args.JUnitXMLFile, c.configLoader) + localRunner.JUnit = junit.NewTestJUnitXMLFile(args.JUnitXMLFile, c.configLoader, localRunner) } runner = localRunner diff --git a/internal/moduletest/suite.go b/internal/moduletest/suite.go index 9c5dded67a..b7a165d4a7 100644 --- a/internal/moduletest/suite.go +++ b/internal/moduletest/suite.go @@ -15,4 +15,8 @@ type TestSuiteRunner interface { Test() (Status, tfdiags.Diagnostics) Stop() Cancel() + + // IsStopped allows code outside the moduletest package to confirm the suite was stopped + // when handling a graceful exit scenario + IsStopped() bool }