Skip to content
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
21 changes: 21 additions & 0 deletions pkg/jobrunaggregator/jobrunaggregatorapi/types_row_test_summary.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package jobrunaggregatorapi

import (
"cloud.google.com/go/civil"
)

// TestSummaryByPeriodRow represents aggregated test results for a specific suite and release over a time period.
// This data structure corresponds to the suite_summary_by_period.sql query results.
type TestSummaryByPeriodRow struct {
Release string `bigquery:"release"`
TestName string `bigquery:"test_name"`
TotalTestCount int64 `bigquery:"total_test_count"`
TotalFailureCount int64 `bigquery:"total_failure_count"`
TotalFlakeCount int64 `bigquery:"total_flake_count"`
FailureRate float64 `bigquery:"failure_rate"`
AvgDurationMs float64 `bigquery:"avg_duration_ms"`
PeriodStart civil.Date `bigquery:"period_start"`
PeriodEnd civil.Date `bigquery:"period_end"`
DaysWithData int64 `bigquery:"days_with_data"`
FirstSampleDate civil.Date `bigquery:"first_sample_date"` // First date this test_name was seen within the sample period across all releases for this suite
}
98 changes: 98 additions & 0 deletions pkg/jobrunaggregator/jobrunaggregatorlib/ci_data_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ type CIDataClient interface {

// ListReleases lists all releases from the new release table
ListReleases(ctx context.Context) ([]jobrunaggregatorapi.ReleaseRow, error)

// ListTestSummaryByPeriod retrieves aggregated test results for a specific suite and release over a time period.
// Parameters:
// - suiteName: The test suite to query (e.g., 'conformance')
// - releaseName: The release version (e.g., '4.15')
// - daysBack: Number of days to look back from current date
// - minTestCount: Minimum number of test executions required to include a test in results
ListTestSummaryByPeriod(ctx context.Context, suiteName, releaseName string, daysBack, minTestCount int) ([]jobrunaggregatorapi.TestSummaryByPeriodRow, error)
}

