Add junit support for non-e2e tests/multi-suite #851

johnSchnake · 2019-08-21T19:27:10Z

What this PR does / why we need it:
The e2e tests report via junit but with the top-level item
in the xml being a <testsuite>. Other implementations seem
to default to allowing multiple testsuites and so they have a
higher level testsuites object.

This PR enables a new post-processing path and makes the older
junit parsing only apply to the result-type e2e. The new,
junit result-type will use this more generalized logic.

Which issue(s) this PR fixes
Fixes #849

Special notes for your reviewer:
TBD; need to discuss this because the above commit message isn't accurate, nor is the issue I originally filed. I thought we were setting the result format for e2e but we were not; we were using junit. So this change makes older sonobuoy gen output no longer get post-processed correctly.

That is a brand new feature so maybe not a huge deal, but we should consider not changing that.
Instead of junit being the 'new' format, maybe we continue to let junit be the e2e-junit/single-suite and we introduce a new junit-suites or something similar for the new format.

Release note:

JUnit output which containts a `testsuites` object consisting of multiple test suites can now be post-processed by setting the result-format to `TBD`

johnSchnake · 2019-08-22T14:34:47Z

After discussion on this we decided that the preferred route would be to try and make the junit result-format be compatible with both.

There will be a bit more work to accomplish this but we want to try and avoid having N different flavors of junit. Instead, it would be preferable to have logic that will figure them all out and report on them as needed.

johnSchnake · 2019-08-22T16:46:16Z

pkg/client/e2e.go

