Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/api/componentreadiness/triage.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,12 @@ func DeleteTriage(dbc *gorm.DB, id int) error {
return nil
}

// ListRegressions lists all regressions for the provided view OR release.
// ListRegressions lists all regressions for the provided view OR release, optionally filtered by test name.
// When view is set, it is resolved to that view's sample release and filtering is by release.
func ListRegressions(dbc *db.DB, release string, views []crview.View, releases []v1.Release, crTimeRoundingFactor, crTimeRoundingOffset time.Duration, req *http.Request) ([]models.TestRegression, error) {
func ListRegressions(dbc *db.DB, release, testName string, views []crview.View, releases []v1.Release, crTimeRoundingFactor, crTimeRoundingOffset time.Duration, req *http.Request) ([]models.TestRegression, error) {
var regressions []models.TestRegression
var err error
regressions, err = query.ListRegressions(dbc, release)
regressions, err = query.ListRegressions(dbc, release, testName)
if err != nil {
return nil, err
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not see this reused in this manner. We would be better off with a GetRegressionForTest function that was only (currently) used for this purpose.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a dedicated GetRegressionsForTest function in both triage_queries.go and triage.go, and reverted ListRegressions back to its original signature. The handler in server.go now dispatches to GetRegressionsForTest when the test parameter is provided, and to ListRegressions otherwise.

Expand Down
6 changes: 5 additions & 1 deletion pkg/db/query/triage_queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,18 @@ func CountRegressionFailuresAfter(dbc *db.DB, regressionID uint, after time.Time
return int(count), nil
}

func ListRegressions(dbc *db.DB, release string) ([]models.TestRegression, error) {
func ListRegressions(dbc *db.DB, release, testName string) ([]models.TestRegression, error) {
var regressions []models.TestRegression
query := dbc.DB.Model(&models.TestRegression{}).Preload("Triages").Preload("JobRuns").Preload("Views")

if release != "" {
query = query.Where("test_regressions.release = ?", release)
}

if testName != "" {
query = query.Where("test_regressions.test_name = ?", testName)
}

res := query.Find(&regressions)
if res.Error != nil {
log.WithError(res.Error).Error("error listing regressions")
Expand Down
7 changes: 4 additions & 3 deletions pkg/sippyserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1930,7 +1930,7 @@ func (s *Server) jsonTriagePotentialMatchingRegressions(w http.ResponseWriter, r
failureResponse(w, http.StatusInternalServerError, err.Error())
return
}
regressions, err := componentreadiness.ListRegressions(s.db, view.SampleRelease.Name, s.views.ComponentReadiness, allReleases, s.crTimeRoundingFactor, s.crTimeRoundingOffset, req)
regressions, err := componentreadiness.ListRegressions(s.db, view.SampleRelease.Name, "", s.views.ComponentReadiness, allReleases, s.crTimeRoundingFactor, s.crTimeRoundingOffset, req)
if err != nil {
failureResponse(w, http.StatusInternalServerError, err.Error())
return
Expand Down Expand Up @@ -1980,6 +1980,7 @@ func (s *Server) jsonGetRegressions(w http.ResponseWriter, req *http.Request) {
// Read query parameters for listing
view := param.SafeRead(req, "view")
release := param.SafeRead(req, "release")
testName := param.SafeRead(req, "test_name")

// Error if both view and release are specified
if view != "" && release != "" {
Expand All @@ -2003,7 +2004,7 @@ func (s *Server) jsonGetRegressions(w http.ResponseWriter, req *http.Request) {
}
}

regressions, err := componentreadiness.ListRegressions(s.db, release, views, allReleases, s.crTimeRoundingFactor, s.crTimeRoundingOffset, req)
regressions, err := componentreadiness.ListRegressions(s.db, release, testName, views, allReleases, s.crTimeRoundingFactor, s.crTimeRoundingOffset, req)
if err != nil {
failureResponse(w, http.StatusInternalServerError, err.Error())
return
Expand Down Expand Up @@ -2788,7 +2789,7 @@ func (s *Server) Serve() {
},
{
EndpointPath: "/api/component_readiness/regressions",
Description: "List component readiness test regressions. Supports view OR release query parameters (not both).",
Description: "List component readiness test regressions. Supports view OR release query parameters (not both). Optional test_name parameter filters by exact test name.",
Capabilities: []string{LocalDBCapability, ComponentReadinessCapability},
HandlerFunc: s.jsonGetRegressions,
},
Expand Down
1 change: 1 addition & 0 deletions pkg/util/param/param.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ var paramRegexp = map[string]*regexp.Regexp{
"job": nameRegexp,
"job_name": nameRegexp,
"test": regexp.MustCompile(`^.+$`), // tests can be anything, so always parameterize in sql
"test_name": regexp.MustCompile(`^.+$`), // test names can be anything, always parameterize in sql

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use "test" here for this? how is that any different? what would "test" be if not a test name?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Switched to using the existing test parameter instead of adding test_name. Removed the test_name entry from param.go.

"test_id": regexp.MustCompile(`^[\w:. -]+$`), // test IDs like "openshift-tests-upgrade:af8a62c596e5c2b5448a5d308f4989a6" or "cluster install:0cb1bb27e418491b1ffdacab58c5c8c0"
"prow_job_run_id": uintRegexp,
"prow_job_run_ids": regexp.MustCompile(`^\d+(,\d+)*$`), // comma-separated integers
Expand Down
12 changes: 12 additions & 0 deletions sippy-ng/src/tests/TestAnalysis.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import React, { Fragment, useEffect } from 'react'
import SimpleBreadcrumbs from '../components/SimpleBreadcrumbs'
import SummaryCard from '../components/SummaryCard'
import TestPassRateCharts from './TestPassRateCharts'
import TestRegressionsTable from './TestRegressionsTable'
import TestTable from './TestTable'

export function TestAnalysis(props) {
Expand Down Expand Up @@ -406,6 +407,17 @@ export function TestAnalysis(props) {
testName={testName}
/>
</Box>
<Typography variant="h6" sx={{ marginTop: 3 }}>
Active Regressions
<Tooltip title="Active regressions detected by Component Readiness for this test. Honors variant filters applied above.">
<InfoIcon />
</Tooltip>
</Typography>
<TestRegressionsTable
release={props.release}
testName={testName}
filterModel={filterModel}
/>
</Card>
</Grid>

Expand Down
184 changes: 184 additions & 0 deletions sippy-ng/src/tests/TestRegressionsTable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
import {
Chip,
Paper,
Table,
TableBody,
TableCell,
TableContainer,
TableHead,
TableRow,
Tooltip,
Typography,
} from '@mui/material'
import { Link } from 'react-router-dom'
import { relativeTime, safeEncodeURIComponent } from '../helpers'
import Alert from '@mui/material/Alert'
import PropTypes from 'prop-types'
import React, { useEffect, useMemo } from 'react'

export default function TestRegressionsTable({
release,
testName,
filterModel,
}) {
const [isLoaded, setLoaded] = React.useState(false)
const [regressions, setRegressions] = React.useState([])
const [fetchError, setFetchError] = React.useState('')

useEffect(() => {
if (!testName || !release) return

Comment thread
coderabbitai[bot] marked this conversation as resolved.
const url = `${
process.env.REACT_APP_API_URL
}/api/component_readiness/regressions?release=${safeEncodeURIComponent(
release
)}&test_name=${safeEncodeURIComponent(testName)}`

fetch(url)
.then((res) => {
if (res.status !== 200) {
throw new Error('server returned ' + res.status)
}
return res.json()
})
.then((data) => {
setRegressions(data || [])
setLoaded(true)
})
.catch((error) => {
setFetchError('Could not retrieve regression data: ' + error)
setLoaded(true)
})
}, [release, testName])

const variantFilters = useMemo(() => {
if (!filterModel || !filterModel.items) return []
return filterModel.items.filter((f) => f.columnField === 'variants')
}, [filterModel])

const filteredRegressions = useMemo(() => {
let filtered = regressions.filter((r) => !r.closed || !r.closed.Valid)

if (variantFilters.length === 0) return filtered

return filtered.filter((regression) => {
const variantValues = (regression.variants || []).map((v) => {
const parts = v.split(':')
return parts.length > 1 ? parts.slice(1).join(':') : v
})

@coderabbitai coderabbitai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use parseVariantName instead of manual string splitting.

Both locations manually split variant strings to strip prefixes, which duplicates logic and violates path instructions. As per path instructions, when dealing with variant strings that may include a prefix, use parseVariantName(variantName) from sippy-ng/src/helpers.js to split them into { name, variant }.

  • sippy-ng/src/tests/TestRegressionsTable.js#L64-L67: Replace manual splitting with parseVariantName(v).variant. Remember to add parseVariantName to the ../helpers import on line 13.
  • sippy-ng/src/tests/TestRegressionsTable.js#L104-L108: Apply the same parseVariantName(v).variant replacement here.
♻️ Proposed refactor for both sites

Update the import at the top of the file:

-import { relativeTime, safeEncodeURIComponent } from '../helpers'
+import { relativeTime, safeEncodeURIComponent, parseVariantName } from '../helpers'

Update the first site (lines 64-67):

-      const variantValues = (regression.variants || []).map((v) => {
-        const parts = v.split(':')
-        return parts.length > 1 ? parts.slice(1).join(':') : v
-      })
+      const variantValues = (regression.variants || []).map(
+        (v) => parseVariantName(v).variant
+      )

Update the second site (lines 104-108):

-            const variantValues = (regression.variants || [])
-              .map((v) => {
-                const parts = v.split(':')
-                return parts.length > 1 ? parts.slice(1).join(':') : v
-              })
+            const variantValues = (regression.variants || [])
+              .map((v) => parseVariantName(v).variant)
📝 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.

Suggested change
const variantValues = (regression.variants || []).map((v) => {
const parts = v.split(':')
return parts.length > 1 ? parts.slice(1).join(':') : v
})
import { relativeTime, safeEncodeURIComponent, parseVariantName } from '../helpers'
...
- const variantValues = (regression.variants || []).map((v) => {
- const parts = v.split(':')
- return parts.length > 1 ? parts.slice(1).join(':') : v
const variantValues = (regression.variants || []).map(
(v) => parseVariantName(v).variant
)
...
- const variantValues = (regression.variants || [])
- .map((v) => {
- const parts = v.split(':')
- return parts.length > 1 ? parts.slice(1).join(':') : v
const variantValues = (regression.variants || [])
.map((v) => parseVariantName(v).variant)
📍 Affects 1 file
  • sippy-ng/src/tests/TestRegressionsTable.js#L64-L67 (this comment)
  • sippy-ng/src/tests/TestRegressionsTable.js#L104-L108
🤖 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 `@sippy-ng/src/tests/TestRegressionsTable.js` around lines 64 - 67, Replace the
manual variant-string splitting in both locations,
sippy-ng/src/tests/TestRegressionsTable.js lines 64-67 and 104-108, with
parseVariantName(v).variant. Add parseVariantName to the ../helpers import so
both sites use the shared parsing logic.

Source: Path instructions

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 53f4c81. Replaced manual string splitting with parseVariantName(v).name at both locations and added the import. Note: used .name (the value after the colon) rather than .variant (the prefix before the colon) since the existing code was extracting the value part, not the prefix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped: comment is from another GitHub bot.


return variantFilters.every((filter) => {
const hasMatch = variantValues.some(
(v) => v.toLowerCase() === filter.value.toLowerCase()
)
if (filter.not) {
return !hasMatch
}
return hasMatch
})
})
}, [regressions, variantFilters])

if (!isLoaded) {
return <p>Loading...</p>
}
if (fetchError !== '') {
return <Alert severity="error">{fetchError}</Alert>
}
if (filteredRegressions.length === 0) {
return <Typography>No active regressions found</Typography>
}

return (
<TableContainer component={Paper} style={{ marginTop: 20 }}>
<Table size="small" aria-label="regressions-table">
<TableHead>
<TableRow>
<TableCell>ID</TableCell>
<TableCell>Variants</TableCell>
<TableCell>Regressed Since</TableCell>
<TableCell>Last Failure</TableCell>
<TableCell>Triage</TableCell>
</TableRow>
</TableHead>
<TableBody>
{filteredRegressions.map((regression) => {
const variantValues = (regression.variants || [])
.map((v) => {
const parts = v.split(':')
return parts.length > 1 ? parts.slice(1).join(':') : v
})
.filter((val) => !['default', 'none', 'unknown'].includes(val))

const tooltipLines = (regression.variants || []).sort().join('\n')

const triaged = regression.triages && regression.triages.length > 0

return (
<TableRow key={regression.id}>
<TableCell>
<Link
to={`/sippy-ng/component_readiness/regressions/${regression.id}`}
>
{regression.id}
</Link>
</TableCell>
<TableCell>
<Tooltip
title={
<span style={{ whiteSpace: 'pre-line' }}>
{tooltipLines}
</span>
}
>
<span>{variantValues.join(', ')}</span>
</Tooltip>
</TableCell>
<TableCell>
{regression.opened
? relativeTime(new Date(regression.opened), new Date())
: ''}
</TableCell>
<TableCell>
{regression.last_failure && regression.last_failure.Valid
? relativeTime(
new Date(regression.last_failure.Time),
new Date()
)
: ''}
</TableCell>
<TableCell>
{triaged ? (
regression.triages.map((t) => (
<Chip
key={t.id}
label={
t.url ? t.url.split('/').pop() : `Triage #${t.id}`
}
component={Link}
to={`/sippy-ng/component_readiness/triages/${t.id}`}
clickable
size="small"
color="primary"
variant="outlined"
sx={{ marginRight: 0.5 }}
/>
))
) : (
<Chip label="Untriaged" size="small" color="warning" />
)}
</TableCell>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple Triages can exist for a single regression. I don't think we should surface triages here at all as it just makes things too complex.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Removed the Triage column entirely from the table.

</TableRow>
)
})}
</TableBody>
</Table>
</TableContainer>
)
}

TestRegressionsTable.propTypes = {
release: PropTypes.string.isRequired,
testName: PropTypes.string.isRequired,
filterModel: PropTypes.object,
}