From 492845cd243b7ac637534a762cf6b4d0dcedc938 Mon Sep 17 00:00:00 2001 From: Taylor Date: Tue, 24 Mar 2026 12:11:43 -0700 Subject: [PATCH 1/2] fix: close rally report after parsing --- internal/benchrunner/runners/rally/runner.go | 20 +++++-- .../benchrunner/runners/rally/runner_test.go | 54 +++++++++++++++++++ 2 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 internal/benchrunner/runners/rally/runner_test.go diff --git a/internal/benchrunner/runners/rally/runner.go b/internal/benchrunner/runners/rally/runner.go index 66550389ae..5cb02ec851 100644 --- a/internal/benchrunner/runners/rally/runner.go +++ b/internal/benchrunner/runners/rally/runner.go @@ -925,16 +925,26 @@ func (r *runner) runRally(ctx context.Context) ([]rallyStat, error) { return nil, fmt.Errorf("could not open esrally report in path: %s: %w", r.svcInfo.Logs.Folder.Local, err) } - reader := csv.NewReader(reportCSV) + return parseRallyReport(reportCSV, r.svcInfo.Logs.Folder.Local, errOutput.String()) +} + +func parseRallyReport(report io.ReadCloser, logsPath string, stderr string) (_ []rallyStat, err error) { + defer func() { + closeErr := report.Close() + if err == nil && closeErr != nil { + err = fmt.Errorf("could not close esrally report in path: %s: %w", logsPath, closeErr) + } + }() + reader := csv.NewReader(report) stats := make([]rallyStat, 0) for { - record, err := reader.Read() - if err == io.EOF { + record, readErr := reader.Read() + if readErr == io.EOF { break } - if err != nil { - return nil, fmt.Errorf("could not read esrally report in path: %s (stderr=%q): %w", r.svcInfo.Logs.Folder.Local, errOutput.String(), err) + if readErr != nil { + return nil, fmt.Errorf("could not read esrally report in path: %s (stderr=%q): %w", logsPath, stderr, readErr) } stats = append(stats, rallyStat{Metric: record[0], Task: record[1], Value: record[2], Unit: record[3]}) diff --git a/internal/benchrunner/runners/rally/runner_test.go b/internal/benchrunner/runners/rally/runner_test.go new file mode 100644 index 0000000000..4116827c49 --- /dev/null +++ b/internal/benchrunner/runners/rally/runner_test.go @@ -0,0 +1,54 @@ +package rally + +import ( + "errors" + "io" + "strings" + "testing" +) + +type trackingReadCloser struct { + io.Reader + closed bool + closeErr error +} + +func (r *trackingReadCloser) Close() error { + r.closed = true + return r.closeErr +} + +func TestParseRallyReportClosesReport(t *testing.T) { + report := &trackingReadCloser{ + Reader: strings.NewReader("metric,task,value,unit\n"), + } + + stats, err := parseRallyReport(report, "/tmp/rally", "") + if err != nil { + t.Fatalf("parseRallyReport returned error: %v", err) + } + if !report.closed { + t.Fatal("parseRallyReport did not close the report reader") + } + if len(stats) != 1 { + t.Fatalf("expected 1 stat, got %d", len(stats)) + } +} + +func TestParseRallyReportReturnsCloseError(t *testing.T) { + report := &trackingReadCloser{ + Reader: strings.NewReader("metric,task,value,unit\n"), + closeErr: errors.New("close failed"), + } + + _, err := parseRallyReport(report, "/tmp/rally", "") + if err == nil { + t.Fatal("parseRallyReport returned nil error") + } + if !report.closed { + t.Fatal("parseRallyReport did not close the report reader") + } + if !strings.Contains(err.Error(), "close failed") { + t.Fatalf("expected close error, got %v", err) + } +} From f6a9a0814b8f13e6fb9abdc4929e5944d23ed747 Mon Sep 17 00:00:00 2001 From: Taylor Date: Fri, 27 Mar 2026 13:57:28 -0700 Subject: [PATCH 2/2] refactor: keep rally report closing at call site --- internal/benchrunner/runners/rally/runner.go | 10 +--- .../benchrunner/runners/rally/runner_test.go | 48 ++++--------------- 2 files changed, 11 insertions(+), 47 deletions(-) diff --git a/internal/benchrunner/runners/rally/runner.go b/internal/benchrunner/runners/rally/runner.go index 5cb02ec851..4ea059acca 100644 --- a/internal/benchrunner/runners/rally/runner.go +++ b/internal/benchrunner/runners/rally/runner.go @@ -924,18 +924,12 @@ func (r *runner) runRally(ctx context.Context) ([]rallyStat, error) { if err != nil { return nil, fmt.Errorf("could not open esrally report in path: %s: %w", r.svcInfo.Logs.Folder.Local, err) } + defer reportCSV.Close() return parseRallyReport(reportCSV, r.svcInfo.Logs.Folder.Local, errOutput.String()) } -func parseRallyReport(report io.ReadCloser, logsPath string, stderr string) (_ []rallyStat, err error) { - defer func() { - closeErr := report.Close() - if err == nil && closeErr != nil { - err = fmt.Errorf("could not close esrally report in path: %s: %w", logsPath, closeErr) - } - }() - +func parseRallyReport(report io.Reader, logsPath string, stderr string) ([]rallyStat, error) { reader := csv.NewReader(report) stats := make([]rallyStat, 0) for { diff --git a/internal/benchrunner/runners/rally/runner_test.go b/internal/benchrunner/runners/rally/runner_test.go index 4116827c49..7783c69885 100644 --- a/internal/benchrunner/runners/rally/runner_test.go +++ b/internal/benchrunner/runners/rally/runner_test.go @@ -1,54 +1,24 @@ package rally import ( - "errors" - "io" "strings" "testing" ) -type trackingReadCloser struct { - io.Reader - closed bool - closeErr error -} - -func (r *trackingReadCloser) Close() error { - r.closed = true - return r.closeErr -} - -func TestParseRallyReportClosesReport(t *testing.T) { - report := &trackingReadCloser{ - Reader: strings.NewReader("metric,task,value,unit\n"), - } - - stats, err := parseRallyReport(report, "/tmp/rally", "") +func TestParseRallyReportReturnsStats(t *testing.T) { + stats, err := parseRallyReport(strings.NewReader("metric,task,value,unit\n"), "/tmp/rally", "") if err != nil { t.Fatalf("parseRallyReport returned error: %v", err) } - if !report.closed { - t.Fatal("parseRallyReport did not close the report reader") - } if len(stats) != 1 { t.Fatalf("expected 1 stat, got %d", len(stats)) } -} - -func TestParseRallyReportReturnsCloseError(t *testing.T) { - report := &trackingReadCloser{ - Reader: strings.NewReader("metric,task,value,unit\n"), - closeErr: errors.New("close failed"), - } - - _, err := parseRallyReport(report, "/tmp/rally", "") - if err == nil { - t.Fatal("parseRallyReport returned nil error") - } - if !report.closed { - t.Fatal("parseRallyReport did not close the report reader") - } - if !strings.Contains(err.Error(), "close failed") { - t.Fatalf("expected close error, got %v", err) + if stats[0] != (rallyStat{ + Metric: "metric", + Task: "task", + Value: "value", + Unit: "unit", + }) { + t.Fatalf("unexpected stat: %#v", stats[0]) } }