type ciDataClient struct {
Expand Down Expand Up @@ -1095,3 +1103,93 @@ func (c *ciDataClient) ListAllKnownAlerts(ctx context.Context) ([]*jobrunaggrega

return allKnownAlerts, nil
}

func (c *ciDataClient) ListTestSummaryByPeriod(ctx context.Context, suiteName, releaseName string, daysBack, minTestCount int) ([]jobrunaggregatorapi.TestSummaryByPeriodRow, error) {
// Query to summarize test results for a specific suite and release over a time period
// Groups by release and test_name only (no infrastructure dimensions)
// Calculates total test_count, failure_count, flake_count, failure_rate, and avg_duration_ms
// Filters results to only include tests with sufficient test runs
// Also includes first_seen_date: the earliest date each test_name appeared across all releases for this suite
queryString := c.dataCoordinates.SubstituteDataSetLocation(`
WITH earliest_test_dates AS (
-- Pre-compute the earliest date each test was seen across all releases for this suite
-- Look back max(100, @days_back) days to ensure we capture historical context
SELECT
test_name,
MIN(date) AS first_sample_date
FROM
DATA_SET_LOCATION.TestsSummaryByDate
WHERE
suite = @suite_name
AND date >= DATE_SUB(CURRENT_DATE(), INTERVAL GREATEST(100, @days_back) DAY)
AND date <= CURRENT_DATE()
GROUP BY
test_name
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
SELECT
main.release,
main.test_name,
SUM(main.test_count) AS total_test_count,
SUM(main.failure_count) AS total_failure_count,
SUM(main.flake_count) AS total_flake_count,
SAFE_DIVIDE(SUM(main.failure_count), SUM(main.test_count)) AS failure_rate,
AVG(main.avg_duration_ms) AS avg_duration_ms,
MIN(main.date) AS period_start,
MAX(main.date) AS period_end,
COUNT(DISTINCT main.date) AS days_with_data,
earliest.first_sample_date
FROM
DATA_SET_LOCATION.TestsSummaryByDate AS main
LEFT JOIN
earliest_test_dates AS earliest
ON
main.test_name = earliest.test_name
WHERE
main.suite = @suite_name
AND main.release = @release_name
AND main.date >= DATE_SUB(CURRENT_DATE(), INTERVAL @days_back DAY)
AND main.date <= CURRENT_DATE()
GROUP BY
main.release,
main.test_name,
earliest.first_sample_date
HAVING
SUM(main.test_count) > @min_test_count
ORDER BY
main.release,
total_failure_count DESC,
main.test_name
`)

query := c.client.Query(queryString)
query.Labels = map[string]string{
bigQueryLabelKeyApp: bigQueryLabelValueApp,
bigQueryLabelKeyQuery: bigQueryLabelValueTestSummaryByPeriod,
}
query.QueryConfig.Parameters = []bigquery.QueryParameter{
{Name: "suite_name", Value: suiteName},
{Name: "release_name", Value: releaseName},
{Name: "days_back", Value: daysBack},
{Name: "min_test_count", Value: minTestCount},
}

rows, err := query.Read(ctx)
if err != nil {
return nil, fmt.Errorf("failed to query generic test summary by period with %q: %w", queryString, err)
}

results := []jobrunaggregatorapi.TestSummaryByPeriodRow{}
for {
row := &jobrunaggregatorapi.TestSummaryByPeriodRow{}
err = rows.Next(row)
if err == iterator.Done {
break
}
if err != nil {
return nil, err
}
results = append(results, *row)
}

return results, nil
}
Comment on lines +1107 to +1195

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n pkg/jobrunaggregator/jobrunaggregatorlib/ci_data_client.go | sed -n '1107,1195p'

Repository: openshift/ci-tools

Length of output: 3664


🏁 Script executed:

# Search for test cases or usage of ListTestSummaryByPeriod
rg "ListTestSummaryByPeriod" -A 5 -B 5

Repository: openshift/ci-tools

Length of output: 8755


🏁 Script executed:

# Look for any documentation about minTestCount parameter
rg "minTestCount|min_test_count" -B 3 -A 3 -i

Repository: openshift/ci-tools

Length of output: 8276


🏁 Script executed:

# Check if there are similar patterns with comparison operators in other queries
rg "HAVING.*COUNT|HAVING.*SUM" -A 2 -B 2

Repository: openshift/ci-tools

Length of output: 44


Change HAVING clause to use >= instead of >.

Line 1157: The HAVING clause uses SUM(main.test_count) > @min_test_count (strict greater than), which excludes tests with exactly minTestCount runs. The parameter documentation states "Minimum number of test executions required to include a test in results" and the parameter name minTestCount both suggest inclusive comparison. Change to >= to match the documented intent.

🤖 Prompt for AI Agents
In @pkg/jobrunaggregator/jobrunaggregatorlib/ci_data_client.go around lines 1107
- 1195, The HAVING clause in ListTestSummaryByPeriod (function
ciDataClient.ListTestSummaryByPeriod) currently uses a strict greater-than check
SUM(main.test_count) > @min_test_count which excludes tests with exactly
minTestCount runs; change the HAVING condition to use >= (SUM(main.test_count)
>= @min_test_count) so the comparison is inclusive of minTestCount to match the
parameter intent and documentation.

15 changes: 15 additions & 0 deletions pkg/jobrunaggregator/jobrunaggregatorlib/ci_data_client_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,16 @@ func (c *retryingCIDataClient) ListAllKnownAlerts(ctx context.Context) ([]*jobru
return ret, err
}

func (c *retryingCIDataClient) ListTestSummaryByPeriod(ctx context.Context, suiteName, releaseName string, daysBack, minTestCount int) ([]jobrunaggregatorapi.TestSummaryByPeriodRow, error) {
var ret []jobrunaggregatorapi.TestSummaryByPeriodRow
err := retry.OnError(slowBackoff, isReadQuotaError, func() error {
var innerErr error
ret, innerErr = c.delegate.ListTestSummaryByPeriod(ctx, suiteName, releaseName, daysBack, minTestCount)
return innerErr
})
return ret, err
}

var slowBackoff = wait.Backoff{
Steps: 4,
Duration: 10 * time.Second,
Expand Down
1 change: 1 addition & 0 deletions pkg/jobrunaggregator/jobrunaggregatorlib/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const (
bigQueryLabelValueAllReleases = "aggregator-all-releases"
bigQueryLabelValueReleaseTags = "aggregator-release-tags"
bigQueryLabelValueJobRunIDsSinceTime = "aggregator-job-run-ids-since-time"
bigQueryLabelValueTestSummaryByPeriod = "aggregator-test-summary-by-period"
)

var (
Expand Down
84 changes: 82 additions & 2 deletions pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ type JobRunHistoricalDataAnalyzerOptions struct {

func (o *JobRunHistoricalDataAnalyzerOptions) Run(ctx context.Context) error {

var newHistoricalData []jobrunaggregatorapi.HistoricalData

// targetRelease will either be what the caller specified on the CLI, or the most recent release.
// previousRelease will be the one prior to targetRelease.
var targetRelease, previousRelease string
Expand All @@ -44,6 +42,14 @@ func (o *JobRunHistoricalDataAnalyzerOptions) Run(ctx context.Context) error {
}
fmt.Printf("Using target release: %s, previous release: %s\n", targetRelease, previousRelease)

// For tests data type, we don't do comparison - just fetch and write directly
if o.dataType == "tests" {
return o.runTestsDataType(ctx, targetRelease)
}

// For other data types (alerts, disruptions), continue with comparison logic
var newHistoricalData []jobrunaggregatorapi.HistoricalData

currentHistoricalData, err := readHistoricalDataFile(o.currentFile, o.dataType)
if err != nil {
return err
Expand Down Expand Up @@ -91,6 +97,80 @@ func (o *JobRunHistoricalDataAnalyzerOptions) Run(ctx context.Context) error {
return nil
}

func (o *JobRunHistoricalDataAnalyzerOptions) runTestsDataType(ctx context.Context, release string) error {
// Hardcoded parameters for test summary query
const (
suiteName = "openshift-tests"
daysBack = 30
minTestCount = 100
minDaysRequired = 10
)

fmt.Printf("Fetching test data for release %s, suite %s, last %d days, min %d test runs\n",
release, suiteName, daysBack, minTestCount)

// Try to read existing data if the output file exists
var existingTestSummaries []jobrunaggregatorapi.TestSummaryByPeriodRow
if _, err := os.Stat(o.outputFile); err == nil {
existingTestSummaries, err = readTestSummaryFile(o.outputFile)
if err != nil {
fmt.Printf("Warning: failed to read existing test summary file: %v\n", err)
existingTestSummaries = nil
} else {
fmt.Printf("Found existing test summary data with %d test results\n", len(existingTestSummaries))
}
}

// Fetch new test data from BigQuery
testSummaries, err := o.ciDataClient.ListTestSummaryByPeriod(ctx, suiteName, release, daysBack, minTestCount)
if err != nil {
return fmt.Errorf("failed to list test summary by period: %w", err)
}

// Validate the new data has sufficient days of data
hasSufficientData := hasSufficientDaysOfData(testSummaries, minDaysRequired)

// Determine which data to use
var finalTestSummaries []jobrunaggregatorapi.TestSummaryByPeriodRow
if len(testSummaries) == 0 {
// No new data available
if len(existingTestSummaries) > 0 {
fmt.Printf("Warning: no new test data found, keeping existing data with %d test results\n", len(existingTestSummaries))
finalTestSummaries = existingTestSummaries
} else {
return fmt.Errorf("no test data found for suite %s, release %s", suiteName, release)
}
} else if !hasSufficientData {
// New data exists but doesn't have enough days
if len(existingTestSummaries) > 0 {
fmt.Printf("Warning: new test data has insufficient days (< %d), keeping existing data with %d test results\n",
minDaysRequired, len(existingTestSummaries))
finalTestSummaries = existingTestSummaries
} else {
fmt.Printf("Warning: new test data has insufficient days (< %d) and no existing data available, using new data with %d test results\n",
minDaysRequired, len(testSummaries))
finalTestSummaries = testSummaries
}
} else {
// New data is sufficient
fmt.Printf("Using new test data with %d test results (sufficient days of data)\n", len(testSummaries))
finalTestSummaries = testSummaries
}

Comment thread
neisw marked this conversation as resolved.
// Write the final test summaries to the output file as JSON
out, err := formatTestOutput(finalTestSummaries)
if err != nil {
return fmt.Errorf("error formatting test output: %w", err)
}

if err := os.WriteFile(o.outputFile, out, 0644); err != nil {
return fmt.Errorf("failed to write output file: %w", err)
}

fmt.Printf("Successfully wrote %d test results to %s\n", len(finalTestSummaries), o.outputFile)
return nil
}

func (o *JobRunHistoricalDataAnalyzerOptions) getAlertData(ctx context.Context) ([]jobrunaggregatorapi.HistoricalData, error) {
var allKnownAlerts []*jobrunaggregatorapi.KnownAlertRow
var newHistoricalData []*jobrunaggregatorapi.AlertHistoricalDataRow
Expand Down
7 changes: 4 additions & 3 deletions pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type JobRunHistoricalDataAnalyzerFlags struct {
PreviousRelease string
}

var supportedDataTypes = sets.New[string]("alerts", "disruptions")
var supportedDataTypes = sets.New[string]("alerts", "disruptions", "tests")

func NewJobRunHistoricalDataAnalyzerFlags() *JobRunHistoricalDataAnalyzerFlags {
return &JobRunHistoricalDataAnalyzerFlags{
Expand Down Expand Up @@ -60,15 +60,16 @@ func (f *JobRunHistoricalDataAnalyzerFlags) Validate() error {
return fmt.Errorf("must provide supported datatype %v", sets.List(supportedDataTypes))
}

if f.CurrentFile == "" {
// For tests data type, we don't need --current since we don't do comparison
if f.DataType != "tests" && f.CurrentFile == "" {
return fmt.Errorf("must provide --current [file_path] flag to compare against")
}

if f.Leeway < 0 {
return fmt.Errorf("leeway percent must be above 0")
}

if f.TargetRelease != "" && f.PreviousRelease == "" {
if f.TargetRelease != "" && f.PreviousRelease == "" && f.DataType != "tests" {
return fmt.Errorf("must specify --previous-release with --target-release")
}

Expand Down
49 changes: 49 additions & 0 deletions pkg/jobrunaggregator/jobrunhistoricaldataanalyzer/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,52 @@ func formatOutput(data []parsedJobData, format string) ([]byte, error) {
return nil, fmt.Errorf("invalid output format (%s)", format)
}
}

func formatTestOutput(data []jobrunaggregatorapi.TestSummaryByPeriodRow) ([]byte, error) {
if len(data) == 0 {
return nil, nil
}
// Sort by release, failure count desc, test name
sort.SliceStable(data, func(i, j int) bool {
if data[i].Release != data[j].Release {
return data[i].Release < data[j].Release
}
if data[i].TotalFailureCount != data[j].TotalFailureCount {
return data[i].TotalFailureCount > data[j].TotalFailureCount
}
return data[i].TestName < data[j].TestName
})
return json.MarshalIndent(data, "", " ")
}

// readTestSummaryFile reads test summary data from a JSON file
func readTestSummaryFile(filePath string) ([]jobrunaggregatorapi.TestSummaryByPeriodRow, error) {
data, err := os.ReadFile(filePath)
if err != nil {
return nil, fmt.Errorf("failed to read file at path (%s): %w", filePath, err)
}

var testSummaries []jobrunaggregatorapi.TestSummaryByPeriodRow
if err := json.Unmarshal(data, &testSummaries); err != nil {
return nil, fmt.Errorf("failed to unmarshal test summary data: %w", err)
}

return testSummaries, nil
}

// hasSufficientDaysOfData checks if test summaries have at least minDays of data
// Returns true if any row has DaysWithData >= minDays
func hasSufficientDaysOfData(testSummaries []jobrunaggregatorapi.TestSummaryByPeriodRow, minDays int64) bool {
if len(testSummaries) == 0 {
return false
}

// Check if any test has sufficient days of data
for _, summary := range testSummaries {
if summary.DaysWithData >= minDays {
return true
}
}

return false
}