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

fix: JUnit processing for large files #5879

Merged
merged 15 commits into from
Oct 3, 2024
Merged
51 changes: 21 additions & 30 deletions cmd/testworkflow-toolkit/artifacts/junit_post_processor.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package artifacts

import (
"bufio"
"bytes"
"context"
"fmt"
"io"
Expand Down Expand Up @@ -59,14 +57,16 @@ func (p *JUnitPostProcessor) Add(path string) error {
if ok := isXMLFile(stat); !ok {
return nil
}
xmlData, err := io.ReadAll(file)
if err != nil {
// Read only the first 8KB of data
const BYTE_SIZE_8KB = 8 * 1024
xmlData := make([]byte, BYTE_SIZE_8KB)
n, err := file.Read(xmlData)
if err != nil && err != io.EOF {
return errors.Wrapf(err, "failed to read %s", path)
}
ok, err := isJUnitReport(xmlData)
if err != nil {
return errors.Wrapf(err, "failed to check if %s is a JUnit report", path)
}
// Trim the slice to the actual number of bytes read
xmlData = xmlData[:n]
ok := isJUnitReport(xmlData)
devcatalin marked this conversation as resolved.
Show resolved Hide resolved
if !ok {
return nil
}
Expand All @@ -79,10 +79,6 @@ func (p *JUnitPostProcessor) Add(path string) error {

// sendJUnitReport sends the JUnit report to the Agent gRPC API.
func (p *JUnitPostProcessor) sendJUnitReport(path string, report []byte) error {
// Apply path prefix correctly
if p.pathPrefix != "" {
path = filepath.Join(p.pathPrefix, path)
}
Comment on lines -82 to -85
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed because in the above Add we already to the same:

uploadPath := path
if p.pathPrefix != "" {
    uploadPath = filepath.Join(p.pathPrefix, uploadPath)
}

ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
defer cancel()
_, err := p.client.Execute(ctx, testworkflow.CmdTestWorkflowExecutionAddReport, &testworkflow.ExecutionsAddReportRequest{
Expand All @@ -104,27 +100,22 @@ func isXMLFile(stat fs.FileInfo) bool {
return strings.HasSuffix(stat.Name(), ".xml")
}

// isJUnitReport checks if the file starts with a JUnit XML tag
func isJUnitReport(xmlData []byte) (bool, error) {
scanner := bufio.NewScanner(bytes.NewReader(xmlData))
// Read only the first few lines of the file
for scanner.Scan() {
line := scanner.Text()
line = strings.TrimSpace(line) // Remove leading and trailing whitespace

// Skip comments and declarations
if strings.HasPrefix(line, "<!--") || strings.HasPrefix(line, "<?xml") {
continue
}
if strings.Contains(line, "<testsuite") || strings.Contains(line, "<testsuites") {
return true, nil
}
if strings.Contains(line, "<") { // Stop if any non-JUnit tag is found
break
// isJUnitReport checks if the XML data is a JUnit report.
func isJUnitReport(xmlData []byte) bool {
tags := []string{
"<testsuite",
"<testsuites",
}

content := string(xmlData)

for _, tag := range tags {
if strings.Contains(content, tag) {
return true
}
}

return false, scanner.Err()
return false
}

func (p *JUnitPostProcessor) End() error {
Expand Down
48 changes: 44 additions & 4 deletions cmd/testworkflow-toolkit/artifacts/junit_post_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package artifacts
import (
"io"
"io/fs"
"path/filepath"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -71,6 +72,33 @@ func TestJUnitPostProcessor_Add(t *testing.T) {

}

func TestJUnitPostProcessor_Add_WithPathPrefix(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

mockFS := filesystem.NewMockFileSystem(mockCtrl)
mockClient := executor.NewMockExecutor(mockCtrl)

pathPrefix := "prefixed/junit/report/"
filePath := "junit.xml"
junitContent := []byte(testdata.BasicJUnit)

mockFS.EXPECT().OpenFileRO(gomock.Any()).Return(filesystem.NewMockFile("junit.xml", junitContent), nil)

pp := NewJUnitPostProcessor(mockFS, mockClient, "/test_root", pathPrefix)

expectedPayload := testworkflow.ExecutionsAddReportRequest{
Filepath: filepath.Join(pathPrefix, filePath),
Report: junitContent,
}

mockClient.EXPECT().Execute(gomock.Any(), testworkflow.CmdTestWorkflowExecutionAddReport, gomock.Eq(&expectedPayload)).Return(nil, nil)

err := pp.Add(filePath)

assert.NoError(t, err)
}

func TestIsXMLFile(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -146,6 +174,21 @@ func TestIsJUnitReport(t *testing.T) {
file: filesystem.NewMockFile("invalid.xml", []byte(testdata.InvalidJUnit)),
want: false,
},
{
name: "one-line junit",
file: filesystem.NewMockFile("oneline.xml", []byte(testdata.OneLineJUnit)),
want: true,
},
{
name: "testsuites only junit",
file: filesystem.NewMockFile("testsuites.xml", []byte(testdata.TestsuitesOnlyJUnit)),
want: true,
},
{
name: "testsuite only junit",
file: filesystem.NewMockFile("testsuite.xml", []byte(testdata.TestsuiteOnlyJUnit)),
want: true,
},
}

for _, tc := range tests {
Expand All @@ -154,10 +197,7 @@ func TestIsJUnitReport(t *testing.T) {
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
ok, err := isJUnitReport(data)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
ok := isJUnitReport(data)
assert.Equal(t, tc.want, ok)
})
}
Expand Down
1 change: 0 additions & 1 deletion cmd/testworkflow-toolkit/artifacts/walker.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,6 @@ func (w *walker) walk(fsys fs.FS, path string, walker WalkerFn) error {
if sanitizedPath == "" {
sanitizedPath = "."
}

return fs.WalkDir(fsys, sanitizedPath, func(filePath string, d fs.DirEntry, err error) error {
resolvedPath := "/" + filepath.ToSlash(filePath)
if !w.matches(resolvedPath) {
Expand Down
13 changes: 13 additions & 0 deletions cmd/testworkflow-toolkit/common/testdata/junit.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,16 @@ var InvalidJUnit = `<?xml version="1.0" encoding="UTF-8"?>
<foo>
<bar>
</foo>`

var OneLineJUnit = `<?xml version="1.0" encoding="UTF-8"?><testsuites><testsuite name="TestSuite" tests="2" errors="0" failures="1" skipped="0"><testcase name="Test1" classname="TestClass"><failure message="Test failed">Failure details</failure></testcase><testcase name="Test2" classname="TestClass"/></testsuite></testsuites>`

var TestsuitesOnlyJUnit = `<testsuites id="" name="" tests="2" failures="0" skipped="0" errors="0" time="14.511833">
<testsuite name="smoke.spec.js" timestamp="2024-10-01T12:48:47.332Z" hostname="chromium" tests="1" failures="0" skipped="0" time="6.259" errors="0">
<testcase name="Smoke 1 - has title" classname="smoke.spec.js" time="6.259"></testcase>
</testsuite>
<testsuite name="smoke2.spec.js" timestamp="2024-10-01T12:48:47.332Z" hostname="chromium" tests="1" failures="0" skipped="0" time="6.657" errors="0">
<testcase name="Smoke 2 - has title" classname="smoke2.spec.js" time="6.657"></testcase>
</testsuite>
</testsuites>`

var TestsuiteOnlyJUnit = `<testsuite name="smoke.spec.js" timestamp="2024-10-01T12:48:47.332Z" hostname="chromium" tests="1" failures="0" skipped="0" time="6.259" errors="0"><testcase name="Smoke 1 - has title" classname="smoke.spec.js" time="6.259"></testcase></testsuite>`
Loading