Correctly parse nested go tests #16817

smarterclayton · 2017-10-12T02:51:13Z

Use a stateful parser approach to process each section of the test
output independently. Correctly handles nested tests and nested output
and puts any stdout/err into the SystemOut field of the generated test
result.

Note: this breaks --suite=nested, but was not able to find any users.

@liggitt @stevekuznetsov tests are updated but want to soak this a bit

smarterclayton · 2017-10-13T17:47:41Z

/test integration
/test unit

smarterclayton · 2017-10-13T17:47:47Z

Where's a flake when you need one.

smarterclayton · 2017-10-13T21:21:51Z

/test integration
/test unit

smarterclayton · 2017-10-13T22:24:06Z

/test all

smarterclayton · 2017-10-14T05:44:01Z

/test integration

smarterclayton · 2017-10-14T17:42:10Z

/test integration

smarterclayton · 2017-10-14T19:01:49Z

Ready for review. Biggest change is that integration runner now only returns the last 20k of any successful job. I also ended up doing a bit of a hack to split the output since there's no perfect way to nest the results for consumption by the calling go test.

smarterclayton · 2017-10-14T20:21:25Z

/test integration

enj

I have no idea what most of this does 😄

enj · 2017-10-16T13:02:46Z

test/integration/runner/runner_test.go

 		}
 		return err
 	}

 	if testing.Verbose() {
-		t.Log(string(out))
+		if len(out) < 20000 {


I thought you wanted the last 20k lines - this seems to ignore anything longer than 20k.

I can change but last is only slightly better

stevekuznetsov

Can we please factor this to be a net-new parser that doesn't support nested output?

stevekuznetsov · 2017-10-16T17:42:52Z

test/integration/runner/runner_test.go

 	}
 	return nil
 }

+func transformOutput(out string) string {


Is there a test for this? It's a little dense without test or explanatory comment

Renamed and documented.

It's tested in 17.txt

stevekuznetsov · 2017-10-16T17:44:12Z

tools/junitreport/pkg/parser/gotest/data_parser.go

+// The first submatch of this regex is the result of the test (PASS, FAIL, or SKIP)
+// The second submatch of this regex is the name of the test
+// The third submatch of this regex is the time taken in seconds for the test to finish
+var testResultPattern = regexp.MustCompile(`^(\s*)--- (PASS|FAIL|SKIP):\s+([^\s]+)\s+\((\d+\.\d+)(s| seconds)\)$`)


Comment isn't valid anymore, since the first sub-match is the leading whitespace?

stevekuznetsov · 2017-10-16T17:44:58Z

tools/junitreport/pkg/parser/gotest/data_parser.go

-	testStartPattern  *regexp.Regexp
-	testResultPattern *regexp.Regexp
-}
+var testOutputPattern = regexp.MustCompile(`^(\s*)(.*)$`)

 // MarksBeginning determines if the line marks the beginning of a test case


Godoc drift?

stevekuznetsov · 2017-10-16T17:47:57Z

tools/junitreport/pkg/parser/gotest/data_parser.go

