WIP: API to list all test failures from a specific PR #3133
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
dgoodwin
changed the title
WalkthroughIntroduces PR test results API functionality by adding a new module to retrieve and serve test failure data from BigQuery, integrating it into the sippyserver with appropriate HTTP handlers and parameter validation for date ranges and PR identifiers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/api/prtestresults.go(1 hunks)pkg/sippyserver/server.go(2 hunks)pkg/util/param/param.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/prtestresults.gopkg/sippyserver/server.gopkg/util/param/param.go
🧬 Code graph analysis (1)
pkg/sippyserver/server.go (2)
pkg/api/prtestresults.go (1)
PrintPRTestResultsJSON(290-402)pkg/sippyserver/capabilities.go (1)
ComponentReadinessCapability(15-15)
🔇 Additional comments (5)
pkg/util/param/param.go (1)
29-29: LGTM!The new parameter validations follow the established patterns in this file. The
dateRegexpvalidates the YYYY-MM-DD format structurally, and semantic date validation (e.g., rejecting invalid dates like2024-99-99) is correctly handled bytime.Parsein the API handler.Also applies to: 44-46, 52-53
pkg/sippyserver/server.go (2)
1026-1032: LGTM!The handler follows the established pattern for BigQuery-dependent endpoints, with proper nil-check for
bigQueryClientand delegation to the API implementation.
2028-2033: LGTM!The endpoint registration is well-structured with appropriate capability requirements and clear documentation.
pkg/api/prtestresults.go (2)
39-75: LGTM!The function correctly queries both BigQuery tables and combines results. The logging is comprehensive for debugging.
212-240: LGTM!The query execution follows proper BigQuery iteration patterns with appropriate error handling.
| { | ||
| Name: "PRNumber", | ||
| Value: strconv.Itoa(prNumber), | ||
| }, |
There was a problem hiding this comment.
Potential type mismatch: PRNumber passed as string.
The PRNumber parameter is converted to a string via strconv.Itoa(prNumber), but the SQL uses jobs.pr_number = @PRNumber which likely expects an integer comparison. BigQuery may handle implicit conversion, but passing the integer directly would be more correct and avoid potential type coercion issues.
{
Name: "PRNumber",
- Value: strconv.Itoa(prNumber),
+ Value: prNumber,
},📝 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.
| { | |
| Name: "PRNumber", | |
| Value: strconv.Itoa(prNumber), | |
| }, | |
| { | |
| Name: "PRNumber", | |
| Value: prNumber, | |
| }, |
🤖 Prompt for AI Agents
In pkg/api/prtestresults.go around lines 186 to 189, the PRNumber query
parameter is being passed as a string via strconv.Itoa(prNumber); change this to
pass the integer value (or int64) directly and ensure the parameter's type
reflects an integer so it binds as an integer for the SQL comparison (remove
strconv.Itoa and set the parameter Value/type to the integer value).
| for i, field := range schema { | ||
| switch field.Name { | ||
| case "prowjob_build_id": | ||
| result.ProwJobBuildID = row[i].(string) | ||
| case "prowjob_name": | ||
| result.ProwJobName = row[i].(string) | ||
| case "prowjob_url": | ||
| if row[i] != nil { | ||
| result.ProwJobURL = row[i].(string) | ||
| } | ||
| case "pr_sha": | ||
| if row[i] != nil { | ||
| result.PRSha = row[i].(string) | ||
| } | ||
| case "prowjob_start": | ||
| if row[i] != nil { | ||
| // BigQuery returns civil.DateTime for DATETIME columns | ||
| civilDT := row[i].(civil.DateTime) | ||
| layout := "2006-01-02T15:04:05" | ||
| parsedTime, err := time.Parse(layout, civilDT.String()) | ||
| if err != nil { | ||
| return PRTestResult{}, errors.Wrap(err, "failed to parse prowjob_start") | ||
| } | ||
| result.ProwJobStart = parsedTime | ||
| } | ||
| case "test_name": | ||
| result.TestName = row[i].(string) | ||
| case "testsuite": | ||
| result.TestSuite = row[i].(string) | ||
| case "flaked": | ||
| result.Flaked = row[i].(bool) | ||
| case "success": | ||
| result.Success = row[i].(bool) | ||
| } | ||
| } |
There was a problem hiding this comment.
Type assertions may panic on nil values.
Fields like prowjob_build_id, prowjob_name, test_name, testsuite, flaked, and success are type-asserted without nil checks. If BigQuery returns NULL for any of these columns, the code will panic at runtime.
Consider adding nil checks for all fields, consistent with how prowjob_url and pr_sha are handled:
case "prowjob_build_id":
- result.ProwJobBuildID = row[i].(string)
+ if row[i] != nil {
+ result.ProwJobBuildID = row[i].(string)
+ }
case "prowjob_name":
- result.ProwJobName = row[i].(string)
+ if row[i] != nil {
+ result.ProwJobName = row[i].(string)
+ }Apply similar changes to test_name, testsuite, flaked, and success.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/api/prtestresults.go around lines 250 to 284, several case branches
perform direct type assertions that will panic if row[i] is nil; make them
consistent with prowjob_url and pr_sha by checking row[i] != nil before
asserting. For string fields (prowjob_build_id, prowjob_name, test_name,
testsuite) wrap the assignment in an if row[i] != nil { result.Field =
row[i].(string) } else leave as empty string; for boolean fields (flaked,
success) likewise check if row[i] != nil before asserting and assign only when
non-nil (otherwise leave default false). Keep the existing prowjob_start parsing
as-is.
| // Add one day to end_date to make it inclusive | ||
| endDate = endDate.AddDate(0, 0, 1) | ||
|
|
||
| // Validate date range | ||
| if endDate.Before(startDate) { | ||
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | ||
| "code": http.StatusBadRequest, | ||
| "message": "end_date must be after start_date", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Date validation order issue: adding a day before range check may allow invalid ranges.
The code adds 1 day to end_date (line 365) before validating that end_date is after start_date (line 368). This allows edge cases where the original end_date equals start_date minus 1 day to pass validation.
Example: start_date=2024-01-02, end_date=2024-01-01
- After line 365:
endDate = 2024-01-02 - Line 368:
endDate.Before(startDate)isfalse, validation passes - But the user intended an invalid range where end is before start.
Consider validating the original dates before adjusting:
+ // Validate date range before adjustment
+ if endDate.Before(startDate) {
+ RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{
+ "code": http.StatusBadRequest,
+ "message": "end_date must be after or equal to start_date",
+ })
+ return
+ }
+
// Add one day to end_date to make it inclusive
endDate = endDate.AddDate(0, 0, 1)
-
- // Validate date range
- if endDate.Before(startDate) {
- RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{
- "code": http.StatusBadRequest,
- "message": "end_date must be after start_date",
- })
- return
- }📝 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.
| // Add one day to end_date to make it inclusive | |
| endDate = endDate.AddDate(0, 0, 1) | |
| // Validate date range | |
| if endDate.Before(startDate) { | |
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | |
| "code": http.StatusBadRequest, | |
| "message": "end_date must be after start_date", | |
| }) | |
| return | |
| } | |
| // Validate date range before adjustment | |
| if endDate.Before(startDate) { | |
| RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{ | |
| "code": http.StatusBadRequest, | |
| "message": "end_date must be after or equal to start_date", | |
| }) | |
| return | |
| } | |
| // Add one day to end_date to make it inclusive | |
| endDate = endDate.AddDate(0, 0, 1) |
🤖 Prompt for AI Agents
In pkg/api/prtestresults.go around lines 364 to 374, the code currently adds one
day to endDate before checking the date range which can make originally-invalid
ranges appear valid; to fix this, validate that the original endDate is not
before startDate first (or compare startDate to a copy of endDateBeforeAdjust :=
endDate), return the bad request if endDate < startDate, and only after that add
one day to endDate to make the range inclusive.
|
@dgoodwin: The following tests failed, say
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. |
3 participants
Intent is to hand this off to an AI to examine if there's anything fishy going on across all results from a PR. Critically this covers both /payload and presubmits, the former does not exist in sippy's db. As such we pull from bigquery with a user specified date range, max 30 days, which is about 9 GB total across the two tables at today's rate of junit testing. Also supports including successes for specific test names or substrings, but including all successes was 100k results on the PR I was testing against.