Add lifecycle filter to the Tests page - #3870
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. |
|
Warning Review limit reached
Next review available in: 27 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 (14)
WalkthroughChangesLifecycle filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestTable
participant TestsAPI
participant PostgresQueries
participant BigQueryGenerator
TestTable->>TestsAPI: Submit lifecycle filter
TestsAPI->>PostgresQueries: Apply lifecycle predicate
PostgresQueries-->>TestsAPI: Return filtered report
TestsAPI->>BigQueryGenerator: Receive lifecycle filter
BigQueryGenerator-->>TestsAPI: Return HTTP 400 validation error
Possibly related PRs
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 |
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: 1
🧹 Nitpick comments (1)
test/integration/tests_report_test.go (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid appending twice to
testMetadataColumns.Lines 57 and 59 both append to
testMetadataColumns. This is safe only because the slice literal hascap == len, so eachappendallocates a new backing array. IftestMetadataColumnsis later built with spare capacity, the secondappendoverwrites the element thatcollapsedColumnsholds, and the collapsed query silently selects the wrong column.Build each slice independently.
♻️ Proposed change
testMetadataColumns := []string{"suite_name", "name", "jira_component", "jira_component_id"} - collapsedColumns := append(testMetadataColumns, query.QueryTestFields) + collapsedColumns := slices.Concat(testMetadataColumns, []string{query.QueryTestFields}) rawQuery := dbc.DB.Table("(?) AS r", collapsedQuery).Select(strings.Join(collapsedColumns, ",")) - selectColumns := append(testMetadataColumns, query.QueryTestSummarizer) + selectColumns := slices.Concat(testMetadataColumns, []string{query.QueryTestSummarizer})Add the
slicesimport.🤖 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/tests_report_test.go` around lines 56 - 61, Build collapsedColumns and selectColumns independently from testMetadataColumns in the test query setup, using the slices package as suggested to avoid shared backing-array mutations when appending query.QueryTestFields and query.QueryTestSummarizer.
🤖 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/README.md`:
- Around line 469-475: The `/api/tests` response documentation still describes a
bare array; update the endpoint response contract to return an object containing
the test results and a `links` object with at least the filtered `self` URL.
Adjust the frontend client and API tests consuming `/api/tests` to use the new
representation, and update the README example/schema accordingly while
preserving lifecycle filtering behavior.
---
Nitpick comments:
In `@test/integration/tests_report_test.go`:
- Around line 56-61: Build collapsedColumns and selectColumns independently from
testMetadataColumns in the test query setup, using the slices package as
suggested to avoid shared backing-array mutations when appending
query.QueryTestFields and query.QueryTestSummarizer.
🪄 Autofix
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: aa46c563-6f5e-49e9-87b3-b5f18de052bf
📒 Files selected for processing (14)
pkg/api/README.mdpkg/api/api.gopkg/api/tests.gopkg/api/tests_test.gopkg/db/query/cumulative_query.gopkg/db/query/cumulative_query_test.gopkg/db/query/test_queries.gopkg/filter/filterable.gosippy-ng/src/datagrid/GridToolbarFilterItem.jsxsippy-ng/src/datagrid/GridToolbarFilterItem.test.jsxsippy-ng/src/tests/TestTable.jsxtest/integration/component_readiness_test.gotest/integration/tests_report_test.gotest/integration/util/fixtures.go
Builds on the lifecycle-as-a-dimension work in test_cumulative_summaries to expose lifecycle (blocking/informing) as a filter on the Tests page, both collapsed and per-variant views. Changes: - pkg/db/query: lifecycleWhereClause applies equals/not-equals before aggregation in TestReportQueryCollapsed and UncollapsedTestReportWithStats (re-applied inside the stats CTE too, so cross-variant averages don't blend in excluded-lifecycle data). Honors the filter's LinkOperator so an OR-linked filter selecting both lifecycles returns their union instead of an unsatisfiable AND. - pkg/filter: new ErrUnsupportedOperator sentinel so unsupported operators surface as a 400 instead of being silently ignored; lives in pkg/filter (not pkg/api) to avoid an import cycle with pkg/db/query - pkg/api: split lifecycle out of the Postgres filter pipeline; BigQuery-backed /api/tests/v2 returns 400 for a lifecycle filter, since the underlying junit_7day_comparison/junit_2day_comparison tables have no lifecycle column - sippy-ng: GridToolbarFilterItem gains generic "values"-restricted dropdown support (fixed choices, equals/!= only) used by the new Lifecycle column in TestTable.jsx - test/integration: new tests_report_test.go covering both query paths end-to-end (period math, name/variant/lifecycle filters including the OR-linked case, cross-variant stats, never-stable handling, open bugs, jira component resolution); reusable fixtures added to util/fixtures.go, and component_readiness_test.go's near-duplicate private helpers now delegate to those shared fixtures instead of re-implementing them make integration: 204/204 pass. make lint: clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aa01fb0 to
70e4e06
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
ready-for-human-review
|
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. |
|
Can we get a display marker that indicates the test is informing (or contains some informing results collapsed into the one row). This will be helpful in a number of views. A column with an icon if informing would be nice but may be too much of a waste of space. Collapsing into the test name perhaps with an icon at the start? |
|
/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
lifecycleWhereClauseapplies equals/not-equals before aggregation, honoringLinkOperatorso OR-linked filters return a union instead of an unsatisfiable AND./api/tests/v2returns HTTP 400 for lifecycle filters (tables lack the column).GridToolbarFilterItemgains generic values-restricted dropdown support (fixed choices, equals/!= only).tests_report_test.gowith end-to-end coverage for both query paths; shared fixtures extracted toutil/fixtures.go.Staging verification
Tested against
sippy-staging(buildsippy-staging-148, release 4.22).Release 4.22 has both lifecycle values: 177M blocking rows, 1M informing rows.
Collapsed path (
/api/tests?collapse=true)lifecycle = blockinglifecycle = informinglifecycle != blockinglifecycle != informinglifecycle = blocking OR lifecycle = informinglifecycle = informing AND name contains 'network'contains)Uncollapsed path (
/api/tests?collapse=false)Scoped to
name contains 'NetworkSegmentation'to keep result sets manageable.lifecycle = blockinglifecycle = informinglifecycle != blockinglifecycle != informingBigQuery path (
/api/tests/v2)lifecycle = blockingObservations
blocking OR informing) returns the same count as unfiltered, confirming the LinkOperator fix works correctly.Test plan
make lintcleanmake testpasses (unit + Vitest)make integrationpasses (204/204)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes