Change JUnit `terraform test` output to include test failure details inside `<failure>` elements, use the error message as the `message` attribute #36316

SarahFrench · 2025-01-14T10:09:14Z

This PR

1. Updates the message attribute on a <failure> element to summarise how many assertions are failing out of a given total, and includes the first failed assertion's error message.
- This is a remaining piece of feedback on the JUnit test report shared in the Discuss forum: https://discuss.hashicorp.com/t/request-for-feedback-on-terraform-test-junit-xml-output/62307/16?u=sarah.french
1. Moves the rendering of the failured message diag into the element's body
- This matches the docs for Azure Devops's use of JUnit XML format
- This also matches feedback in this message from the Discuss forum thread

Update docs for -junit-xml flag

Target Release

1.11.x

CHANGELOG entry

This change is user-facing and I added a changelog entry.
This change is not user-facing.

This change relates to a feature that isn't released yet

SarahFrench · 2025-01-15T21:37:56Z

Questions for the reviewer:

Is there any risk of the message spanning multiple lines? This was the blocker mentioned in the Discuss thread linked in the description
~~Opinions re: the element's body content?~~ This has been discussed

SarahFrench · 2025-01-15T21:40:53Z

internal/command/testdata/test/junit-output/1pass-1fail/expected-output.xml

@@ -1,17 +1,16 @@
 <?xml version="1.0" encoding="UTF-8"?><testsuites>
  <testsuite name="main.tftest.hcl" tests="2" skipped="0" failures="1" errors="0">
    <testcase name="failing_assertion" classname="main.tftest.hcl" time="TIME_REDACTED" timestamp="TIMESTAMP_REDACTED">
-      <failure message="Test run failed"></failure>
+      <failure message="local variable &#39;number&#39; has a value greater than zero, so assertion 2 will fail"><![CDATA[Test failed on assertion 2 of 3]]></failure>


These strings will contain HTML character references, depending on the message. I assume the tools using the XML will parse this fine.

internal/command/junit/junit.go

…error message

… attribute is set

…element

SarahFrench · 2025-01-22T13:43:55Z

internal/command/junit/junit_internal_test.go

+// From internal/backend/remote-state/s3/testing_test.go
+// diagnosticComparer is a Comparer function for use with cmp.Diff to compare two tfdiags.Diagnostic values


Should/could this be moved into the tfdiags package? Or are there other ways that diagnostics are compared in the code base that are a better option?

I think moving it into tfdiags is a good idea!

We could follow the pattern of ctydebug - https://github.com/zclconf/go-cty-debug/blob/0d6042c539401a57fc0cca85ded2861d4a5173c4/ctydebug/cmp.go#L19-L42 I don't mean the implementation necessarily but more the fact that it exports a cmp.Option variable which can be directly used as an argument in cmp.Diff.

radeksimko

LGTM! Feel free to merge and leave the in-line comments for a separate PR at some point later.

radeksimko · 2025-01-22T14:08:21Z

internal/command/junit/junit_internal_test.go

+// From internal/backend/remote-state/s3/testing_test.go
+// diagnosticComparer is a Comparer function for use with cmp.Diff to compare two tfdiags.Diagnostic values


I think moving it into tfdiags is a good idea!

We could follow the pattern of ctydebug - https://github.com/zclconf/go-cty-debug/blob/0d6042c539401a57fc0cca85ded2861d4a5173c4/ctydebug/cmp.go#L19-L42 I don't mean the implementation necessarily but more the fact that it exports a cmp.Option variable which can be directly used as an argument in cmp.Diff.

radeksimko · 2025-01-22T14:08:34Z

internal/command/junit/junit_internal_test.go

@@ -67,6 +70,65 @@ func Test_TestJUnitXMLFile_save(t *testing.T) {
 	}
 }

