TRT-2821: Fall back to aggregate tables for base stats in test_details - #3847
TRT-2821: Fall back to aggregate tables for base stats in test_details#3847mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble 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
232123b to
ba1a34e
Compare
mstaeble
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2821 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughChangesComponent readiness test-detail flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestDetailsQuerier
participant BigQueryProvider
participant PostgresProvider
participant SummarizeTestJobRuns
participant TestDetailsReport
TestDetailsQuerier->>BigQueryProvider: query base or sample status
BigQueryProvider->>SummarizeTestJobRuns: pass raw job-run rows
SummarizeTestJobRuns-->>BigQueryProvider: return TestDetailsSummary values
TestDetailsQuerier->>PostgresProvider: query base status
PostgresProvider->>PostgresProvider: use detailed data or aggregate fallback
PostgresProvider-->>TestDetailsQuerier: return TestDetailsSummary values
TestDetailsQuerier->>TestDetailsReport: provide summarized status data
TestDetailsReport->>TestDetailsReport: combine aggregate statistics and nested run details
Suggested reviewers: 🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
pkg/apis/api/componentreport/crstatus/summarize.go (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
SuccessRatein the summary is provisional.
AddTestCountreceives a hardcodedfalseforflakeAsFailure. The counts are independent of that flag, butStats.SuccessRateis not. Consumers must recompute the rate with the request'sFlakeAsFailure, assummarizeRecordedTestStatsdoes inpkg/api/componentreadiness/test_details.go. A short comment prevents a future consumer from readingsummary.Stats.SuccessRateas authoritative.♻️ Proposed comment
+ // Counts are flake-policy independent; SuccessRate here is provisional and + // callers must recompute it with the request's FlakeAsFailure setting. summary.Stats = summary.Stats.AddTestCount(row.Count, false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/api/componentreport/crstatus/summarize.go` at line 27, Add a concise comment at the summary.Stats assignment in summarizeRecordedTestStats documenting that SuccessRate is provisional because AddTestCount uses flakeAsFailure=false, and that consumers must recompute it using the request’s FlakeAsFailure setting.pkg/apis/api/componentreport/crstatus/summarize_test.go (1)
12-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage beyond lifecycle promotion.
The table covers only
Lifecycle.SummarizeTestJobRunsalso implements behavior that a regression would hide:
- Count aggregation across rows into
Stats.- Skipping
JobRunsentries whenProwJobRunIDis empty (the aggregate fallback path depends on this).- First-non-empty selection for
JiraComponent,JiraComponentID, andTestName.- Grouping of several test keys under one job, and stable first-seen ordering.
These are pure-logic checks and need no database. Do you want me to generate the additional table-driven cases?
As per path instructions: "Prefer table-driven Go tests with descriptive case names, and search the same package for existing test patterns before adding new tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/api/componentreport/crstatus/summarize_test.go` around lines 12 - 70, Extend TestSummarizeTestJobRuns_LifecyclePromotion or add a companion table-driven test to cover Stats count aggregation, skipping rows with empty ProwJobRunID, first-non-empty JiraComponent/JiraComponentID/TestName selection, and grouping multiple test keys under one job while preserving first-seen order. Follow existing same-package test patterns and assert the complete summarized output for each descriptive case.Source: Path instructions
pkg/api/componentreadiness/dataprovider/postgres/provider.go (3)
483-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog when the aggregate fallback is used.
The fallback is silent. Reports built from the aggregate path contain no
JobRuns, so the UI shows no individual runs. A single structured log line makes that state diagnosable.♻️ Proposed change
if len(result) > 0 { return result, nil } + log.WithField("release", reqOptions.BaseRelease.Name). + Info("no per-run base test details found, falling back to aggregate tables") return p.queryBaseAggregateTestDetails(ctx, reqOptions)As per coding guidelines: "Prefer structured logging, especially for names and IDs, and prefer
log.WithField()over formatting values into log strings when appropriate."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go` around lines 483 - 486, In the result fallback within the provider method, add one structured log entry immediately before calling queryBaseAggregateTestDetails, using the existing logger and fields for relevant names or IDs rather than formatting values into the message. Keep the successful result return unchanged and preserve the existing aggregate fallback behavior.Source: Coding guidelines
629-635: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated variant-filter append.
The same block now appears three times in this file:
queryTestDetails(lines 378-384),buildAggregatePrefixSumQuery(lines 591-597), and here. A small helper that returns the clause and appends the args keeps the three call sites in sync.♻️ Proposed helper
// appendVariantFilter appends the variant-combination subquery and its args when // includeVariants produces a filter clause. func appendVariantFilter(sqlQuery string, args []any, includeVariants map[string][]string) (string, []any) { if len(includeVariants) == 0 { return sqlQuery, args } filterClause, filterArgs := buildVariantFilterClause(includeVariants) if filterClause == "" { return sqlQuery, args } return sqlQuery + " AND pj.variant_combination_id IN (SELECT vc.id FROM variant_combinations vc WHERE " + filterClause + ")", append(args, filterArgs...) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go` around lines 629 - 635, Extract the duplicated variant-filter logic into an appendVariantFilter helper that accepts the SQL string, args, and includeVariants, then returns the updated values while preserving empty-input and empty-clause behavior. Replace the repeated blocks in queryTestDetails, buildAggregatePrefixSumQuery, and the shown query flow with calls to this helper so all three sites remain consistent.
656-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared requested-variant filtering and key construction.
Lines 656-698 duplicate
queryTestDetailslines 406-465: therequestedVariantsByTestIDmap build, the variant match loop, thefilterByDBGroupBycall, theKeyWithVariantsconstruction, and thebig.Ratconversion ofJiraComponentID. The two paths must stay behaviorally identical, so a shared helper reduces the risk that only one path is updated later.A helper such as
matchAndBuildTestKey(row testID string, variants map[string]string, requested map[string]map[string]string, dbGroupBy sets.Set[string]) (crtest.KeyWithVariants, bool)covers both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go` around lines 656 - 698, Extract the duplicated requested-variant matching and test-key construction from queryTestDetails and the shown result-building loop into a shared helper, such as matchAndBuildTestKey. Have it apply the requested variant filter, call filterByDBGroupBy, and build the crtest.KeyWithVariants consistently; update both callers to use it and preserve the existing JiraComponentID big.Rat conversion in the surrounding row-processing logic.pkg/api/componentreadiness/test_details.go (2)
502-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the metadata-merge and lifecycle-promotion rule with
crstatus.Lines 503-518 repeat the exact logic in
pkg/apis/api/componentreport/crstatus/summarize.golines 41-56: first-non-empty selection forJiraComponent,JiraComponentID, andTestName, plus promotion ofLifecycleto"informing". Two copies of the promotion rule can diverge if a new lifecycle value is added.Move the rule into an exported helper in
crstatusand call it from both places.extractMetadataalso does not use its receiverc, so it can become a package-level function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/componentreadiness/test_details.go` around lines 502 - 519, Move the shared metadata merge and lifecycle-promotion logic from ComponentReportGenerator.extractMetadata and crstatus summarization into an exported helper in crstatus, then call that helper from both callers. Convert extractMetadata to a package-level function because it does not use c, and preserve the existing first-non-empty field selection and "informing" lifecycle promotion behavior.
458-483: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the one-summary-per-job precondition.
Both inner loops assign
jobStats.SampleJobNameandjobStats.BaseJobNameon every iteration. If a job maps to more than one summary, the last summary wins and the earlier job names are lost, while the counts still accumulate across all of them.Callers satisfy the precondition today:
GenerateDetailsReportForTestreceives statuses already split by test key. A short comment records that assumption for future callers.♻️ Proposed comment
jobNames := sets.New(slices.Collect(maps.Keys(baseStatus))...) jobNames.Insert(slices.Collect(maps.Keys(sampleStatus))...) + // Callers pass statuses already split by test key, so each job maps to at most + // one summary here; the job-name assignments below rely on that. for job := range jobNames {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/componentreadiness/test_details.go` around lines 458 - 483, Add a concise comment near the sampleStatus/baseStatus processing in the job loop documenting that callers provide at most one summary per job, with GenerateDetailsReportForTest supplying statuses split by test key. Do not alter the existing aggregation or name-assignment logic.test/integration/component_readiness_test.go (1)
2622-2635: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePositional indexing of
JobRunsdepends on the query ordering.The assertions map
JobRuns[0],[1], and[2]to the pass, fail, and flake runs. That holds only becausequeryTestDetailsappliesORDER BY pjr.timestampand the summarizer appends in row order. If the ordering clause changes, these assertions fail with a message that does not name the cause.Selecting each run by
ProwJobRunIDmakes the intent explicit and removes the ordering dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/component_readiness_test.go` around lines 2622 - 2635, Replace the positional JobRuns[0], JobRuns[1], and JobRuns[2] lookups in the pass/fail/flake assertions with selections by each run’s ProwJobRunID. Preserve the existing count and success/flake assertions while making each detail lookup independent of queryTestDetails ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go`:
- Around line 501-504: Update the doc comment for
PostgresProvider.queryBaseAggregateTestDetails to state that it returns
map[string][]crstatus.TestDetailsSummary values instead of TestJobRunRows
entries.
In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go`:
- Line 252: Update the logging call in the release fallback test-key handling to
use Debug-level structured logging with testKeyStr as a named field, rather than
concatenating it into the Infof format string. Preserve the existing message
context while ensuring arbitrary key contents, including percent signs, are
logged safely.
In `@pkg/api/componentreadiness/test_details.go`:
- Around line 410-411: Update summarizeRecordedTestStats to add an explanation
when aggregate base summaries contain counts but no JobRuns, so the report
explains why BaseJobRunStats has no individual runs. Populate
testStats.Explanations only for this missing-detail condition and preserve empty
explanations for cases where base runs are available.
In `@pkg/apis/api/componentreport/crstatus/types.go`:
- Around line 52-56: Update GetDataFromCacheOrGenerate to restore TestKeyStr for
cached TestDetailsSummary and TestJobRunRows values by deriving it from each
corresponding TestKey after json.Unmarshal. Ensure both cached result types
retain the same grouped test-key behavior as freshly generated data.
---
Nitpick comments:
In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go`:
- Around line 483-486: In the result fallback within the provider method, add
one structured log entry immediately before calling
queryBaseAggregateTestDetails, using the existing logger and fields for relevant
names or IDs rather than formatting values into the message. Keep the successful
result return unchanged and preserve the existing aggregate fallback behavior.
- Around line 629-635: Extract the duplicated variant-filter logic into an
appendVariantFilter helper that accepts the SQL string, args, and
includeVariants, then returns the updated values while preserving empty-input
and empty-clause behavior. Replace the repeated blocks in queryTestDetails,
buildAggregatePrefixSumQuery, and the shown query flow with calls to this helper
so all three sites remain consistent.
- Around line 656-698: Extract the duplicated requested-variant matching and
test-key construction from queryTestDetails and the shown result-building loop
into a shared helper, such as matchAndBuildTestKey. Have it apply the requested
variant filter, call filterByDBGroupBy, and build the crtest.KeyWithVariants
consistently; update both callers to use it and preserve the existing
JiraComponentID big.Rat conversion in the surrounding row-processing logic.
In `@pkg/api/componentreadiness/test_details.go`:
- Around line 502-519: Move the shared metadata merge and lifecycle-promotion
logic from ComponentReportGenerator.extractMetadata and crstatus summarization
into an exported helper in crstatus, then call that helper from both callers.
Convert extractMetadata to a package-level function because it does not use c,
and preserve the existing first-non-empty field selection and "informing"
lifecycle promotion behavior.
- Around line 458-483: Add a concise comment near the sampleStatus/baseStatus
processing in the job loop documenting that callers provide at most one summary
per job, with GenerateDetailsReportForTest supplying statuses split by test key.
Do not alter the existing aggregation or name-assignment logic.
In `@pkg/apis/api/componentreport/crstatus/summarize_test.go`:
- Around line 12-70: Extend TestSummarizeTestJobRuns_LifecyclePromotion or add a
companion table-driven test to cover Stats count aggregation, skipping rows with
empty ProwJobRunID, first-non-empty JiraComponent/JiraComponentID/TestName
selection, and grouping multiple test keys under one job while preserving
first-seen order. Follow existing same-package test patterns and assert the
complete summarized output for each descriptive case.
In `@pkg/apis/api/componentreport/crstatus/summarize.go`:
- Line 27: Add a concise comment at the summary.Stats assignment in
summarizeRecordedTestStats documenting that SuccessRate is provisional because
AddTestCount uses flakeAsFailure=false, and that consumers must recompute it
using the request’s FlakeAsFailure setting.
In `@test/integration/component_readiness_test.go`:
- Around line 2622-2635: Replace the positional JobRuns[0], JobRuns[1], and
JobRuns[2] lookups in the pass/fail/flake assertions with selections by each
run’s ProwJobRunID. Preserve the existing count and success/flake assertions
while making each detail lookup independent of queryTestDetails ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d67c79c2-a323-4559-9b9b-1895f58b479f
📒 Files selected for processing (12)
pkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/test_details.gopkg/apis/api/componentreport/crstatus/summarize.gopkg/apis/api/componentreport/crstatus/summarize_test.gopkg/apis/api/componentreport/crstatus/types.gotest/integration/component_readiness_test.go
Refactor test details to use TestDetailsSummary with pre-computed Stats (crtest.Stats) instead of raw Count aggregation. This fixes a failure count bug where aggregating Count values before computing failures lost hard failures when flakes coexisted. Move per-job summarization into SummarizeTestJobRuns in the crstatus package, making it shared between providers. Add aggregate table fallback for base stats in the Postgres provider when per-run data is absent. Add integration tests for end-to-end report generation covering aggregate base stats fallback, last failure tracking, and FlakeAsFailure mode. Add unit test for lifecycle "informing" promotion in SummarizeTestJobRuns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ba1a34e to
9d2a7f8
Compare
openshift-ci
Bot
added
the
ready-for-human-review
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@mstaeble: 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
Summary
TestDetailsSummaryas the provider return type for test details queries, replacing raw per-run rows with pre-computed per-job stats and optional individual run details.SummarizeTestJobRunsto group raw rows by (job, test key) into summaries, applied consistently in both BigQuery and Postgres providers.QueryBaseJobRunTestStatus(Postgres provider) to populate base statistics from aggregate tables (test_cumulative_summariesfor prefix-sum releases,prow_ga_raw_test_datafor GA releases) when per-run data is unavailable for older base releases. Aggregate summaries have no individualJobRuns, so the UI shows stats without broken run links.test_details.goreport generation to work with pre-summarized data, simplifying stats aggregation and fixing a bug where per-run counts were double-aggregated.TestDetailsSummary.TestKeyStrserialization (json:"-"removed) so test key grouping survives the BQ Redis cache round-trip.Test plan
SummarizeTestJobRunslifecycle promotion logicmake testpassesmake lintpassesStaging verification
Ran local server against staging Postgres DB and BigQuery with the branch code. Tested with explicit query parameters (5.0 sample, 4.22 base, Build and Networking components).
Postgres path: per-run base data (4.22 base)
Postgres path: aggregate fallback (4.17 base)
BigQuery path (4.22 base)
Networking test cross-path comparison
CR grid (Postgres)
No errors in server logs across all requests.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests