TRT-2821: Fall back to aggregate tables for base stats in test_details - #3847
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. |
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. |
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
WalkthroughThis pull request replaces raw test job-run rows with ChangesComponent readiness test-detail pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QueryBaseJobRunTestStatus
participant queryTestDetails
participant AggregateFallback
participant TestDetailsReport
QueryBaseJobRunTestStatus->>queryTestDetails: fetch detailed test data
alt Detailed data exists
queryTestDetails-->>QueryBaseJobRunTestStatus: return summaries
else Detailed data is absent
QueryBaseJobRunTestStatus->>AggregateFallback: query aggregate statistics
AggregateFallback-->>QueryBaseJobRunTestStatus: return summaries
end
QueryBaseJobRunTestStatus-->>TestDetailsReport: provide summarized status
TestDetailsReport->>TestDetailsReport: build report statistics
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 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
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: |
9d2a7f8 to
1f5d6e3
Compare
|
Scheduling required tests: |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/api/componentreadiness/test_details.go (1)
514-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared helper for lifecycle promotion.
This promotion rule repeats the logic in
SummarizeTestJobRunsatpkg/apis/api/componentreport/crstatus/summarize.golines 52-58. A small exported helper, for examplecrstatus.PromoteLifecycle(current, incoming string) string, would hold the rule in one place. Named constants for"informing"and"blocking"would also remove the repeated string literals across both files.🤖 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 514 - 520, Extract the duplicated lifecycle promotion rule from the surrounding test-detail logic and SummarizeTestJobRuns into an exported crstatus.PromoteLifecycle(current, incoming string) helper. Update both call sites to use it, and introduce shared named constants for the informing and blocking lifecycle values instead of repeating string literals.pkg/api/componentreadiness/dataprovider/postgres/provider.go (1)
662-693: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared variant filter and key builder.
Lines 662-693 duplicate lines 409-440 of
queryTestDetails: the samerequestedVariantsByTestIDmap build, the same requested-variant match loop, and the samefilterByDBGroupByplusKeyWithVariantsconstruction. A helper that takes the variant map,dbGroupBy, and the row's test ID and returns the encoded key plus a match flag would remove the duplication and keep both paths aligned when filtering rules change.🤖 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 662 - 693, Extract the shared requested-variant filtering and key construction from queryTestDetails and the current result-building loop into a helper. Have it accept the variant map, dbGroupBy, and row test ID, return the encoded crtest.KeyWithVariants plus a match flag, and reuse it in both paths while preserving existing filtering behavior.
🤖 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.
Nitpick comments:
In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go`:
- Around line 662-693: Extract the shared requested-variant filtering and key
construction from queryTestDetails and the current result-building loop into a
helper. Have it accept the variant map, dbGroupBy, and row test ID, return the
encoded crtest.KeyWithVariants plus a match flag, and reuse it in both paths
while preserving existing filtering behavior.
In `@pkg/api/componentreadiness/test_details.go`:
- Around line 514-520: Extract the duplicated lifecycle promotion rule from the
surrounding test-detail logic and SummarizeTestJobRuns into an exported
crstatus.PromoteLifecycle(current, incoming string) helper. Update both call
sites to use it, and introduce shared named constants for the informing and
blocking lifecycle values instead of repeating string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f3f1248-01a6-420f-a402-9390775dc6b1
📒 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
|
Scheduling required tests: |
|
/lgtm |
8aff205 to
66462d9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/component_readiness_test.go (2)
1609-1614: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the prefix-sum date comment with the value used.
The comment states the base window is
[2024-05-15, 2024-06-01)and thatlookupEnd = 2024-06-01. The earlier seed helper at lines 228-232 documents the opposite convention:lookupEnd = end - 1. Both conventions now appear in the same file, so a reader cannot tell which one the query implements.State why this suite uses
2024-06-01directly, or reference the query behavior that makes both correct.🤖 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 1609 - 1614, Clarify the comments surrounding baseLookupStart and baseLookupEnd in the prefix-sum setup to explain why this suite passes 2024-06-01 directly as lookupEnd, or reference the query behavior that makes this inclusive value correct. Keep the existing date values and cumulative-summary calls unchanged.
1598-1647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated aggregate-fallback fixture setup.
Most subtests in
TestQueryBaseJobRunTestStatus_AggregateFallbackrepeat the same block: one variant combination, one prow job, one test, one suite, one ownership, twocreateCumulativeSummarycalls with the same prefix sums, and the sameoptsconstruction withRequestedVariantsandIncludeVariantsset to aws/ovn. The new report tests at lines 3211-3359 repeat the same aggregate base seed as well.Extract a small helper such as
seedAggregateFallback(t, dbc, release, jobName, testName, suiteName, uniqueID)that returns the created rows, plus an options helper for the aws/ovn request. This reduces the risk that one subtest drifts from the others and keeps the assertions visible.As per coding guidelines: "Prefer table-driven Go tests with descriptive case names, and search the same package for existing test patterns before adding new tests."
Also applies to: 1904-1940, 2020-2068, 2070-2116
🤖 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 1598 - 1647, Extract the repeated aggregate-fallback fixture creation from TestQueryBaseJobRunTestStatus_AggregateFallback into a helper such as seedAggregateFallback, parameterized by release, job name, test name, suite name, and ownership ID, returning the created entities needed by assertions. Add a focused options helper for the shared aws/ovn RequestedVariants and IncludeVariants setup, and update the affected subtests and aggregate report tests to reuse both helpers while keeping assertions and descriptive table-driven cases intact.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/integration/component_readiness_test.go`:
- Around line 1609-1614: Clarify the comments surrounding baseLookupStart and
baseLookupEnd in the prefix-sum setup to explain why this suite passes
2024-06-01 directly as lookupEnd, or reference the query behavior that makes
this inclusive value correct. Keep the existing date values and
cumulative-summary calls unchanged.
- Around line 1598-1647: Extract the repeated aggregate-fallback fixture
creation from TestQueryBaseJobRunTestStatus_AggregateFallback into a helper such
as seedAggregateFallback, parameterized by release, job name, test name, suite
name, and ownership ID, returning the created entities needed by assertions. Add
a focused options helper for the shared aws/ovn RequestedVariants and
IncludeVariants setup, and update the affected subtests and aggregate report
tests to reuse both helpers while keeping assertions and descriptive
table-driven cases intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a137991-e7e2-4536-89a6-dda6bc7269ad
📒 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
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/api/componentreadiness/dataprovider/interface.go
- pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go
- pkg/api/componentreadiness/dataprovider/mixed/provider.go
- pkg/api/componentreadiness/dataprovider/bigquery/provider.go
- pkg/api/componentreadiness/component_report_test.go
- pkg/apis/api/componentreport/crstatus/summarize_test.go
- pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go
- pkg/api/componentreadiness/test_details.go
- pkg/apis/api/componentreport/crstatus/types.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
66462d9 to
50bdbaf
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
50bdbaf to
5140c74
Compare
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw 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 |
3 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