From fbce690f6e3989a73561de0fd9fece0b7518cd4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 11:12:35 +0200 Subject: [PATCH 1/7] green: ingestJunitDir error includes filename when XML parsing fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace junit.IngestDir() with our own filepath.Walk that calls junit.IngestFile() per file and wraps errors with the filename. This makes XML parsing errors actionable — users can now see which file caused the failure. Add test fixture with ISO-8859-15 encoding declaration and three unit tests for ingestJunitDir: filename in error, no-tests-found, and valid JUnit parsing. Refs #894 --- cmd/kosli/attestJunit.go | 20 ++++++++++++++--- cmd/kosli/attestJunit_test.go | 23 ++++++++++++++++++++ cmd/kosli/testdata_junit_iso8859/results.xml | 7 ++++++ 3 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 cmd/kosli/testdata_junit_iso8859/results.xml diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index 8b2d8119e..8b8813b50 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -214,16 +214,30 @@ type JUnitResults struct { func ingestJunitDir(testResultsDir string) ([]*JUnitResults, error) { results := []*JUnitResults{} - suites, err := junit.IngestDir(testResultsDir) + + var allSuites []junit.Suite + err := filepath.Walk(testResultsDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".xml") { + suites, err := junit.IngestFile(path) + if err != nil { + return fmt.Errorf("failed to parse JUnit XML file %s: %w", path, err) + } + allSuites = append(allSuites, suites...) + } + return nil + }) if err != nil { return results, err } - if len(suites) == 0 { + if len(allSuites) == 0 { return results, fmt.Errorf("no tests found in %s directory", testResultsDir) } - for _, suite := range suites { + for _, suite := range allSuites { var timestamp float64 timestamp, err := parseTimestamp(suite.Properties["timestamp"]) if err != nil { diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index b9bcdd5d1..e8151e12a 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -138,3 +140,24 @@ func (suite *AttestJunitCommandTestSuite) TestAttestJunitCmd() { func TestAttestJunitCommandTestSuite(t *testing.T) { suite.Run(t, new(AttestJunitCommandTestSuite)) } + +func TestIngestJunitDir(t *testing.T) { + t.Run("error includes filename when XML parsing fails", func(t *testing.T) { + _, err := ingestJunitDir("testdata_junit_iso8859") + require.Error(t, err) + assert.Contains(t, err.Error(), "results.xml") + }) + + t.Run("returns no tests found for empty directory", func(t *testing.T) { + dir := t.TempDir() + _, err := ingestJunitDir(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), "no tests found") + }) + + t.Run("parses valid JUnit XML correctly", func(t *testing.T) { + results, err := ingestJunitDir("testdata/junit") + require.NoError(t, err) + assert.NotEmpty(t, results) + }) +} diff --git a/cmd/kosli/testdata_junit_iso8859/results.xml b/cmd/kosli/testdata_junit_iso8859/results.xml new file mode 100644 index 000000000..b7f1983dd --- /dev/null +++ b/cmd/kosli/testdata_junit_iso8859/results.xml @@ -0,0 +1,7 @@ + + + + + + + From a026a97eb7037644cd97dbf1f3ebcab43204f9cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 11:13:18 +0200 Subject: [PATCH 2/7] green: getJunitFilenames error includes filename when XML parsing fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same wrapping treatment as ingestJunitDir — wrap junit.IngestFile() errors with the filename so users can identify which file caused the failure. Refs #894 --- cmd/kosli/attestJunit.go | 2 +- cmd/kosli/attestJunit_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index 8b8813b50..3e3798e86 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -273,7 +273,7 @@ func getJunitFilenames(directory string) ([]string, error) { if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".xml") { suites, err := junit.IngestFile(path) if err != nil { - return err + return fmt.Errorf("failed to parse JUnit XML file %s: %w", path, err) } if len(suites) > 0 { filenames = append(filenames, path) diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index e8151e12a..83028d248 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -161,3 +161,11 @@ func TestIngestJunitDir(t *testing.T) { assert.NotEmpty(t, results) }) } + +func TestGetJunitFilenames(t *testing.T) { + t.Run("error includes filename when XML parsing fails", func(t *testing.T) { + _, err := getJunitFilenames("testdata_junit_iso8859") + require.Error(t, err) + assert.Contains(t, err.Error(), "results.xml") + }) +} From 0b7733cccb2dabb34667cce0d83df734f721c30c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 11:15:03 +0200 Subject: [PATCH 3/7] green: consolidate JUnit dir walk into single pass ingestJunitDir now returns both parsed results and the list of JUnit filenames (for upload), eliminating the separate getJunitFilenames() function and its redundant directory walk. Refs #894 --- cmd/kosli/attestJunit.go | 53 ++++++++--------------------------- cmd/kosli/attestJunit_test.go | 18 ++++++------ 2 files changed, 22 insertions(+), 49 deletions(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index 3e3798e86..1929d19f7 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -161,17 +161,13 @@ func (o *attestJunitOptions) run(args []string) error { return err } - o.payload.JUnitResults, err = ingestJunitDir(o.testResultsDir) + var junitFilenames []string + o.payload.JUnitResults, junitFilenames, err = ingestJunitDir(o.testResultsDir) if err != nil { return err } if o.uploadResultsDir { - // prepare the files to upload as attachments. We are only interested in the actual Junit XMl files - junitFilenames, err := getJunitFilenames(o.testResultsDir) - if err != nil { - return err - } o.attachments = append(o.attachments, junitFilenames...) } @@ -212,8 +208,9 @@ type JUnitResults struct { Timestamp float64 `json:"timestamp,omitempty"` } -func ingestJunitDir(testResultsDir string) ([]*JUnitResults, error) { +func ingestJunitDir(testResultsDir string) ([]*JUnitResults, []string, error) { results := []*JUnitResults{} + var junitFilenames []string var allSuites []junit.Suite err := filepath.Walk(testResultsDir, func(path string, info os.FileInfo, err error) error { @@ -225,23 +222,26 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, error) { if err != nil { return fmt.Errorf("failed to parse JUnit XML file %s: %w", path, err) } - allSuites = append(allSuites, suites...) + if len(suites) > 0 { + allSuites = append(allSuites, suites...) + junitFilenames = append(junitFilenames, path) + } } return nil }) if err != nil { - return results, err + return results, nil, err } if len(allSuites) == 0 { - return results, fmt.Errorf("no tests found in %s directory", testResultsDir) + return results, nil, fmt.Errorf("no tests found in %s directory", testResultsDir) } for _, suite := range allSuites { var timestamp float64 timestamp, err := parseTimestamp(suite.Properties["timestamp"]) if err != nil { - return results, err + return results, nil, err } // The values in suite.Totals are based on the results of the tests in the suite and not in the header of the suite. @@ -258,36 +258,7 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, error) { results = append(results, suiteResult) } - return results, nil -} - -func getJunitFilenames(directory string) ([]string, error) { - var filenames []string - - err := filepath.Walk(directory, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // Add all regular files that end with ".xml" - if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".xml") { - suites, err := junit.IngestFile(path) - if err != nil { - return fmt.Errorf("failed to parse JUnit XML file %s: %w", path, err) - } - if len(suites) > 0 { - filenames = append(filenames, path) - } - } - - return nil - }) - - if err != nil { - return filenames, err - } - - return filenames, nil + return results, junitFilenames, nil } func parseTimestamp(timestampStr string) (float64, error) { diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index 83028d248..555e70f4b 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -143,29 +143,31 @@ func TestAttestJunitCommandTestSuite(t *testing.T) { func TestIngestJunitDir(t *testing.T) { t.Run("error includes filename when XML parsing fails", func(t *testing.T) { - _, err := ingestJunitDir("testdata_junit_iso8859") + _, _, err := ingestJunitDir("testdata_junit_iso8859") require.Error(t, err) assert.Contains(t, err.Error(), "results.xml") }) t.Run("returns no tests found for empty directory", func(t *testing.T) { dir := t.TempDir() - _, err := ingestJunitDir(dir) + _, _, err := ingestJunitDir(dir) require.Error(t, err) assert.Contains(t, err.Error(), "no tests found") }) t.Run("parses valid JUnit XML correctly", func(t *testing.T) { - results, err := ingestJunitDir("testdata/junit") + results, _, err := ingestJunitDir("testdata/junit") require.NoError(t, err) assert.NotEmpty(t, results) }) } -func TestGetJunitFilenames(t *testing.T) { - t.Run("error includes filename when XML parsing fails", func(t *testing.T) { - _, err := getJunitFilenames("testdata_junit_iso8859") - require.Error(t, err) - assert.Contains(t, err.Error(), "results.xml") +func TestIngestJunitDir_ReturnsFilenames(t *testing.T) { + t.Run("returns filenames of parsed JUnit XML files", func(t *testing.T) { + results, filenames, err := ingestJunitDir("testdata/junit") + require.NoError(t, err) + assert.NotEmpty(t, results) + assert.NotEmpty(t, filenames) + assert.Contains(t, filenames[0], ".xml") }) } From 473213dc24118dae51a29c98118a8107648fdfee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 12:12:03 +0200 Subject: [PATCH 4/7] refactor: address review feedback - Return nil instead of empty slice on Walk error for consistency - Merge TestIngestJunitDir_ReturnsFilenames into TestIngestJunitDir Refs #894 --- cmd/kosli/attestJunit.go | 2 +- cmd/kosli/attestJunit_test.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index 1929d19f7..925fb8402 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -230,7 +230,7 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, []string, error) { return nil }) if err != nil { - return results, nil, err + return nil, nil, err } if len(allSuites) == 0 { diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index 555e70f4b..0c635861e 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -160,9 +160,7 @@ func TestIngestJunitDir(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, results) }) -} -func TestIngestJunitDir_ReturnsFilenames(t *testing.T) { t.Run("returns filenames of parsed JUnit XML files", func(t *testing.T) { results, filenames, err := ingestJunitDir("testdata/junit") require.NoError(t, err) From df7ec6b197826b63b2f9b84fffde5918f150e7e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 12:27:35 +0200 Subject: [PATCH 5/7] green: user-friendly error message for unsupported XML encoding Replace the cryptic Go stdlib error (Decoder.CharsetReader is nil) with an actionable message that tells the user which file failed, that only UTF-8 is supported, and to use --results-dir if the file is not a JUnit results file. Refs #894 --- cmd/kosli/attestJunit.go | 6 +++++- cmd/kosli/attestJunit_test.go | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index 925fb8402..e49915328 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -220,7 +220,11 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, []string, error) { if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".xml") { suites, err := junit.IngestFile(path) if err != nil { - return fmt.Errorf("failed to parse JUnit XML file %s: %w", path, err) + if strings.Contains(err.Error(), "Decoder.CharsetReader is nil") { + return fmt.Errorf("failed to parse XML file %s: only UTF-8 encoding is supported. "+ + "If this is not a JUnit results file, use --results-dir to specify the directory containing only JUnit XML files", path) + } + return fmt.Errorf("failed to parse XML file %s: %w", path, err) } if len(suites) > 0 { allSuites = append(allSuites, suites...) diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index 0c635861e..a09dcd902 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -142,10 +142,12 @@ func TestAttestJunitCommandTestSuite(t *testing.T) { } func TestIngestJunitDir(t *testing.T) { - t.Run("error includes filename when XML parsing fails", func(t *testing.T) { + t.Run("error includes filename and user-friendly message for unsupported encoding", func(t *testing.T) { _, _, err := ingestJunitDir("testdata_junit_iso8859") require.Error(t, err) assert.Contains(t, err.Error(), "results.xml") + assert.Contains(t, err.Error(), "only UTF-8 encoding is supported") + assert.Contains(t, err.Error(), "--results-dir") }) t.Run("returns no tests found for empty directory", func(t *testing.T) { From 8b524cd988bf41fb33457fee34a2aaefc773deb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 12:35:21 +0200 Subject: [PATCH 6/7] refactor: address second round of review feedback - Use filepath.WalkDir instead of filepath.Walk (avoids os.Lstat per entry) - Return nil, nil on all error paths for consistency - Assert exact filename count in test Refs #894 --- cmd/kosli/attestJunit.go | 9 +++++---- cmd/kosli/attestJunit_test.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index e49915328..c1f794d03 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/url" + "io/fs" "os" "path/filepath" "strings" @@ -213,11 +214,11 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, []string, error) { var junitFilenames []string var allSuites []junit.Suite - err := filepath.Walk(testResultsDir, func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(testResultsDir, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".xml") { + if d.Type().IsRegular() && strings.HasSuffix(d.Name(), ".xml") { suites, err := junit.IngestFile(path) if err != nil { if strings.Contains(err.Error(), "Decoder.CharsetReader is nil") { @@ -238,14 +239,14 @@ func ingestJunitDir(testResultsDir string) ([]*JUnitResults, []string, error) { } if len(allSuites) == 0 { - return results, nil, fmt.Errorf("no tests found in %s directory", testResultsDir) + return nil, nil, fmt.Errorf("no tests found in %s directory", testResultsDir) } for _, suite := range allSuites { var timestamp float64 timestamp, err := parseTimestamp(suite.Properties["timestamp"]) if err != nil { - return results, nil, err + return nil, nil, err } // The values in suite.Totals are based on the results of the tests in the suite and not in the header of the suite. diff --git a/cmd/kosli/attestJunit_test.go b/cmd/kosli/attestJunit_test.go index a09dcd902..be75c8235 100644 --- a/cmd/kosli/attestJunit_test.go +++ b/cmd/kosli/attestJunit_test.go @@ -167,7 +167,7 @@ func TestIngestJunitDir(t *testing.T) { results, filenames, err := ingestJunitDir("testdata/junit") require.NoError(t, err) assert.NotEmpty(t, results) - assert.NotEmpty(t, filenames) + assert.Len(t, filenames, 1) assert.Contains(t, filenames[0], ".xml") }) } From bf837f0abe156ac30f45986160f2743c2615c079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 18 May 2026 12:40:54 +0200 Subject: [PATCH 7/7] fix: alphabetical import ordering Refs #894 --- cmd/kosli/attestJunit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/kosli/attestJunit.go b/cmd/kosli/attestJunit.go index c1f794d03..57577ade5 100644 --- a/cmd/kosli/attestJunit.go +++ b/cmd/kosli/attestJunit.go @@ -3,9 +3,9 @@ package main import ( "fmt" "io" + "io/fs" "net/http" "net/url" - "io/fs" "os" "path/filepath" "strings"