Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mitigate test2json bug causing missing test output #102

Merged
merged 3 commits into from
Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/junitxml/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,16 @@ func packageTestCases(pkg *testjson.Package, formatClassname FormatFunc) []JUnit
jtc := newJUnitTestCase(tc, formatClassname)
jtc.Failure = &JUnitFailure{
Message: "Failed",
Contents: pkg.Output(tc.Test),
Contents: strings.Join(pkg.OutputLines(tc), ""),
}
cases = append(cases, jtc)
}

for _, tc := range pkg.Skipped {
jtc := newJUnitTestCase(tc, formatClassname)
jtc.SkipMessage = &JUnitSkipMessage{Message: pkg.Output(tc.Test)}
jtc.SkipMessage = &JUnitSkipMessage{
Message: strings.Join(pkg.OutputLines(tc), ""),
}
cases = append(cases, jtc)
}

Expand Down
101 changes: 87 additions & 14 deletions testjson/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ type Package struct {
Failed []TestCase
Skipped []TestCase
Passed []TestCase
output map[string][]string
// output printed by test cases. Output is stored first by root TestCase
// name, then by subtest name to mitigate github.com/golang/go/issues/29755.
// In the future when that bug is fixed this can be reverted to store all
// output by full test name.
output map[string]map[string][]string
// coverage stores the code coverage output for the package without the
// trailing newline (ex: coverage: 91.1% of statements).
coverage string
Expand Down Expand Up @@ -105,8 +109,68 @@ func (p Package) TestCases() []TestCase {
}

// Output returns the full test output for a test.
//
// Unlike OutputLines() it does not return any extra lines in some cases.
func (p Package) Output(test string) string {
return strings.Join(p.output[test], "")
root, sub := splitTestName(test)
return strings.Join(p.output[root][sub], "")
}

// OutputLines returns the full test output for a test as a slice of strings.
//
// As a workaround for test output being attributed to the wrong subtest, if:
// - the requested test output only contains framing lines (ex: --- FAIL: ), and
// - the TestCase is a root TestCase (not a subtest), and
// - the TestCase has no subtest failures;
// then all output for every subtest under the root test is returned.
// See https://github.com/golang/go/issues/29755.
func (p Package) OutputLines(tc TestCase) []string {
root, sub := splitTestName(tc.Test)
lines := p.output[root][sub]
// every test will have 2 framing lines, === RUN, and --- {PASS,FAIL}
if len(lines) > 2 {
return lines
}

// If this is a subtest, or a root test case with subtest failures the
// subtest failure output should contain the relevant lines, so we don't need
// extra lines.
if sub != "" || tc.subTestFailed {
return lines
}
//
result := make([]string, 0, len(p.output[root])*2)
for _, sub := range testNamesSorted(p.output[root]) {
result = append(result, p.output[root][sub]...)
}
return result
}

func testNamesSorted(subs map[string][]string) []string {
names := make([]string, 0, len(subs))
for name := range subs {
names = append(names, name)
}
sort.Strings(names)
return names
}

func (p Package) addOutput(test string, output string) {
root, sub := splitTestName(test)
if p.output[root] == nil {
p.output[root] = make(map[string][]string)
}
// TODO: limit size of buffered test output
p.output[root][sub] = append(p.output[root][sub], output)
}

// splitTestName into root test name and any subtest names.
func splitTestName(name string) (root, sub string) {
parts := strings.SplitN(name, "/", 2)
if len(parts) < 2 {
return name, ""
}
return parts[0], parts[1]
}

// TestMainFailed returns true if the package failed, but there were no tests.
Expand All @@ -130,11 +194,14 @@ type TestCase struct {
Package string
Test string
Elapsed time.Duration
// subTestFailed is true when a subtest of this TestCase has failed. It is
// used to find root TestCases which have no failing subtests.
subTestFailed bool
}

func newPackage() *Package {
return &Package{
output: make(map[string][]string),
output: make(map[string]map[string][]string),
running: make(map[string]TestCase),
}
}
Expand Down Expand Up @@ -171,7 +238,7 @@ func (e *Execution) addPackageEvent(pkg *Package, event TestEvent) {
if isCachedOutput(event.Output) {
pkg.cached = true
}
pkg.output[""] = append(pkg.output[""], event.Output)
pkg.addOutput("", event.Output)
}
}