+func Test_getWarnings(t *testing.T) {
+


Suggested change

radeksimko · 2025-01-22T14:13:18Z

internal/command/junit/junit.go

+func getWarnings(diags tfdiags.Diagnostics) tfdiags.Diagnostics {
+	// Collect warnings
+	warnDiags := tfdiags.Diagnostics{}
+	if diags.HasWarnings() {
+		for _, d := range diags {
+			if d.Severity() == tfdiags.Warning {
+				warnDiags = warnDiags.Append(d)
+			}
+		}
+	}
+	return warnDiags
+}


Nitpick - AFAICT the code here would work just as well without HasWarnings() - it would return empty slice as nothing would be appended.

On a separate note I am somewhat surprised we don't have Warnings() method yet on tfdiags.Diagnostics - so I think it may be another candidate for logic to extract there and just call from here but feel free to leave this for another PR if you like.

SarahFrench · 2025-01-22T15:20:38Z

Thanks! I'll address those in follow-up PRs like you suggested; as long as this PR's changes are in 1.11 then any refactors can be kept on main/in 1.12. As well as those comments I'll address this feedback about making test assertion failure diags more easily identify-able.

vercel bot deployed to Preview January 14, 2025 10:11 View deployment

SarahFrench force-pushed the junit-add-failure-details branch from fb0eeb7 to 1437843 Compare January 15, 2025 20:54

vercel bot deployed to Preview January 15, 2025 21:07 View deployment

vercel bot deployed to Preview January 15, 2025 21:36 View deployment

SarahFrench marked this pull request as ready for review January 15, 2025 21:37

SarahFrench requested a review from a team as a code owner January 15, 2025 21:37

SarahFrench commented Jan 15, 2025

View reviewed changes

vercel bot deployed to Preview January 16, 2025 11:34 View deployment

SarahFrench changed the title ~~Change JUnit terraform test output to include test failure details in <failure> elements~~ Change JUnit terraform test output to include test failure details inside <failure> elements, use the error message as the message attribute Jan 16, 2025

radeksimko reviewed Jan 21, 2025

View reviewed changes

internal/command/junit/junit.go Outdated Show resolved Hide resolved

internal/command/junit/junit.go Outdated Show resolved Hide resolved

internal/command/junit/junit.go Outdated Show resolved Hide resolved

SarahFrench added 6 commits January 21, 2025 12:04

Add details to <failure> element describing which assertion failed

855f2e9

Remove unused diagnostic string

132b8bd

Set message attribute of <failure> element to failed assertion's …

d8da87e

…error message

Make <failure> contain diagnostic's message, refactor how message…

b0ea078

… attribute is set

Ensure that system-err is only added when needed

5aea43e

Update test fixtures

080505c

SarahFrench force-pushed the junit-add-failure-details branch from 338114b to 080505c Compare January 21, 2025 12:08

SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jan 21, 2025

vercel bot deployed to Preview January 21, 2025 12:13 View deployment

SarahFrench added 2 commits January 22, 2025 12:38

Make diags usage clearer, ensure all test failure diags in "failure" …

b6798ae

…element

Refactor how "skipped" element value is set

add24c6

vercel bot deployed to Preview January 22, 2025 13:39 View deployment

SarahFrench commented Jan 22, 2025

View reviewed changes

SarahFrench requested a review from radeksimko January 22, 2025 13:44

Fix failing test Test_TestJUnitXMLFile_Save

0bbcfdd

vercel bot deployed to Preview January 22, 2025 13:51 View deployment

radeksimko approved these changes Jan 22, 2025

View reviewed changes

SarahFrench added the 1.11-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jan 22, 2025

SarahFrench mentioned this pull request Jan 22, 2025

Add reusable Diagnostic comparer to tfdiags package #36385

Merged

2 tasks

SarahFrench merged commit 6b81f71 into main Jan 22, 2025
14 checks passed

SarahFrench deleted the junit-add-failure-details branch January 22, 2025 16:42

github-actions bot mentioned this pull request Jan 22, 2025

Backport of Change JUnit terraform test output to include test failure details inside <failure> elements, use the error message as the message attribute into v1.11 #36386

Merged

3 tasks

SarahFrench mentioned this pull request Jan 22, 2025

Add a Warnings() method to tfdiags.Diagnostics #36387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change JUnit `terraform test` output to include test failure details inside `<failure>` elements, use the error message as the `message` attribute #36316

Change JUnit `terraform test` output to include test failure details inside `<failure>` elements, use the error message as the `message` attribute #36316

SarahFrench commented Jan 14, 2025 •

edited

Loading

SarahFrench commented Jan 15, 2025 •

edited

Loading

SarahFrench Jan 15, 2025

SarahFrench Jan 22, 2025

radeksimko Jan 22, 2025

radeksimko left a comment

radeksimko Jan 22, 2025

radeksimko Jan 22, 2025

radeksimko Jan 22, 2025

SarahFrench commented Jan 22, 2025 •

edited

Loading

		// From internal/backend/remote-state/s3/testing_test.go
		// diagnosticComparer is a Comparer function for use with cmp.Diff to compare two tfdiags.Diagnostic values

Change JUnit terraform test output to include test failure details inside <failure> elements, use the error message as the message attribute #36316

Change JUnit terraform test output to include test failure details inside <failure> elements, use the error message as the message attribute #36316

Conversation

SarahFrench commented Jan 14, 2025 • edited Loading

Target Release

CHANGELOG entry

SarahFrench commented Jan 15, 2025 • edited Loading

SarahFrench Jan 15, 2025

Choose a reason for hiding this comment

SarahFrench Jan 22, 2025

Choose a reason for hiding this comment

radeksimko Jan 22, 2025

Choose a reason for hiding this comment

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Jan 22, 2025

Choose a reason for hiding this comment

radeksimko Jan 22, 2025

Choose a reason for hiding this comment

radeksimko Jan 22, 2025

Choose a reason for hiding this comment

SarahFrench commented Jan 22, 2025 • edited Loading

Change JUnit `terraform test` output to include test failure details inside `<failure>` elements, use the error message as the `message` attribute #36316

Change JUnit `terraform test` output to include test failure details inside `<failure>` elements, use the error message as the `message` attribute #36316

SarahFrench commented Jan 14, 2025 •

edited

Loading

SarahFrench commented Jan 15, 2025 •

edited

Loading

SarahFrench commented Jan 22, 2025 •

edited

Loading