Make the v2 test outputs API more generally usable to list all executions and include successes#3249
Make the v2 test outputs API more generally usable to list all executions and include successes#3249dgoodwin wants to merge 2 commits intoopenshift:mainfrom
Conversation
…ions and include successes
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughRename and extend BigQuery test-output retrieval to return test runs with additional filters (prowJobRunIDs, prowJobNames, includeSuccess) and date windowing; add TestOutputBigQuery type and ProwJobURL/StartTime/Success/ProwJobName fields; update HTTP handler and endpoint path to expose the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as sippyserver
participant BQ as BigQuery
Client->>Server: GET /api/tests/v2/runs?test_id=...&prow_job_run_ids=...&include_success=true&start=...&end=...
Server->>Server: parse params (ids, names, include_success, start/end)
Server->>BQ: Query with dynamic filters, join jobs, limit 500
BQ-->>Server: rows (output, test_name, success, prowjob_name, prowjob_url, prowjob_start)
Server->>Client: JSON response ([]TestOutputBigQuery)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sippyserver/server.go (1)
781-810:⚠️ Potential issue | 🟡 MinorReject
start_dateafterend_date.Currently a reversed range slips through and yields empty results without feedback. Consider returning 400 when
startDate.After(endDate).💡 Suggested fix
// Validate date range does not exceed 30 days (expensive query) + if startDate.After(endDate) { + failureResponse(w, http.StatusBadRequest, "start_date must be on or before end_date") + return + } dateRange := endDate.Sub(startDate)
🤖 Fix all issues with AI agents
In `@pkg/api/tests.go`:
- Around line 69-74: The SQL query string that currently uses INNER JOIN
`openshift-gce-devel.ci_analysis_us.jobs` AS jobs should be changed to a LEFT
JOIN so junit rows without matching job metadata are preserved; keep the INNER
JOIN on test_mapping as-is, change only the join on jobs to LEFT JOIN (alias
jobs) so prowjob_url and prowjob_start may be NULL but all executions are
returned, and ensure the WHERE clause on junit.modified_time remains unchanged.
In `@pkg/apis/api/types.go`:
- Around line 879-886: The DB query in TestOutputs is not selecting the test
status so GORM leaves TestOutput.Success false; update the SELECT in the
TestOutputs query to include prow_job_run_tests.status (or a computed boolean
like (prow_job_run_tests.status = 'success') AS success) so that when GORM scans
into the TestOutput struct the Success field is populated the same way
GetTestRunsAndOutputsFromBigQuery does; locate the TestOutputs function and add
the status/boolean selection and mapping to TestOutput.Success.
| SELECT junit.prowjob_build_id, junit.test_name, junit.success, junit.test_id, junit.branch, junit.prowjob_name, junit.failure_content, | ||
| jobs.prowjob_url, jobs.prowjob_start | ||
| FROM ` + "`openshift-gce-devel.ci_analysis_us.junit`" + ` AS junit | ||
| INNER JOIN test_mapping ON junit.test_name = test_mapping.name AND junit.testsuite = test_mapping.suite | ||
| WHERE junit.success = false | ||
| AND junit.prowjob_build_id IN UNNEST(@prowJobRunIDs) | ||
| AND junit.modified_time BETWEEN DATETIME(@startDate) AND DATETIME(@endDate) | ||
| LIMIT 1000` | ||
| INNER JOIN ` + "`openshift-gce-devel.ci_analysis_us.jobs`" + ` AS jobs ON junit.prowjob_build_id = jobs.prowjob_build_id | ||
| WHERE junit.modified_time BETWEEN DATETIME(@startDate) AND DATETIME(@endDate)` |
There was a problem hiding this comment.
Avoid dropping runs when job metadata is missing (use LEFT JOIN).
Line 73 introduces an INNER JOIN on jobs; any junit row without a matching job row will be excluded, which conflicts with the goal to list all executions. A LEFT JOIN preserves those rows while still filling prowjob_url/prowjob_start when available.
💡 Suggested fix
-INNER JOIN `openshift-gce-devel.ci_analysis_us.jobs` AS jobs ON junit.prowjob_build_id = jobs.prowjob_build_id
+LEFT JOIN `openshift-gce-devel.ci_analysis_us.jobs` AS jobs ON junit.prowjob_build_id = jobs.prowjob_build_id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SELECT junit.prowjob_build_id, junit.test_name, junit.success, junit.test_id, junit.branch, junit.prowjob_name, junit.failure_content, | |
| jobs.prowjob_url, jobs.prowjob_start | |
| FROM ` + "`openshift-gce-devel.ci_analysis_us.junit`" + ` AS junit | |
| INNER JOIN test_mapping ON junit.test_name = test_mapping.name AND junit.testsuite = test_mapping.suite | |
| WHERE junit.success = false | |
| AND junit.prowjob_build_id IN UNNEST(@prowJobRunIDs) | |
| AND junit.modified_time BETWEEN DATETIME(@startDate) AND DATETIME(@endDate) | |
| LIMIT 1000` | |
| INNER JOIN ` + "`openshift-gce-devel.ci_analysis_us.jobs`" + ` AS jobs ON junit.prowjob_build_id = jobs.prowjob_build_id | |
| WHERE junit.modified_time BETWEEN DATETIME(@startDate) AND DATETIME(@endDate)` | |
| SELECT junit.prowjob_build_id, junit.test_name, junit.success, junit.test_id, junit.branch, junit.prowjob_name, junit.failure_content, | |
| jobs.prowjob_url, jobs.prowjob_start | |
| FROM ` + "`openshift-gce-devel.ci_analysis_us.junit`" + ` AS junit | |
| INNER JOIN test_mapping ON junit.test_name = test_mapping.name AND junit.testsuite = test_mapping.suite | |
| LEFT JOIN ` + "`openshift-gce-devel.ci_analysis_us.jobs`" + ` AS jobs ON junit.prowjob_build_id = jobs.prowjob_build_id | |
| WHERE junit.modified_time BETWEEN DATETIME(`@startDate`) AND DATETIME(`@endDate`)` |
🤖 Prompt for AI Agents
In `@pkg/api/tests.go` around lines 69 - 74, The SQL query string that currently
uses INNER JOIN `openshift-gce-devel.ci_analysis_us.jobs` AS jobs should be
changed to a LEFT JOIN so junit rows without matching job metadata are
preserved; keep the INNER JOIN on test_mapping as-is, change only the join on
jobs to LEFT JOIN (alias jobs) so prowjob_url and prowjob_start may be NULL but
all executions are returned, and ensure the WHERE clause on junit.modified_time
remains unchanged.
| type TestOutput struct { | ||
| URL string `json:"url"` | ||
| Output string `json:"output"` | ||
| TestName string `json:"test_name,omitempty"` | ||
| ProwJobURL string `json:"url"` | ||
| GCSBucket string `json:"gcs_bucket,omitempty"` | ||
| Output string `json:"output"` | ||
| TestName string `json:"test_name,omitempty"` | ||
| Success bool `json:"success"` | ||
| ProwJobName string `json:"prowjob_name,omitempty"` | ||
| StartTime *time.Time `json:"start_time,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all TestOutput struct initializations and their usage patterns
rg -n --type=go 'TestOutput\s*\{' -A 5 -B 2Repository: openshift/sippy
Length of output: 1274
🏁 Script executed:
# Find any functions that return TestOutput
rg -n --type=go 'func.*TestOutput'Repository: openshift/sippy
Length of output: 826
🏁 Script executed:
# Search for TestOutput variable assignments
rg -n --type=go ':\s*TestOutput|=.*TestOutput' -B 2 -A 5Repository: openshift/sippy
Length of output: 3495
🏁 Script executed:
# Read the GetTestOutputsFromDB function in full
sed -n '42,120p' pkg/api/tests.goRepository: openshift/sippy
Length of output: 2742
🏁 Script executed:
# Read the GetTestRunsAndOutputsFromBigQuery function in full
sed -n '59,200p' pkg/api/tests.goRepository: openshift/sippy
Length of output: 4626
🏁 Script executed:
# Also check the test_queries.go function that creates TestOutput slices
sed -n '277,350p' pkg/db/query/test_queries.goRepository: openshift/sippy
Length of output: 2600
🏁 Script executed:
# Check TestOutput struct definition to see if Success is indeed a field
sed -n '870,890p' pkg/apis/api/types.goRepository: openshift/sippy
Length of output: 792
🏁 Script executed:
# Search for prow_job_run_test_outputs model to see what columns it has
rg -n 'type.*ProwJobRunTestOutput|prow_job_run_test_output' --type=go -A 10Repository: openshift/sippy
Length of output: 2402
🏁 Script executed:
# Check if there's a success column in the model
rg -n 'Success' pkg/models/ --type=go | head -20Repository: openshift/sippy
Length of output: 110
🏁 Script executed:
# Check the ProwJobRunTest model to see if it has Success field
rg -n 'type ProwJobRunTest' pkg/db/models/ --type=go -A 20Repository: openshift/sippy
Length of output: 2439
🏁 Script executed:
# Check the test_queries.go more carefully - does Success come from prow_job_run_tests?
sed -n '277,304p' pkg/db/query/test_queries.goRepository: openshift/sippy
Length of output: 1166
🏁 Script executed:
# Verify the Status field in ProwJobRunTest is related to success/failure
rg -n 'Status.*TestStatus|TestStatus' pkg/db/models/ --type=go -B 2 -A 2Repository: openshift/sippy
Length of output: 41
🏁 Script executed:
# Check how Status is used when creating ProwJobRunTest records
rg -n 'ProwJobRunTest.*Status|\.Status\s*=' pkg/dataloader/ --type=go | head -20Repository: openshift/sippy
Length of output: 549
🏁 Script executed:
# Check the sippyprocessingv1 package to understand TestStatus enum
rg -n 'TestStatus|TestStatusSuccess|TestStatusFailure' --type=go pkg/ | grep -E '(const|TestStatus)' | head -20Repository: openshift/sippy
Length of output: 2577
🏁 Script executed:
# Let me verify the full context of how the database path constructs TestOutput
# Check if there's any mapping of Status to Success in the database path
rg -n 'Success.*Status|Status.*Success' pkg/api/ --type=go -B 3 -A 3Repository: openshift/sippy
Length of output: 41
🏁 Script executed:
# Let me check what fields are actually being scanned from the database query
# Look at the Scan call and what struct fields would match
sed -n '275,305p' pkg/db/query/test_queries.goRepository: openshift/sippy
Length of output: 1170
🏁 Script executed:
# Check if there's a custom scan or mapping happening for TestOutput
rg -n 'type TestOutput' pkg/apis/api/ --type=go -B 2 -A 15Repository: openshift/sippy
Length of output: 1134
TestOutput.Success in the database query path is always false—add the Status field selection to distinguish real failures.
The database query in pkg/db/query/test_queries.go at lines 277–304 (the TestOutputs function) only selects "prow_job_runs.url, output" and does not retrieve the Status field from the joined prow_job_run_tests table. When GORM scans these results into TestOutput, the Success bool field will be zero-initialized to false for all records, making it indistinguishable from actual test failures.
The BigQuery path (GetTestRunsAndOutputsFromBigQuery) correctly handles this by explicitly selecting junit.success and mapping it to Success. Update the database query to also select prow_job_run_tests.status (or compute Success from it) to match the BigQuery behavior.
🤖 Prompt for AI Agents
In `@pkg/apis/api/types.go` around lines 879 - 886, The DB query in TestOutputs is
not selecting the test status so GORM leaves TestOutput.Success false; update
the SELECT in the TestOutputs query to include prow_job_run_tests.status (or a
computed boolean like (prow_job_run_tests.status = 'success') AS success) so
that when GORM scans into the TestOutput struct the Success field is populated
the same way GetTestRunsAndOutputsFromBigQuery does; locate the TestOutputs
function and add the status/boolean selection and mapping to TestOutput.Success.
There was a problem hiding this comment.
this looks legit, this re-used a postgres type. Working on it.
There was a problem hiding this comment.
@dgoodwin, thank you for confirming and taking care of this! Looking forward to seeing the fix.
|
Scheduling required tests: |
|
Scheduling required tests: |
|
@dgoodwin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
These changes are intended to let claude examine history further back to determine when a test went bad. In testing it actually seems to work.
End goal will be find the first job run, check what's in the payload, point the finger.
Summary by CodeRabbit
New Features
Refactor