@@ -27,16 +27,15 @@ import (

 	"github.com/heptio/sonobuoy/pkg/client/results"
 	"github.com/heptio/sonobuoy/pkg/client/results/e2e"
-	"github.com/onsi/ginkgo/reporters"


Now use our own type which is an amalgamation of the ginkgo/reporters and the jstemmer types. Paired with a custom unmarshaller, this allows us to handle both cases with a single type.

johnSchnake · 2019-08-22T16:46:44Z

pkg/client/e2e.go

@@ -46,7 +45,7 @@ func (*SonobuoyClient) GetTests(reader io.Reader, show string) ([]reporters.JUni
 			}
 			// TODO(chuckha) consider reusing this function for any generic e2e-esque plugin results.
 			// TODO(chuckha) consider using path.Join()
-			if path == e2eJunitPath || path == legacye2eJunitPath {
+			if path == e2eJUnitPath || path == legacye2eJUnitPath {


Updated a bunch of little lines for consistent naming: JUnit preferred over Junit

johnSchnake · 2019-08-22T16:47:52Z

pkg/client/e2e.go

 	if show == "passed" || show == "all" {
-		out = append(out, results.Filter(results.Passed, junitResults)...)
+		out = append(out, results.JUnitFilter(results.JUnitPassed, junitResults)...)


Although not strictly requiring this prefix now that there isnt a junit and a e2e filter/passed/etc, it makes sense to me to leave a prefix since this is type specific.

As we add other formats we will probably need similar named things. At that point we may also want to consider some interface to do this instead though.

pkg/client/gen.go

johnSchnake · 2019-08-22T16:49:01Z

pkg/client/results/junit.go

+// testsuite whereas other tools report a top-level testsuites object
+// which may have 1+ testsuite children. Only one of the fields should be
+// set, not both.
+type junitResult struct {


This type is "where the magic happens" since it has a custom unmarshal method which will handle either case based on the top level object.

johnSchnake · 2019-08-22T16:50:05Z

pkg/client/results/junit.go

+	suites JUnitTestSuites
+}
+
+// UnmarshalXML will unmarshal the document into either the 'Suites'


Needs updated; in another implementation I had Suites and Suite and only one would get set. Instead, now there is just suites and we will just add the lone suite to the list if that is what we are given.

johnSchnake · 2019-08-22T16:51:02Z

pkg/client/results/junit.go

+// which may have 1+ testsuite children. Only one of the fields should be
+// set, not both.
+type junitResult struct {
+	suites JUnitTestSuites


This is slightly awkward naming but I dont have a better way to name this really. It just leads to junitResults.suites.Suites.... I hate the repetition but I couldn't come up with something that would be clear/intutive/didnt repeat.

johnSchnake · 2019-08-22T16:52:04Z

pkg/client/results/junit.go

+	}
+	for i := range j.suites.Suites {
+		if j.suites.Suites[i].Name == "" {
+			j.suites.Suites[i].Name = fmt.Sprintf("testsuite-%03d", i+1)


When processing junit results, each suite is on its own branch. As a result, if you dont have a suite name you get a weird branch that you can't target very well.

In particular, this happens for the e2e tests so I wanted to ensure that non-empty names were set.

johnSchnake · 2019-08-22T16:52:37Z

pkg/client/results/junit.go

+	SkipMessage *JUnitSkipMessage    `xml:"skipped,omitempty"`
+	Failure     *JUnitFailureMessage `xml:"failure,omitempty"`
+	SystemOut   string               `xml:"system-out,omitempty"`
+	SystemErr   string               `xml:"system-err,omitempty"`


New field; not really used right now but could be. Didnt add this into the metadata but could.

Update: Went ahead and added the processing to support this now too. Just seems too intuitive a field to not include

johnSchnake · 2019-08-22T17:40:53Z

pkg/client/results/junit.go

-		resultObj.Status = StatusUnknown
-		resultObj.Metadata["error"] = err.Error()
-		return resultObj, errors.Wrap(err, "error decoding json into object")
+	resultObj, err = junitProcessReader(


Removed all the file handling and put the rest of the logic into this new method.

pkg/client/results/processing.go

johnSchnake · 2019-08-22T17:44:21Z

pkg/client/results/testdata/mockResults/plugins/job-junit-02/job-junit-02.golden.json

The main thing that changes in all of these is that the entire 'testsuite' is put into a new level. This way when you have N test suites, each is on their own branch and can be filtered/walked appropriately.

There are a number of these golden files where it looks like the failure messages aren't being recorded. They seem to get captured correctly in junit_good_junit.xml file though.

💯 thanks for the catch. When developing I do tend to be a little less than critical with my review of those goldenfiles.

Probably worth it to do something like change (for tests) to MarshalIndent (with 0 indent so changes in depth don't cascade to every sub item) so that the diffs are more clean.

I think that would help. It can be really easy to miss changes otherwise. I didn't notice the issue on my first look at this change :)

pkg/client/testdata/ssh.golden

johnSchnake · 2019-08-22T17:49:57Z

This will fail CI now because nothing uses the ginkgo reporters and they can be removed; will leave that commit out for now just for clarity.

That diff will be 16 files changed, 1664 deletions(-)

codecov-io · 2019-08-22T18:09:12Z

Codecov Report

Merging #851 into master will increase coverage by 0.46%.
The diff coverage is 90.21%.

@@            Coverage Diff             @@
##           master     #851      +/-   ##
==========================================
+ Coverage   46.54%   47.01%   +0.46%     
==========================================
  Files          75       75              
  Lines        4830     4867      +37     
==========================================
+ Hits         2248     2288      +40     
+ Misses       2453     2449       -4     
- Partials      129      130       +1

Impacted Files	Coverage Δ
pkg/client/results/raw.go	`53.84% <100%> (ø)`	⬆️
pkg/client/results/processing.go	`81.81% <100%> (ø)`	⬆️
pkg/client/results/junit.go	`80.21% <89.33%> (+16.58%)`	⬆️
pkg/client/e2e.go	`89.74% <91.66%> (ø)`	⬆️
pkg/plugin/aggregation/aggregator.go	`79.2% <0%> (-4%)`	⬇️
pkg/plugin/driver/job/job.go	`74.19% <0%> (+0.2%)`	⬆️
pkg/plugin/driver/daemonset/daemonset.go	`66.25% <0%> (+0.21%)`	⬆️
cmd/sonobuoy/app/worker.go	`10.63% <0%> (+0.96%)`	⬆️
cmd/sonobuoy/app/status.go	`59.43% <0%> (+1.88%)`	⬆️
cmd/sonobuoy/app/master.go	`50% <0%> (+10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 451b313...9ba8ede. Read the comment docs.

zubron

This is looking good so far, thanks @johnSchnake. The only issue I've found is that there are some more significant changes to the existing golden file tests, the failure messages aren't being included in the output.

zubron · 2019-08-23T12:45:21Z

pkg/client/results/junit_test.go

+	}{
+		{
+			desc:  "top-level testsuites",
+			input: `<testsuites><testsuite name="testsuite1"></testsuite><testsuite name="testsuite2"></testsuite></testsuites>`,


Thanks for adding these tests. I know some of this functionality is covered in the other tests but I like having this where the data isn't in a separate file and the behaviour is clear 😄

zubron · 2019-08-23T13:18:10Z

pkg/client/results/testdata/mockResults/plugins/job-junit-02/job-junit-02.golden.json

There are a number of these golden files where it looks like the failure messages aren't being recorded. They seem to get captured correctly in junit_good_junit.xml file though.

zubron

Thanks for making the changes!

johnSchnake · 2019-08-23T14:36:43Z

After merging the easy-json-diff PR I can see I still have a small problem here. Going to fix up.

johnSchnake · 2019-08-23T14:37:40Z

pkg/client/results/testdata/mockResults/plugins/ds-junit-02/ds-junit-02.golden.json

 "name": "[k8s.io] Pods should be submitted and removed [NodeConformance] [Conformance]",
 "status": "passed"
 },
 {
 "name": "[sig-apps] Daemon set [Serial] should rollback without unnecessary restarts [Conformance]",
 "status": "failed",
 "details": {
-"failure": "/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:696\nConformance test suite needs a cluster with at least 2 nodes.\nExpected\n    \u003cint\u003e: 1\nto be \u003e\n    \u003cint\u003e: 1\n/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/apps/daemon_set.go:385",


This should still be here; not sure why it got removed. I see a few other tests seem to show the failure stuck around... Should be an easy fix when I find it.

The problem seems to be that I was looking for the failure.Message but that is 0, this is the chardata from the xml which (in our datatype) is called Contents.

The fix is fast (use the new field) but raises the question about what we should do with this new field. Should we just concat the message/contents or should we save them separately? Going to take a peek at some of the other junit (non-e2e) and see what makes sense there to compare.

:) So it seems they just use the opposite on the other output I've tested against (chef/inspec with junit output). They only use the message for the failure message and https://github.com/inspec/inspec/blob/a35d37043e65ce5fd528d58cd48ecb369c74a432/lib/inspec/reporters/junit.rb#L48-L54 seems to show they never even use the contents.

Settled on joining message/contents together (space separated) so the data is all present. Users could still grep the data and have one main location to look still. I thought this was better than adding a new Details entry for every type of attribute/field. Open to discussion though.

The e2e tests report via junit but with the top-level item in the xml being a `<testsuite>`. Other implementations seem to default to allowing multiple testsuites and so they have a higher level `testsuites` object. This PR enables a new post-processing path and makes the older junit parsing only apply to the result-type `e2e`. The new, `junit` result-type will use this more generalized logic. Fixes #849 Signed-off-by: John Schnake <[email protected]>

johnSchnake · 2019-08-23T15:06:29Z

pkg/client/results/junit.go

+			// the values, separated with a space.
+			hasFailureContents := (t.Failure != nil && (t.Failure.Message != "" || t.Failure.Contents != ""))
+			if hasFailureContents {
+				testItem.Details[JUnitFailureKey] = strings.TrimSpace(t.Failure.Message + " " + t.Failure.Contents)


This (and the lines above it) where the most recent change. Just checks that either value exists and joins them (trimming spaces at the end).

I think this is fine. You could also build the string up gradually by checking whether either Message or Contents was empty and appending if they aren't but I don't feel particularly strongly about it. I'm happy for you to leave it as is.

johnSchnake · 2019-08-23T15:07:40Z

@zubron , rerequesting review since I found issues/resolved them again. Open to discussion if you dont agree with my approach.

zubron · 2019-08-23T15:31:53Z

pkg/client/results/junit.go

+			// the values, separated with a space.
+			hasFailureContents := (t.Failure != nil && (t.Failure.Message != "" || t.Failure.Contents != ""))
+			if hasFailureContents {
+				testItem.Details[JUnitFailureKey] = strings.TrimSpace(t.Failure.Message + " " + t.Failure.Contents)


I think this is fine. You could also build the string up gradually by checking whether either Message or Contents was empty and appending if they aren't but I don't feel particularly strongly about it. I'm happy for you to leave it as is.

johnSchnake requested a review from zubron August 21, 2019 19:27

johnSchnake commented Aug 22, 2019

View reviewed changes

pkg/client/gen.go Outdated Show resolved Hide resolved

johnSchnake commented Aug 22, 2019

View reviewed changes

pkg/client/results/processing.go Outdated Show resolved Hide resolved

johnSchnake commented Aug 22, 2019

View reviewed changes

pkg/client/testdata/ssh.golden Outdated Show resolved Hide resolved

zubron reviewed Aug 23, 2019

View reviewed changes

zubron approved these changes Aug 23, 2019

View reviewed changes

johnSchnake commented Aug 23, 2019

View reviewed changes

johnSchnake requested a review from zubron August 23, 2019 15:07

zubron approved these changes Aug 23, 2019

View reviewed changes

johnSchnake merged commit a268401 into vmware-tanzu:master Aug 23, 2019

johnSchnake deleted the multiJunitSuites branch August 23, 2019 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add junit support for non-e2e tests/multi-suite #851

Add junit support for non-e2e tests/multi-suite #851

johnSchnake commented Aug 21, 2019

johnSchnake commented Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 23, 2019

johnSchnake Aug 22, 2019

johnSchnake Aug 22, 2019

zubron Aug 23, 2019

johnSchnake Aug 23, 2019

zubron Aug 23, 2019

johnSchnake Aug 23, 2019

johnSchnake commented Aug 22, 2019

codecov-io commented Aug 22, 2019 •

edited

Loading

zubron left a comment

zubron Aug 23, 2019

zubron Aug 23, 2019

zubron left a comment

johnSchnake commented Aug 23, 2019

johnSchnake Aug 23, 2019

johnSchnake Aug 23, 2019

johnSchnake Aug 23, 2019

johnSchnake Aug 23, 2019

johnSchnake Aug 23, 2019

zubron Aug 23, 2019

johnSchnake commented Aug 23, 2019

zubron Aug 23, 2019

Add junit support for non-e2e tests/multi-suite #851

Add junit support for non-e2e tests/multi-suite #851

Conversation

johnSchnake commented Aug 21, 2019

johnSchnake commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnSchnake commented Aug 22, 2019

codecov-io commented Aug 22, 2019 • edited Loading

Codecov Report

zubron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zubron left a comment

Choose a reason for hiding this comment

johnSchnake commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnSchnake commented Aug 23, 2019

Choose a reason for hiding this comment

codecov-io commented Aug 22, 2019 •

edited

Loading