From 260b47cabef4364ee6d6ee757d3e7ce2827c559a Mon Sep 17 00:00:00 2001
From: Brittany Walentin <brittw@uber.com>
Date: Fri, 25 May 2018 10:59:00 -0700
Subject: [PATCH] Addressing code review comments. Mainly: * Moving the
averaging/merging of benchmarks from the parser to the formatter package *
Tightening up the regex so it only captures the numeric values (no more of
trimming spaces and the ns/op) * Deleting the writing up in xml file the
benchmark memory sections of B/op and Allocs/op
Also added a test case for parseNanoseconds().
---
README.md | 8 ++-
formatter/formatter.go | 62 +++++++++++++-------
go-junit-report_test.go | 126 +++++++++++++++++++++++++++++++---------
parser/parser.go | 76 ++++--------------------
parser/parser_test.go | 21 +++++++
testdata/23-report.xml | 4 +-
testdata/24-report.xml | 4 +-
testdata/25-report.xml | 6 +-
testdata/26-report.xml | 6 +-
9 files changed, 185 insertions(+), 128 deletions(-)
diff --git a/README.md b/README.md
index 0ad120a..95d3614 100644
--- a/README.md
+++ b/README.md
@@ -37,9 +37,13 @@ go test -v 2>&1 | go-junit-report > report.xml
Note that it also can parse benchmark output with `-bench` flag:
```bash
- go test -bench . -benchmem -count 100
+go test -v -bench . 2>&1 | ./go-junit-report > report.xml
```
-will return the average mean benchmark time as the test case time.
+
+or using the optional -count parameter:
+```bash
+go test -v -bench . -count 5 2>&1 | ./go-junit-report > report.xml
+```
[travis-badge]: https://travis-ci.org/jstemmer/go-junit-report.svg
[travis-link]: https://travis-ci.org/jstemmer/go-junit-report
diff --git a/formatter/formatter.go b/formatter/formatter.go
index 51b4593..c55c67d 100644
--- a/formatter/formatter.go
+++ b/formatter/formatter.go
@@ -6,7 +6,6 @@ import (
"fmt"
"io"
"runtime"
- "strconv"
"strings"
"time"
@@ -39,9 +38,6 @@ type JUnitTestCase struct {
Time string `xml:"time,attr"`
SkipMessage *JUnitSkipMessage `xml:"skipped,omitempty"`
Failure *JUnitFailure `xml:"failure,omitempty"`
- // for benchmarks
- Bytes string `xml:"bytes,attr,omitempty"`
- Allocs string `xml:"allocs,attr,omitempty"`
}
// JUnitSkipMessage contains the reason why a testcase was skipped.
@@ -69,17 +65,8 @@ func JUnitReportXML(report *parser.Report, noXMLHeader bool, goVersion string, w
// convert Report to JUnit test suites
for _, pkg := range report.Packages {
- var tests int
- if len(pkg.Tests) >= 1 && len(pkg.Benchmarks) >= 1 {
- tests = len(pkg.Tests) + len(pkg.Benchmarks)
- } else if len(pkg.Benchmarks) >= 1 {
- tests = len(pkg.Benchmarks)
- } else {
- tests = len(pkg.Tests)
- }
-
ts := JUnitTestSuite{
- Tests: tests,
+ Tests: len(pkg.Tests) + len(pkg.Benchmarks),
Failures: 0,
Time: formatTime(pkg.Duration),
Name: pkg.Name,
@@ -128,7 +115,7 @@ func JUnitReportXML(report *parser.Report, noXMLHeader bool, goVersion string, w
}
// individual benchmarks
- for _, benchmark := range pkg.Benchmarks {
+ for _, benchmark := range mergeBenchmarks(pkg.Benchmarks) {
benchmarkCase := JUnitTestCase{
Classname: classname,
Name: benchmark.Name,
@@ -136,13 +123,6 @@ func JUnitReportXML(report *parser.Report, noXMLHeader bool, goVersion string, w
Failure: nil,
}
- if benchmark.Bytes != 0 {
- benchmarkCase.Bytes = strconv.Itoa(benchmark.Bytes)
- }
- if benchmark.Allocs != 0 {
- benchmarkCase.Allocs = strconv.Itoa(benchmark.Allocs)
- }
-
ts.TestCases = append(ts.TestCases, benchmarkCase)
}
@@ -168,6 +148,44 @@ func JUnitReportXML(report *parser.Report, noXMLHeader bool, goVersion string, w
return nil
}
+func mergeBenchmarks(benchmarks []*parser.Benchmark) []*parser.Benchmark {
+ // calculate the cumulative moving average CMA for each benchmark.
+ // CMA(n + 1) = val(n+1) + n*CMA/(n+1)
+ alloc := "Allocs"
+ bytes := "Bytes"
+ dur := "Duration"
+ n := 1
+ var merged []*parser.Benchmark
+ averages := make(map[string] /*bench name*/ map[string] /* type */ int)
+ for _, b := range benchmarks {
+ if avg, found := averages[b.Name]; found {
+ // calculate CMAs
+ averages[b.Name][alloc] = (b.Allocs + n*averages[b.Name][alloc]) / (n + 1)
+ averages[b.Name][bytes] = (b.Bytes + n*avg[bytes]) / (n + 1)
+ averages[b.Name][dur] = (int(b.Duration.Nanoseconds()) + n*avg[dur]) / (n + 1)
+
+ n++
+ continue
+ }
+ n = 1 // reset duplicate counter
+ merged = append(merged, &parser.Benchmark{Name: b.Name})
+ averages[b.Name] = make(map[string]int)
+ averages[b.Name][alloc] = b.Allocs
+ averages[b.Name][bytes] = b.Bytes
+ averages[b.Name][dur] = int(b.Duration.Nanoseconds())
+ }
+
+ // fill out benchmarks
+ for i := range merged {
+ avgVals := averages[merged[i].Name]
+ merged[i].Allocs = avgVals[alloc]
+ merged[i].Bytes = avgVals[bytes]
+ merged[i].Duration = time.Duration(avgVals[dur])
+ }
+
+ return merged
+}
+
func formatTime(d time.Duration) string {
return fmt.Sprintf("%.3f", d.Seconds())
}
diff --git a/go-junit-report_test.go b/go-junit-report_test.go
index 5b32ca9..e44a0ba 100644
--- a/go-junit-report_test.go
+++ b/go-junit-report_test.go
@@ -957,14 +957,12 @@ var testCases = []TestCase{
Duration: 52568 * time.Nanosecond,
Bytes: 24879,
Allocs: 494,
- Output: []string{},
},
{
Name: "BenchmarkIpsHistoryLookup",
Duration: 15208 * time.Nanosecond,
Bytes: 7369,
Allocs: 143,
- Output: []string{},
},
},
},
@@ -1016,14 +1014,12 @@ var testCases = []TestCase{
Duration: 2611 * time.Nanosecond,
Bytes: 1110,
Allocs: 16,
- Output: []string{},
},
{
Name: "BenchmarkNext",
Duration: 100 * time.Nanosecond,
Bytes: 100,
Allocs: 1,
- Output: []string{},
},
},
},
@@ -1042,19 +1038,63 @@ var testCases = []TestCase{
Benchmarks: []*parser.Benchmark{
{
Name: "BenchmarkNew",
- Duration: 352 * time.Nanosecond,
+ Duration: 350 * time.Nanosecond,
Bytes: 80,
Allocs: 3,
- Count: 5,
- Output: []string{},
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 357 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 354 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 358 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 345 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 100 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 105 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 102 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 102 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
},
{
Name: "BenchmarkFew",
Duration: 102 * time.Nanosecond,
Bytes: 20,
Allocs: 1,
- Count: 5,
- Output: []string{},
},
},
},
@@ -1076,53 +1116,92 @@ var testCases = []TestCase{
Duration: 0,
Time: 0,
Result: parser.PASS,
- Output: []string{},
},
{
Name: "TestRepeat",
Duration: 0,
Time: 0,
Result: parser.PASS,
- Output: []string{},
},
{
Name: "TestRepeat",
Duration: 0,
Time: 0,
Result: parser.PASS,
- Output: []string{},
},
{
Name: "TestRepeat",
Duration: 0,
Time: 0,
Result: parser.PASS,
- Output: []string{},
},
{
Name: "TestRepeat",
Duration: 0,
Time: 0,
Result: parser.PASS,
- Output: []string{},
},
},
Benchmarks: []*parser.Benchmark{
{
Name: "BenchmarkNew",
- Duration: 352 * time.Nanosecond,
+ Duration: 350 * time.Nanosecond,
Bytes: 80,
Allocs: 3,
- Count: 5,
- Output: []string{},
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 357 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 354 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 358 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkNew",
+ Duration: 345 * time.Nanosecond,
+ Bytes: 80,
+ Allocs: 3,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 100 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 105 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 102 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
+ },
+ {
+ Name: "BenchmarkFew",
+ Duration: 102 * time.Nanosecond,
+ Bytes: 20,
+ Allocs: 1,
},
{
Name: "BenchmarkFew",
Duration: 102 * time.Nanosecond,
Bytes: 20,
Allocs: 1,
- Count: 5,
- Output: []string{},
},
},
},
@@ -1142,17 +1221,14 @@ var testCases = []TestCase{
{
Name: "BenchmarkItsy",
Duration: 45 * time.Nanosecond,
- Output: []string{},
},
{
Name: "BenchmarkTeeny",
Duration: 2 * time.Nanosecond,
- Output: []string{},
},
{
Name: "BenchmarkWeeny",
Duration: 0 * time.Second,
- Output: []string{},
},
},
},
@@ -1257,12 +1333,6 @@ func TestParser(t *testing.T) {
if benchmark.Allocs != expBenchmark.Allocs {
t.Errorf("benchmark.Allocs == %d, want %d", benchmark.Allocs, expBenchmark.Allocs)
}
-
- benchmarkOutput := strings.Join(benchmark.Output, "\n")
- expBenchmarkOutput := strings.Join(expBenchmark.Output, "\n")
- if benchmarkOutput != expBenchmarkOutput {
- t.Errorf("Benchmark.Output (%s) ==\n%s\n, want\n%s", benchmark.Name, benchmarkOutput, expBenchmarkOutput)
- }
}
if pkg.CoveragePct != expPkg.CoveragePct {
diff --git a/parser/parser.go b/parser/parser.go
index 45ceb72..8b79f9a 100644
--- a/parser/parser.go
+++ b/parser/parser.go
@@ -55,9 +55,6 @@ type Benchmark struct {
Bytes int
// number of allocs/op
Allocs int
- // number of times this benchmark has been seen (for averaging).
- Count int
- Output []string
}
var (
@@ -65,7 +62,7 @@ var (
regexCoverage = regexp.MustCompile(`^coverage:\s+(\d+\.\d+)%\s+of\s+statements(?:\sin\s.+)?$`)
regexResult = regexp.MustCompile(`^(ok|FAIL)\s+([^ ]+)\s+(?:(\d+\.\d+)s|\(cached\)|(\[\w+ failed]))(?:\s+coverage:\s+(\d+\.\d+)%\sof\sstatements(?:\sin\s.+)?)?$`)
// regexBenchmark captures 3-5 groups: benchmark name, number of times ran, ns/op (with or without decimal), B/op (optional), and allocs/op (optional).
- regexBenchmark = regexp.MustCompile(`^(Benchmark\w+)-\d\s+(\d+)\s+((\d+|\d+\.\d+)\sns/op)(\s+\d+\sB/op)?(\s+\d+\sallocs/op)?`)
+ regexBenchmark = regexp.MustCompile(`^(Benchmark[^ ]+)-\d\s+(\d+)\s+(\d+|\d+\.\d+)\sns/op(?:\s+(\d+)\sB/op)?(?:\s+(\d+)\sallocs/op)?`)
regexOutput = regexp.MustCompile(`( )*\t(.*)`)
regexSummary = regexp.MustCompile(`^(PASS|FAIL|SKIP)$`)
)
@@ -128,60 +125,16 @@ func Parse(r io.Reader, pkgName string) (*Report, error) {
// clear the current build package, so output lines won't be added to that build
capturedPackage = ""
seenSummary = false
- } else if strings.HasPrefix(line, "Benchmark") {
- // parse benchmarking info
- matches := regexBenchmark.FindStringSubmatch(line)
- if len(matches) < 1 {
- continue
- }
- var name string
- var duration time.Duration
- var bytes int
- var allocs int
-
- for _, field := range matches[1:] {
- field = strings.TrimSpace(field)
- if strings.HasPrefix(field, "Benchmark") {
- name = field
- }
- if strings.HasSuffix(field, " ns/op") {
- durString := strings.TrimSuffix(field, " ns/op")
- duration = parseNanoseconds(durString)
- }
- if strings.HasSuffix(field, " B/op") {
- b, _ := strconv.Atoi(strings.TrimSuffix(field, " B/op"))
- bytes = b
- }
- if strings.HasSuffix(field, " allocs/op") {
- a, _ := strconv.Atoi(strings.TrimSuffix(field, " allocs/op"))
- allocs = a
- }
- }
-
- var duplicate bool
- // check if duplicate benchmark
- for _, bench := range benchmarks {
- if bench.Name == name {
- duplicate = true
- bench.Count++
- bench.Duration += duration
- bench.Bytes += bytes
- bench.Allocs += allocs
- }
- }
-
- if len(benchmarks) < 1 || duplicate == false {
- // the first time this benchmark has been seen
- benchmarks = append(benchmarks, &Benchmark{
- Name: name,
- Duration: duration,
- Bytes: bytes,
- Allocs: allocs,
- Count: 1,
- },
- )
- }
+ } else if matches := regexBenchmark.FindStringSubmatch(line); len(matches) == 6 {
+ bytes, _ := strconv.Atoi(matches[4])
+ allocs, _ := strconv.Atoi(matches[5])
+ benchmarks = append(benchmarks, &Benchmark{
+ Name: matches[1],
+ Duration: parseNanoseconds(matches[3]),
+ Bytes: bytes,
+ Allocs: allocs,
+ })
} else if strings.HasPrefix(line, "=== PAUSE ") {
continue
} else if strings.HasPrefix(line, "=== CONT ") {
@@ -212,15 +165,6 @@ func Parse(r io.Reader, pkgName string) (*Report, error) {
// all tests in this package are finished
- for _, bench := range benchmarks {
- if bench.Count > 1 {
- bench.Allocs = bench.Allocs / bench.Count
- bench.Bytes = bench.Bytes / bench.Count
- newDuration := bench.Duration / time.Duration(bench.Count)
- bench.Duration = newDuration
- }
- }
-
report.Packages = append(report.Packages, Package{
Name: matches[2],
Duration: parseSeconds(matches[3]),
diff --git a/parser/parser_test.go b/parser/parser_test.go
index 63f3ac4..0daa747 100644
--- a/parser/parser_test.go
+++ b/parser/parser_test.go
@@ -24,3 +24,24 @@ func TestParseSeconds(t *testing.T) {
}
}
}
+
+func TestParseNanoseconds(t *testing.T) {
+ tests := []struct {
+ in string
+ d time.Duration
+ }{
+ {"", 0},
+ {"0.1", 0 * time.Nanosecond},
+ {"0.9", 0 * time.Nanosecond},
+ {"4", 4 * time.Nanosecond},
+ {"5000", 5 * time.Microsecond},
+ {"2000003", 2*time.Millisecond + 3*time.Nanosecond},
+ }
+
+ for _, test := range tests {
+ d := parseNanoseconds(test.in)
+ if d != test.d {
+ t.Errorf("parseSeconds(%q) == %v, want %v\n", test.in, d, test.d)
+ }
+ }
+}
diff --git a/testdata/23-report.xml b/testdata/23-report.xml
index 31a04b3..3d6e11a 100644
--- a/testdata/23-report.xml
+++ b/testdata/23-report.xml
@@ -4,7 +4,7 @@
<properties>
<property name="go.version" value="1.0"></property>
</properties>
- <testcase classname="one" name="BenchmarkIpsHistoryInsert" time="0.000052568" bytes="24879" allocs="494"></testcase>
- <testcase classname="one" name="BenchmarkIpsHistoryLookup" time="0.000015208" bytes="7369" allocs="143"></testcase>
+ <testcase classname="one" name="BenchmarkIpsHistoryInsert" time="0.000052568"></testcase>
+ <testcase classname="one" name="BenchmarkIpsHistoryLookup" time="0.000015208"></testcase>
</testsuite>
</testsuites>
diff --git a/testdata/24-report.xml b/testdata/24-report.xml
index 199fa1e..b1b27a6 100644
--- a/testdata/24-report.xml
+++ b/testdata/24-report.xml
@@ -8,7 +8,7 @@
<testcase classname="baz" name="TestNew/no" time="0.000"></testcase>
<testcase classname="baz" name="TestNew/normal" time="0.000"></testcase>
<testcase classname="baz" name="TestWriteThis" time="0.000"></testcase>
- <testcase classname="baz" name="BenchmarkDeepMerge" time="0.000002611" bytes="1110" allocs="16"></testcase>
- <testcase classname="baz" name="BenchmarkNext" time="0.000000100" bytes="100" allocs="1"></testcase>
+ <testcase classname="baz" name="BenchmarkDeepMerge" time="0.000002611"></testcase>
+ <testcase classname="baz" name="BenchmarkNext" time="0.000000100"></testcase>
</testsuite>
</testsuites>
diff --git a/testdata/25-report.xml b/testdata/25-report.xml
index a8399ae..d5277f1 100644
--- a/testdata/25-report.xml
+++ b/testdata/25-report.xml
@@ -1,10 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
- <testsuite tests="2" failures="0" time="14.211" name="pkg/count">
+ <testsuite tests="10" failures="0" time="14.211" name="pkg/count">
<properties>
<property name="go.version" value="1.0"></property>
</properties>
- <testcase classname="count" name="BenchmarkNew" time="0.000000352" bytes="80" allocs="3"></testcase>
- <testcase classname="count" name="BenchmarkFew" time="0.000000102" bytes="20" allocs="1"></testcase>
+ <testcase classname="count" name="BenchmarkNew" time="0.000000352"></testcase>
+ <testcase classname="count" name="BenchmarkFew" time="0.000000102"></testcase>
</testsuite>
</testsuites>
diff --git a/testdata/26-report.xml b/testdata/26-report.xml
index 293d2ab..24330d9 100644
--- a/testdata/26-report.xml
+++ b/testdata/26-report.xml
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
- <testsuite tests="7" failures="0" time="14.211" name="multiple/repeating">
+ <testsuite tests="15" failures="0" time="14.211" name="multiple/repeating">
<properties>
<property name="go.version" value="1.0"></property>
</properties>
@@ -9,7 +9,7 @@
<testcase classname="repeating" name="TestRepeat" time="0.000"></testcase>
<testcase classname="repeating" name="TestRepeat" time="0.000"></testcase>
<testcase classname="repeating" name="TestRepeat" time="0.000"></testcase>
- <testcase classname="repeating" name="BenchmarkNew" time="0.000000352" bytes="80" allocs="3"></testcase>
- <testcase classname="repeating" name="BenchmarkFew" time="0.000000102" bytes="20" allocs="1"></testcase>
+ <testcase classname="repeating" name="BenchmarkNew" time="0.000000352"></testcase>
+ <testcase classname="repeating" name="BenchmarkFew" time="0.000000102"></testcase>
</testsuite>
</testsuites>