Expand All @@ -185,8 +252,7 @@ func (e *Execution) addTestEvent(pkg *Package, event TestEvent) {
}
return
case ActionOutput, ActionBench:
// TODO: limit size of buffered test output
pkg.output[event.Test] = append(pkg.output[event.Test], event.Output)
pkg.addOutput(event.Test, event.Output)
return
case ActionPause, ActionCont:
return
Expand All @@ -199,6 +265,14 @@ func (e *Execution) addTestEvent(pkg *Package, event TestEvent) {
switch event.Action {
case ActionFail:
pkg.Failed = append(pkg.Failed, tc)

// If this is a subtest, mark the root test as having subtests.
root, subTest := splitTestName(event.Test)
if subTest != "" {
rootTestCase := pkg.running[root]
rootTestCase.subTestFailed = true
pkg.running[root] = rootTestCase
}
case ActionSkip:
pkg.Skipped = append(pkg.Skipped, tc)
case ActionPass:
Expand All @@ -222,14 +296,13 @@ func isCachedOutput(output string) bool {
return strings.Contains(output, "\t(cached)")
}

// Output returns the full test output for a test.
func (e *Execution) Output(pkg, test string) string {
return strings.Join(e.packages[pkg].output[test], "")
}

// OutputLines returns the full test output for a test as an array of lines.
func (e *Execution) OutputLines(pkg, test string) []string {
return e.packages[pkg].output[test]
// OutputLines returns the full test output for a test as an slice of lines.
// This function is a convenient wrapper around Package.OutputLines() to
// support the hiding of output in the summary.
//
// See Package.OutLines() for more details.
func (e *Execution) OutputLines(tc TestCase) []string {
return e.packages[tc.Package].OutputLines(tc)
}

// Package returns the Package by name.
Expand Down
6 changes: 4 additions & 2 deletions testjson/execution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ func TestExecution_Add_PackageCoverage(t *testing.T) {
pkg := exec.Package("mytestpkg")
expected := &Package{
coverage: "coverage: 33.1% of statements",
output: map[string][]string{
"": {"coverage: 33.1% of statements\n"},
output: map[string]map[string][]string{
"": {
"": {"coverage: 33.1% of statements\n"},
},
},
running: map[string]TestCase{},
}
Expand Down
4 changes: 2 additions & 2 deletions testjson/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func shortVerboseFormat(event TestEvent, exec *Execution) (string, error) {
}

case event.Action == ActionFail:
return exec.Output(event.Package, event.Test) + formatTest(), nil
return exec.Package(event.Package).Output(event.Test) + formatTest(), nil

case event.Action == ActionPass:
return formatTest(), nil
Expand Down Expand Up @@ -164,7 +164,7 @@ func shortFormatPackageEvent(event TestEvent, exec *Execution) (string, error) {
func shortWithFailuresFormat(event TestEvent, exec *Execution) (string, error) {
if !event.PackageEvent() {
if event.Action == ActionFail {
return exec.Output(event.Package, event.Test), nil
return exec.Package(event.Package).Output(event.Test), nil
}
return "", nil
}
Expand Down
18 changes: 9 additions & 9 deletions testjson/summary.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,14 +143,14 @@ func countErrors(errors []string) int {
type executionSummary interface {
Failed() []TestCase
Skipped() []TestCase
OutputLines(pkg, test string) []string
OutputLines(TestCase) []string
}

type noOutputSummary struct {
Execution
}

func (s *noOutputSummary) OutputLines(_, _ string) []string {
func (s *noOutputSummary) OutputLines(_ TestCase) []string {
return nil
}

Expand All @@ -173,8 +173,8 @@ func writeTestCaseSummary(out io.Writer, execution executionSummary, conf testCa
RelativePackagePath(tc.Package),
tc.Test,
FormatDurationAsSeconds(tc.Elapsed, 2))
for _, line := range execution.OutputLines(tc.Package, tc.Test) {
if isRunLine(line) || conf.filter(line) {
for _, line := range execution.OutputLines(tc) {
if isRunLine(line) || conf.filter(tc.Test, line) {
continue
}
fmt.Fprint(out, line)
Expand All @@ -186,7 +186,7 @@ func writeTestCaseSummary(out io.Writer, execution executionSummary, conf testCa
type testCaseFormatConfig struct {
header string
prefix string
filter func(string) bool
filter func(testName string, line string) bool
getter func(executionSummary) []TestCase
}

Expand All @@ -195,8 +195,8 @@ func formatFailed() testCaseFormatConfig {
return testCaseFormatConfig{
header: withColor("Failed"),
prefix: withColor("FAIL"),
filter: func(line string) bool {
return strings.HasPrefix(line, "--- FAIL: Test")
filter: func(testName string, line string) bool {
return strings.HasPrefix(line, "--- FAIL: "+testName+" ")
},
getter: func(execution executionSummary) []TestCase {
return execution.Failed()
Expand All @@ -209,8 +209,8 @@ func formatSkipped() testCaseFormatConfig {
return testCaseFormatConfig{
header: withColor("Skipped"),
prefix: withColor("SKIP"),
filter: func(line string) bool {
return strings.HasPrefix(line, "--- SKIP: Test")
filter: func(testName string, line string) bool {
return strings.HasPrefix(line, "--- SKIP: "+testName+" ")
},
getter: func(execution executionSummary) []TestCase {
return execution.Skipped()
Expand Down
53 changes: 44 additions & 9 deletions testjson/summary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,17 @@ func TestPrintSummary_WithFailures(t *testing.T) {
Elapsed: 12 * time.Millisecond,
},
},
output: map[string][]string{
output: map[string]map[string][]string{
"TestFileDo": multiLine(`=== RUN TestFileDo
Some stdout/stderr here
--- FAIL: TestFailDo (1.41s)
--- FAIL: TestFileDo (1.41s)
do_test.go:33 assertion failed
`),
"TestFileDoError": multiLine(`=== RUN TestFileDoError
--- FAIL: TestFailDoError (0.01s)
--- FAIL: TestFileDoError (0.01s)
do_test.go:50 assertion failed: expected nil error, got WHAT!
`),
"": {"FAIL\n"},
"": multiLine("FAIL\n"),
},
action: ActionFail,
},
Expand All @@ -118,7 +118,7 @@ Some stdout/stderr here
Elapsed: 0,
},
},
output: map[string][]string{
output: map[string]map[string][]string{
"TestAlbatross": multiLine(`=== RUN TestAlbatross
--- FAIL: TestAlbatross (0.04s)
`),
Expand All @@ -130,8 +130,8 @@ Some stdout/stderr here
},
"example.com/project/badmain": {
action: ActionFail,
output: map[string][]string{
"": {"sometimes main can exit 2\n"},
output: map[string]map[string][]string{
"": multiLine("sometimes main can exit 2\n"),
},
},
},
Expand Down Expand Up @@ -220,13 +220,16 @@ func patchClock() (clockwork.FakeClock, func()) {
return fake, func() { clock = clockwork.NewRealClock() }
}

func multiLine(s string) []string {
return strings.SplitAfter(s, "\n")
func multiLine(s string) map[string][]string {
return map[string][]string{
"": strings.SplitAfter(s, "\n"),
}
}

func TestPrintSummary_MissingTestFailEvent(t *testing.T) {
_, reset := patchClock()
defer reset()

exec, err := ScanTestOutput(ScanConfig{
Stdout: bytes.NewReader(golden.Get(t, "go-test-json-missing-test-fail.out")),
Stderr: bytes.NewReader(nil),
Expand All @@ -248,3 +251,35 @@ func (s noopHandler) Event(TestEvent, *Execution) error {
func (s noopHandler) Err(string) error {
return nil
}

func TestPrintSummary_WithMisattributedOutput(t *testing.T) {
_, reset := patchClock()
defer reset()

exec, err := ScanTestOutput(ScanConfig{
Stdout: bytes.NewReader(golden.Get(t, "go-test-json-misattributed.out")),
Stderr: bytes.NewBuffer(nil),
Handler: noopHandler{},
})
assert.NilError(t, err)

buf := new(bytes.Buffer)
PrintSummary(buf, exec, SummarizeAll)
golden.Assert(t, buf.String(), "summary-misattributed-output")
}

func TestPrintSummary_WithSubtestFailures(t *testing.T) {
_, reset := patchClock()
defer reset()

exec, err := ScanTestOutput(ScanConfig{
Stdout: bytes.NewReader(golden.Get(t, "go-test-json.out")),
Stderr: bytes.NewBuffer(nil),
Handler: noopHandler{},
})
assert.NilError(t, err)

buf := new(bytes.Buffer)
PrintSummary(buf, exec, SummarizeAll)
golden.Assert(t, buf.String(), "summary-root-test-has-subtest-failures")
}
35 changes: 35 additions & 0 deletions testjson/testdata/go-test-json-misattributed.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{"Action":"run","Test":"TestOutputWithSubtest"}
{"Action":"output","Test":"TestOutputWithSubtest","Output":"=== RUN TestOutputWithSubtest\n"}
{"Action":"run","Test":"TestOutputWithSubtest/sub_test"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test","Output":"=== RUN TestOutputWithSubtest/sub_test\n"}
{"Action":"run","Test":"TestOutputWithSubtest/sub_test/sub2"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test/sub2","Output":"=== RUN TestOutputWithSubtest/sub_test/sub2\n"}
{"Action":"run","Test":"TestOutputWithSubtest/sub_test2"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2","Output":"=== RUN TestOutputWithSubtest/sub_test2\n"}
{"Action":"run","Test":"TestOutputWithSubtest/sub_test2/sub2"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2/sub2","Output":"=== RUN TestOutputWithSubtest/sub_test2/sub2\n"}
{"Action":"output","Test":"TestOutputWithSubtest","Output":"--- FAIL: TestOutputWithSubtest (0.00s)\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test","Output":" --- PASS: TestOutputWithSubtest/sub_test (0.00s)\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test","Output":" foo_test.go:9: output from sub test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test","Output":" foo_test.go:11: more output from sub test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test","Output":" foo_test.go:16: more output from sub test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test/sub2","Output":" --- PASS: TestOutputWithSubtest/sub_test/sub2 (0.00s)\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test/sub2","Output":" foo_test.go:14: output from sub2 test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test/sub2","Output":" foo_test.go:22: output from root test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test/sub2","Output":" foo_test.go:27: output from root test\n"}
{"Action":"pass","Test":"TestOutputWithSubtest/sub_test/sub2"}
{"Action":"pass","Test":"TestOutputWithSubtest/sub_test"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2","Output":" --- PASS: TestOutputWithSubtest/sub_test2 (0.00s)\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2","Output":" foo_test.go:21: output from sub test2\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2","Output":" foo_test.go:23: more output from sub test2\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2","Output":" foo_test.go:28: more output from sub test2\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2/sub2","Output":" --- PASS: TestOutputWithSubtest/sub_test2/sub2 (0.00s)\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2/sub2","Output":" foo_test.go:26: output from sub2 test\n"}
{"Action":"output","Test":"TestOutputWithSubtest/sub_test2/sub2","Output":" foo_test.go:32: output after sub test\n"}
{"Action":"pass","Test":"TestOutputWithSubtest/sub_test2/sub2"}
{"Action":"pass","Test":"TestOutputWithSubtest/sub_test2"}
{"Action":"fail","Test":"TestOutputWithSubtest"}
{"Action":"output","Output":"FAIL\n"}
{"Action":"output","Output":"FAIL\tgotest.tools/gotestsum/foo\t0.001s\n"}
{"Action":"output","Output":"FAIL\n"}
{"Action":"fail"}
Loading