-		return matches[3] + "s", true
+func ExtractOutput(line string) (string, int, bool) {
+	if matches := testOutputPattern.FindStringSubmatch(line); len(matches) > 1 {
+		return matches[2], len(matches[1]) / 4, true


Not clear what the int returned here is ... or why we divide it by four

Will clarify but it's go indentation

stevekuznetsov · 2017-10-16T17:48:16Z

tools/junitreport/pkg/parser/gotest/data_parser.go

-func (p *testDataParser) ExtractResult(line string) (api.TestResult, bool) {
-	if matches := p.testResultPattern.FindStringSubmatch(line); len(matches) > 1 && len(matches[1]) > 0 {
-		switch matches[1] {
+func ExtractResult(line string) (r api.TestResult, name string, depth int, duration string, ok bool) {


An internal type to bind this data together may be less unwieldy

Not worth it

stevekuznetsov · 2017-10-16T17:54:03Z

tools/junitreport/pkg/parser/gotest/parser.go

+					tests[name] = test
+				}
+				orderedTests = append(orderedTests, name)
+				currentTestName = []string{name}


Why is this a slice?

stevekuznetsov · 2017-10-16T17:55:05Z

tools/junitreport/pkg/parser/gotest/parser.go

+					test.SkipMessage = &api.SkipMessage{}
+				}
+				if err := test.SetDuration(duration); err != nil {
+					return nil, fmt.Errorf("unexpected duration on line %d: %s", count, duration)


isnt this a parser failure, not unexpected duration?

Not sure what you mean - the duration is wrog

It's not unexpected that we found the duration, it just doesn't parse

stevekuznetsov · 2017-10-16T17:58:16Z

tools/junitreport/pkg/parser/gotest/parser.go

+					name := currentTestName[len(currentTestName)-1]
+					test := tests[name]
+					switch {
+					case test.FailureOutput != nil, test.SkipMessage != nil:


Switch is idiomatic for more than one condition

There's only one?

There are two conditions

stevekuznetsov · 2017-10-16T17:59:59Z

tools/junitreport/pkg/parser/gotest/parser.go

+				if err := test.SetDuration(duration); err != nil {
+					return nil, fmt.Errorf("unexpected duration on line %d: %s", count, duration)
+				}
+				switch {


Can we hide this behind a stack library so it's easier to read?

I don't think it will help.

I mean, it took me a million and a half years to figure it out ...

I renamed it.

stevekuznetsov · 2017-10-16T18:01:12Z

tools/junitreport/pkg/parser/gotest/parser_flat_test.go

@@ -6,6 +6,8 @@ import (
 	"reflect"
 	"testing"

+	"k8s.io/apimachinery/pkg/util/diff"


Please let's not do this

diff.ObjectReflectDiff is the most useful piece of code I have ever written.

Use a stateful parser approach to process each section of the test output independently. Correctly handles nested tests and nested output and puts any stdout/err into the SystemOut field of the generated test result. Note: this breaks --suite=nested, but was not able to find any users.

Allows junitreport to parse output correctly for nested integration tests.

smarterclayton · 2017-10-17T21:34:54Z

Updated

smarterclayton · 2017-10-19T09:18:09Z

/retest

Flakes, unit verified that test output is correct.

smarterclayton · 2017-10-20T07:27:27Z

/retest

smarterclayton · 2017-10-23T00:09:17Z

/retest

stevekuznetsov · 2017-10-23T14:15:12Z

/lgtm

openshift-merge-robot · 2017-10-23T14:15:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, stevekuznetsov

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~test/integration/OWNERS~~ [smarterclayton,stevekuznetsov]
~~tools/junitreport/OWNERS~~ [smarterclayton,stevekuznetsov]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-10-23T16:00:57Z

Automatic merge from submit-queue (batch tested with PRs 16068, 16817).

openshift-merge-robot assigned liggitt and adelton Oct 12, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2017

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 12, 2017

liggitt assigned stevekuznetsov and unassigned liggitt and adelton Oct 12, 2017

smarterclayton force-pushed the parse_test_data branch 5 times, most recently from bcb4e0e to af44e17 Compare October 13, 2017 16:39

smarterclayton force-pushed the parse_test_data branch from af44e17 to 309eb38 Compare October 13, 2017 20:10

smarterclayton closed this Oct 13, 2017

smarterclayton reopened this Oct 13, 2017

smarterclayton force-pushed the parse_test_data branch from 309eb38 to 76aa972 Compare October 14, 2017 00:39

smarterclayton force-pushed the parse_test_data branch from 76aa972 to 03c848d Compare October 16, 2017 07:18

enj reviewed Oct 16, 2017

View reviewed changes

stevekuznetsov reviewed Oct 16, 2017

View reviewed changes

smarterclayton added 2 commits October 17, 2017 23:34

Split integration test output into two parts

5c502e3

Allows junitreport to parse output correctly for nested integration tests.

smarterclayton force-pushed the parse_test_data branch from 03c848d to 5c502e3 Compare October 17, 2017 21:34

smarterclayton added kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. labels Oct 23, 2017

openshift-merge-robot merged commit 7884876 into openshift:master Oct 23, 2017

Correctly parse nested go tests #16817

Correctly parse nested go tests #16817

Conversation

smarterclayton commented Oct 12, 2017

smarterclayton commented Oct 13, 2017

smarterclayton commented Oct 13, 2017

smarterclayton commented Oct 13, 2017

smarterclayton commented Oct 13, 2017

smarterclayton commented Oct 14, 2017 via email

smarterclayton commented Oct 14, 2017

smarterclayton commented Oct 14, 2017

smarterclayton commented Oct 14, 2017

enj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a 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

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

smarterclayton commented Oct 17, 2017

smarterclayton commented Oct 19, 2017

smarterclayton commented Oct 20, 2017

smarterclayton commented Oct 23, 2017

stevekuznetsov commented Oct 23, 2017

openshift-merge-robot commented Oct 23, 2017

openshift-merge-robot commented Oct 23, 2017