From bc991a8aea6bde2d5479d25c6be9e52a155bc1d5 Mon Sep 17 00:00:00 2001 From: Stephen Goeddel Date: Tue, 5 May 2026 14:20:47 -0400 Subject: [PATCH 1/3] TRT-2633: Link symptoms to triage records with summaries and filtering Surfaces job run symptoms on triage detail pages by syncing them into a triage_symptoms junction table during regression cache loading, then exposing per-symptom summaries (with regression IDs and job run counts) via the expand=symptoms query parameter. Adds frontend symptom chips on regression rows and a filterable symptom summary panel. Includes unit and e2e tests for symptom syncing, API expansion, and cascade deletion. --- docs/plans/trt-2633-triage-symptoms.md | 573 ++++++++++++++++++ .../dataprovider/bigquery/querygenerators.go | 9 +- .../componentreadiness/regressiontracker.go | 66 ++ .../regressiontracker_test.go | 86 +++ pkg/api/componentreadiness/test_details.go | 1 + pkg/api/componentreadiness/triage.go | 81 +++ .../api/componentreport/crstatus/types.go | 1 + .../api/componentreport/testdetails/types.go | 1 + .../regressioncacheloader.go | 17 +- pkg/db/db.go | 1 + pkg/db/models/triage.go | 14 + pkg/sippyserver/server.go | 65 +- .../src/component_readiness/CompReadyUtils.js | 21 + sippy-ng/src/component_readiness/Triage.js | 41 +- .../src/component_readiness/TriageSymptoms.js | 129 ++++ .../TriagedRegressionTestList.js | 80 ++- .../regressiontracker_test.go | 324 ++++++++++ .../triage/triageapi_test.go | 155 +++++ 18 files changed, 1623 insertions(+), 42 deletions(-) create mode 100644 docs/plans/trt-2633-triage-symptoms.md create mode 100644 sippy-ng/src/component_readiness/TriageSymptoms.js diff --git a/docs/plans/trt-2633-triage-symptoms.md b/docs/plans/trt-2633-triage-symptoms.md new file mode 100644 index 000000000..9f04f3d68 --- /dev/null +++ b/docs/plans/trt-2633-triage-symptoms.md @@ -0,0 +1,573 @@ +# TRT-2633: Support Linking Symptoms/Labels to Triage Records + +**Date:** 2026-04-30 +**JIRA:** [TRT-2633](https://redhat.atlassian.net/browse/TRT-2633) +**Author:** Stephen Goeddel + +## Problem Statement + +Triage can get messy in cases where a generic test is failing for lots of +different reasons. It's often hard to spot what's really going on across a set +of regressions tied to a single triage. + +Symptom/label usage is increasing as a way to automatically detect problems +hidden deep in job logs and artifacts. Tying this data into triage records would +let engineers see at a glance which symptoms are present across all regressions +in a triage — and what percentage of those regressions exhibit each one. + +## Current Architecture + +### Symptom Pipeline + +1. **Symptom definitions** live in PostgreSQL (`job_run_symptoms` table). + Each has an `ID` (string), `Summary`, `MatcherType`, `FilePattern`, + `MatchString`, and `LabelIDs` (labels to apply on match). + +2. **Symptom detection** runs against job run artifacts. When a symptom + matches, it creates entries in BigQuery's `job_labels` table with the + `symptom_id` field populated, linking the label application back to the + triggering symptom. + +3. **Regression tracking** queries BigQuery for test details, joining + `job_labels` to aggregate labels per job run: + ```sql + SELECT prowjob_build_id, + STRING_AGG(DISTINCT label, ',' ORDER BY label) AS job_labels + FROM .job_labels ... + ``` + These label IDs flow through the pipeline: + `TestJobRunRows.JobLabels` → `JobRunStats.JobLabels` → `RegressionJobRun.JobLabels` + +4. **Missing link**: Symptom IDs (`job_labels.symptom_id`) are not aggregated + or carried through the pipeline. Only label IDs are preserved. There is no + association between symptoms and triage records. + +### Triage Architecture + +- A `Triage` record has many `TestRegression` records (via `triage_regressions` join table). +- Each `TestRegression` has many `RegressionJobRun` records (via FK). +- Each `RegressionJobRun` has `JobLabels pq.StringArray` with label IDs from failed runs. +- The regression cache loader (`regressioncacheloader.go`) orchestrates regression + tracking and job run syncing in a single pass per view. + +## Proposed Solution + +### Two Core Changes + +1. **Carry symptom IDs through the pipeline**: Extend the BigQuery query to + also aggregate `symptom_id` from `job_labels`, and carry `JobSymptoms` + alongside `JobLabels` through `TestJobRunRows` → `JobRunStats` → + `RegressionJobRun`. + +2. **New `triage_symptoms` junction table**: A many-to-many association between + triages and symptoms. During regression tracking, after job runs are synced, + collect symptom IDs from job runs of regressions that belong to triages and + upsert the associations. + +### Why a Persistent Junction Table + +- Provides fast triage detail queries without joining through + regressions → job runs → symptom IDs → symptoms at request time. +- Creates a stable record of which symptoms were observed, even after + regression job runs age out or the regression closes. +- Enables future features like notifications when new symptoms appear on a triage. +- Satisfies AC #1 (a many-to-many association must exist between triage records and symptoms in the database). + +### Percentage Computation + +For AC #5 (show the percentage of regressions exhibiting each symptom on the triage details page), the percentage is computed at +API time via a `COUNT(DISTINCT regression_id)` query against the +`triage_symptoms` junction table, grouped by `symptom_id`. This is fast (single +indexed query) and avoids walking the full regression → job run → symptom graph. + +The junction table also stores a per-regression job run count (`job_run_count` column), +which is summed across regressions to produce `job_run_count` in the summary. + +Junction rows are upserted and counts updated in place on each loader run. +Since job runs only accumulate, counts grow monotonically and can't +double-count. + +## Implementation Plan + +### Phase 1: Carry Symptom IDs Through the Data Pipeline + +#### BigQuery query + +**File:** `pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go` + +Extend the `job_labels` aggregation subquery to also aggregate symptom IDs: + +```go +jobLabelsJoin := fmt.Sprintf(`LEFT JOIN ( + SELECT prowjob_build_id, + STRING_AGG(DISTINCT label, ',' ORDER BY label) AS job_labels, + STRING_AGG(DISTINCT CASE WHEN symptom_id != '' THEN symptom_id END, + ',' ORDER BY symptom_id) AS job_symptoms + FROM %s.job_labels + WHERE prowjob_start >= DATETIME(@From) + AND prowjob_start < DATETIME(@To) + GROUP BY prowjob_build_id +) agg_labels ON junit.prowjob_build_id = agg_labels.prowjob_build_id +`, client.Dataset) +``` + +Add `ANY_VALUE(agg_labels.job_symptoms) AS job_symptoms` to the SELECT list +alongside the existing `job_labels` column. + +#### Data structure changes + +**File:** `pkg/apis/api/componentreport/crstatus/types.go` +```go +type TestJobRunRows struct { + // ... existing fields ... + JobLabels []string `bigquery:"-" json:"job_labels,omitempty"` + JobSymptoms []string `bigquery:"-" json:"job_symptoms,omitempty"` // NEW + TestFailures int `bigquery:"-" json:"test_failures"` +} +``` + +**File:** `pkg/apis/api/componentreport/testdetails/types.go` +```go +type JobRunStats struct { + // ... existing fields ... + JobLabels []string `json:"job_labels,omitempty"` + JobSymptoms []string `json:"job_symptoms,omitempty"` // NEW + TestFailures int `json:"test_failures"` +} +``` + +**File:** `pkg/db/models/triage.go` +```go +type RegressionJobRun struct { + // ... existing fields ... + JobLabels pq.StringArray `json:"job_labels,omitempty" gorm:"column:job_labels;type:text[]"` + JobSymptoms pq.StringArray `json:"job_symptoms,omitempty" gorm:"column:job_symptoms;type:text[]"` // NEW +} +``` + +#### Deserialization + +**File:** `pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go` + +In `deserializeRowToJobRunTestReportStatus()`, add handling alongside the +existing `job_labels` case: +```go +case col == "job_symptoms": + if row[i] != nil { + cts.JobSymptoms = strings.Split(row[i].(string), ",") + } +``` + +#### Conversion through the pipeline + +**File:** `pkg/api/componentreadiness/test_details.go` — `getJobRunStats()`: +```go +JobSymptoms: stats.JobSymptoms, +``` + +**File:** `pkg/api/componentreadiness/regressiontracker.go` — `FailedJobRunsFromTestDetails()`: +```go +jobRun := models.RegressionJobRun{ + // ... existing fields ... + JobLabels: pq.StringArray(run.JobLabels), + JobSymptoms: pq.StringArray(run.JobSymptoms), // NEW +} +``` + +#### Update MergeJobRuns to refresh symptom data + +**File:** `pkg/api/componentreadiness/regressiontracker.go` + +`MergeJobRuns` currently uses `FirstOrCreate` which won't update existing +records. Extend it to update `JobSymptoms` (and `JobLabels`) on existing +records so that newly-detected symptoms are captured on subsequent loader runs: + +```go +func (prs *PostgresRegressionStore) MergeJobRuns(regressionID uint, jobRuns []models.RegressionJobRun) error { + for i := range jobRuns { + jobRuns[i].RegressionID = regressionID + res := prs.dbc.DB. + Where("regression_id = ? AND prow_job_run_id = ?", regressionID, jobRuns[i].ProwJobRunID). + Assign(models.RegressionJobRun{ + JobLabels: jobRuns[i].JobLabels, + JobSymptoms: jobRuns[i].JobSymptoms, + }). + FirstOrCreate(&jobRuns[i]) + if res.Error != nil { + return fmt.Errorf("error merging job run %s for regression %d: %w", + jobRuns[i].ProwJobRunID, regressionID, res.Error) + } + } + return nil +} +``` + +`Assign` tells GORM to always update the specified fields — whether the record +is found or created. This ensures that if new symptoms appear on a job run that +was already processed, they are captured on subsequent loader runs. The BigQuery +query returns the full set of symptoms each time (via `STRING_AGG(DISTINCT ...)`), +so the stored value is replaced entirely, not appended to. + +### Phase 2: Database Schema — Junction Table + +#### New model + +**File:** `pkg/db/models/triage.go` + +```go +type TriageSymptom struct { + TriageID uint `json:"triage_id" gorm:"primaryKey;column:triage_id"` + SymptomID string `json:"symptom_id" gorm:"primaryKey;column:symptom_id"` + RegressionID uint `json:"regression_id" gorm:"primaryKey;column:regression_id"` + JobRunCount int `json:"job_run_count" gorm:"column:job_run_count;not null;default:0"` +} +``` + +The composite key `(triage_id, symptom_id, regression_id)` records exactly which +regression(s) surfaced each symptom on a given triage. `JobRunCount` stores how +many failed job runs on that regression exhibited the symptom. + +Rows are upserted during the regression cache loader run, matching the +`FirstOrCreate`-then-update pattern used by `MergeJobRuns`. Since job runs only +accumulate (never deleted), symptom counts are monotonically increasing and +can be safely updated in place without a full replace. + +#### Add association to Triage model + +Since the junction table now includes `regression_id`, GORM's built-in +`many2many` tag no longer fits (it expects a two-column join). Instead, model +`TriageSymptom` as a standalone entity and query it directly. + +```go +type Triage struct { + // ... existing fields ... + TriageSymptoms []TriageSymptom `json:"triage_symptoms,omitempty" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` +} +``` + +The `OnDelete:CASCADE` GORM constraint ensures that deleting a triage +automatically removes its `TriageSymptom` junction rows. This follows the same +pattern used by `TestRegression.JobRuns` and `TestRegression.Views`. + +#### Auto-migration + +**File:** `pkg/db/db.go` + +Add `TriageSymptom` to the auto-migrate list (this creates the +`triage_symptoms` table). + +### Phase 3: Automatic Linking During Regression Tracking + +#### New interface method + +**File:** `pkg/api/componentreadiness/regressiontracker.go` + +```go +type RegressionStore interface { + // ... existing methods ... + // SyncTriageSymptoms upserts symptom associations for all triages + // that have regressions with the given symptom-bearing job runs. + SyncTriageSymptoms(regressions []*models.TestRegression) error +} +``` + +#### Implementation + +```go +func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.TestRegression) error { + regIDs := make([]uint, len(regressions)) + for i, r := range regressions { + regIDs[i] = r.ID + } + + var regs []models.TestRegression + res := prs.dbc.DB. + Preload("Triages"). + Preload("JobRuns"). + Where("id IN ?", regIDs). + Find(®s) + if res.Error != nil { + return fmt.Errorf("error loading regressions for symptom sync: %w", res.Error) + } + + for _, reg := range regs { + if len(reg.Triages) == 0 { + continue + } + // Count job runs per symptom for this regression + symptomCounts := map[string]int{} + for _, jr := range reg.JobRuns { + seen := sets.New[string]() + for _, s := range jr.JobSymptoms { + if s != "" && !seen.Has(s) { + seen.Insert(s) + symptomCounts[s]++ + } + } + } + // Upsert one junction row per (triage, symptom, regression) + for symptomID, count := range symptomCounts { + for _, triage := range reg.Triages { + ts := models.TriageSymptom{ + TriageID: triage.ID, + SymptomID: symptomID, + RegressionID: reg.ID, + } + result := prs.dbc.DB.Where(ts).FirstOrCreate(&ts) + if result.Error != nil { + return fmt.Errorf("error syncing symptom %s to triage %d regression %d: %w", + symptomID, triage.ID, reg.ID, result.Error) + } + // Update count whether newly created or already existed + if err := prs.dbc.DB.Model(&ts).Update("job_run_count", count).Error; err != nil { + return fmt.Errorf("error updating symptom count: %w", err) + } + } + } + } + return nil +} +``` + +#### Integration in cache loader + +**File:** `pkg/dataloader/regressioncacheloader/regressioncacheloader.go` + +After the per-release regression closing loop (around line 180), add a global +symptom sync step: + +```go +// SyncTriageSymptoms runs unconditionally — it is additive and idempotent, +// so partial errors in individual views should not block symptom linking. +var allActiveRegs []*models.TestRegression +for _, result := range releaseResults { + for _, id := range result.activeIDs.UnsortedList() { + allActiveRegs = append(allActiveRegs, &models.TestRegression{ID: id}) + } +} +if len(allActiveRegs) > 0 { + if err := l.regressionStore.SyncTriageSymptoms(allActiveRegs); err != nil { + l.logger.WithError(err).Error("error syncing triage symptoms") + l.errs = append(l.errs, err) + } +} +``` + +This runs once per loader execution, after all views are processed, ensuring +symptoms from all views are linked. It is not gated behind `!anyErrors` because +the operation is additive (only upserts) and idempotent — partial view errors +should not block symptom linking for regressions that were successfully processed. + +### Phase 4: API Changes + +#### New response type for symptom summaries + +**File:** `pkg/api/componentreadiness/triage.go` + +```go +type TriageSymptomSummary struct { + Symptom jobrunscan.Symptom `json:"symptom"` + RegressionCount int `json:"regression_count"` + TotalCount int `json:"total_count"` + Percentage float64 `json:"percentage"` + JobRunCount int `json:"job_run_count"` + RegressionIDs []uint `json:"regression_ids"` +} +``` + +`RegressionIDs` lists which regressions on this triage exhibit each symptom. +The frontend uses this to build a per-regression symptom map and to filter +the regression table by symptom — without walking job runs client-side. + +`GetTriageSymptomSummaries(dbc, triageID, totalRegressions)` queries the +`triage_symptoms` junction table with `COUNT(DISTINCT regression_id)` and +`SUM(job_run_count)` grouped by `symptom_id`, then loads symptom definitions +from `job_run_symptoms`, and finally queries the raw junction rows to build +the per-symptom `RegressionIDs` slice. Symptom IDs not found in +`job_run_symptoms` are silently skipped (handles stale/test data). + +#### Extend ExpandedTriage response + +**File:** `pkg/sippyserver/server.go` + +```go +type ExpandedTriage struct { + *models.Triage + RegressedTests map[string][]*componentreport.ReportTestSummary `json:"regressed_tests"` + SymptomSummaries []componentreadiness.TriageSymptomSummary `json:"symptom_summaries,omitempty"` +} +``` + +#### Comma-separated `expand` parameter + +The `jsonGetTriageByID` handler parses `expand` as a comma-separated set of +field names (e.g., `?expand=regressions,symptoms`). Each field is handled +independently: + +- `symptoms` — computes symptom summaries via `GetTriageSymptomSummaries`. + This is cheap (single indexed query against the junction table). +- `regressions` — looks up regressed tests from the component report cache. + This is more expensive, so it is only computed when explicitly requested. + +Symptoms are also computed when `regressions` is requested (since the triage +detail page needs both), but `expand=symptoms` alone returns summaries +without the regressed test lookups. + +### Phase 5: Frontend Changes + +#### Triage Details Page Layout + +**File:** `sippy-ng/src/component_readiness/Triage.js` + +The current page layout is: +1. Header with action buttons (Ask Sippy, Update, Delete, etc.) +2. Metadata table (Resolved, Description, Type, Jira fields, etc.) +3. "Included Tests" section with the `TriagedRegressionTestList` DataGrid + +The triage detail fetch uses `?expand=regressions,symptoms` to request both +regressed test lookups and symptom summaries in a single call. + +Add a new **"Symptoms"** section between the metadata table and the +"Included Tests" section. This is the natural placement because symptoms +provide context for interpreting the regressions below. + +**Symptoms section structure:** + +- Section header: **"Symptoms"** with a count badge (e.g., "Symptoms (3)") +- If `symptom_summaries` is empty or absent, show a muted "No symptoms + detected" message — don't hide the section entirely, so users know it exists. +- If populated, render a MUI `Table` (not DataGrid — the list will be small + enough that pagination and filtering aren't needed) with these columns: + +| Column | Source field | Display | +|--------|-------------|---------| +| Symptom | `symptom.summary` | Text, clickable link to the symptom detail page | +| Regressions | `regression_count` / `total_count` | e.g., "2 / 5", with a filter button (see below) | +| Percentage | `percentage` | MUI `LinearProgress` bar with percentage label | +| Failed Runs | `job_run_count` | Plain number — total failed job runs across regressions exhibiting this symptom | + +- Table is sorted by percentage descending (already sorted by the API). +- Each symptom row's summary text should link to the Job Artifact Query page + prefilled with that symptom, following the existing `JAQPrefilled` pattern + from `JobArtifactQuery.js`. + +#### Filtering Regressions by Symptom + +The Regressions column shows how many of the triage's regressions exhibit each +symptom (e.g., "2 / 5"). Next to the count, add a small MUI `IconButton` with +a filter icon (`FilterList`). Clicking it filters the "Included Tests" +`TriagedRegressionTestList` DataGrid below to show only regressions that have +that symptom. + +**Implementation:** + +- `Triage.js` holds a `symptomFilter` state (symptom ID or `null`). +- Clicking the filter button sets `symptomFilter` to that symptom's ID. +- When a filter is active, show a `Chip` above the regressions table indicating + the active filter (e.g., "Filtered by: ") with a clear (X) + button that resets `symptomFilter` to `null`. +- `TriagedRegressionTestList` receives `symptomFilter` and `symptomSummaries` + props. When `symptomFilter` is set, it filters rows to only regressions + whose IDs appear in the matching symptom summary's `regression_ids` array. + +#### Per-Regression Symptom Indicators (AC #6 — each regression row indicates which triage-associated symptoms it exhibits) + +**File:** `sippy-ng/src/component_readiness/TriagedRegressionTestList.js` + +A **"Symptoms"** column (flex: 10) appears after the Variants column, only +when `symptomSummaries` is provided. For each regression row, the component +builds a `regressionSymptomMap` from the `symptomSummaries` prop — iterating +each summary's `regression_ids` to map regression IDs to their symptom IDs. +Each matching symptom is rendered as a small MUI `Chip` with a truncated label +(first ~12 chars) and a `Tooltip` showing the full summary. Chips are colored +using a deterministic hash of the symptom ID for visual distinction. + +## Test Plan + +### Unit Tests: Pipeline Changes + +**File:** `pkg/api/componentreadiness/regressiontracker_test.go` + +Added to existing `TestFailedJobRunsFromTestDetails` table-driven tests: + +| Test Case | Input | Expected | +|-----------|-------|----------| +| preserves JobSymptoms | Report with `JobSymptoms: ["SymA","SymB"]` | `runs[0].JobSymptoms` matches | +| empty JobSymptoms results in nil | Report with no symptoms | `runs[0].JobSymptoms` is nil | +| mixed runs: only symptomatic run carries symptoms | One run with `["SymA"]`, one without | Only first run has symptoms, second is nil | + +### E2E Tests: MergeJobRuns Symptom Behavior + +**File:** `test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go` + +Added to existing `Test_RegressionJobRuns`: + +| Subtest | Setup | Assert | +|---------|-------|--------| +| new job run with symptoms | Merge run with `JobSymptoms: ["SymA"]` | Stored run has `JobSymptoms` = `["SymA"]` | +| existing job run gains symptoms on re-merge | First merge with no symptoms, second with `["SymA"]` | Stored run updated to `["SymA"]` | + +### E2E Tests: SyncTriageSymptoms + +**File:** `test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go` + +New top-level `Test_SyncTriageSymptoms` function: + +| Subtest | Setup | Assert | +|---------|-------|--------| +| links symptoms to triage | Regression + triage + job run with `["SymA","SymB"]`, call `SyncTriageSymptoms` | 2 junction rows, correct `regression_id` and `job_run_count` | +| idempotent | Call `SyncTriageSymptoms` twice with same data | Same row count, same `job_run_count` | +| count accuracy | 3 job runs: 2 with SymA, 1 without | `job_run_count` = 2 for SymA | +| count grows with new job runs | After first sync, merge additional run with SymA, re-sync | `job_run_count` increments | +| regression without triage is skipped | Regression with symptoms but no triage | Zero `triage_symptoms` rows | +| multiple symptoms per run | Job run with `["SymA","SymB"]` | Both symptoms get junction rows | + +### E2E Tests: Symptom Summaries in API Response + +**File:** `test/e2e/componentreadiness/triage/triageapi_test.go` + +Added to existing `Test_TriageAPI`: + +| Subtest | Setup | Assert | +|---------|-------|--------| +| expanded triage includes symptom summaries | Regression + triage + job runs with symptoms, `SyncTriageSymptoms`, GET `?expand=regressions,symptoms` | `symptom_summaries` non-empty, correct `regression_ids`, `regression_count`, `job_run_count` | +| expand=symptoms only returns symptoms without regressed_tests | Same setup, GET `?expand=symptoms` | `symptom_summaries` present, `regressed_tests` nil | +| delete triage cascades to triage_symptoms | Create triage + symptoms, DELETE triage | Zero junction rows for that triage | + +These tests seed `job_run_symptoms` (Symptom) records for the test symptom IDs +so `GetTriageSymptomSummaries` can resolve them via the join. + +## Cascade and Deletion Behavior + +| Action | Result | +|--------|--------| +| Delete triage | Junction rows in `triage_symptoms` removed. Symptoms unaffected. | +| Soft-delete symptom | Junction rows persist. Symptom filtered from preloaded queries (GORM soft delete). | +| Hard-delete symptom | Junction rows removed (DB-level FK cascade or explicit cleanup). | +| Delete regression from triage | Junction rows for that triage/regression pair should be removed (FK cascade or explicit cleanup). | +| Close regression | No effect on junction. Symptoms remain as historical record. | + +## Migration and Backward Compatibility + +1. **Zero impact on existing data.** The new `job_symptoms` column on + `regression_job_runs` is added via GORM AutoMigrate with NULL default. + Existing rows remain unchanged. + +2. **No retroactive backfill.** Per AC #8 (no retroactive linking of symptoms to existing triages), existing triages will not have + symptoms linked until the next regression cache loader run processes their + regressions. Symptoms accumulate naturally going forward. + +3. **BigQuery compatibility.** The `symptom_id` column already exists in the + `job_labels` BigQuery table. The query change only adds an aggregation of + this existing column. + +4. **API backward compatibility.** The `symptom_summaries` field is additive + on the `ExpandedTriage` response. Clients that don't use it are unaffected. + The `symptoms` field on `Triage` is also additive. + +## Out of Scope (Future Work) + +- Manual association of symptoms to triages via the UI. +- Retroactive linking of symptoms to existing/historical triages. +- Display of symptom data on the Test Details page. +- Compare Sample Failures integration with symptom filtering. diff --git a/pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go b/pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go index 530f2b16f..bbb988c55 100644 --- a/pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go +++ b/pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go @@ -563,7 +563,9 @@ func buildTestDetailsQuery( withClause, commonParams := buildCRQueryCTEs(client.Dataset, junitTable, jobNameQueryPortion, jobRunAnnotationToIgnore, c.AdvancedOption.KeyTestNames) jobLabelsJoin := fmt.Sprintf(`LEFT JOIN ( - SELECT prowjob_build_id, STRING_AGG(DISTINCT label, ',' ORDER BY label) AS job_labels + SELECT prowjob_build_id, + STRING_AGG(DISTINCT label, ',' ORDER BY label) AS job_labels, + STRING_AGG(DISTINCT CASE WHEN symptom_id != '' THEN symptom_id END, ',') AS job_symptoms FROM %s.job_labels WHERE prowjob_start >= DATETIME(@From) AND prowjob_start < DATETIME(@To) @@ -597,6 +599,7 @@ func buildTestDetailsQuery( SUM(adjusted_success_val) AS success_count, SUM(adjusted_flake_count) AS flake_count, ANY_VALUE(agg_labels.job_labels) AS job_labels, + ANY_VALUE(agg_labels.job_symptoms) AS job_symptoms, ANY_VALUE(agg_failures.job_run_test_failure_count) AS job_run_test_failure_count, COALESCE(NULLIF(ANY_VALUE(lifecycle), ''), 'blocking') AS lifecycle, FROM deduped_testcases junit @@ -1094,6 +1097,10 @@ func deserializeRowToJobRunTestReportStatus(row []bigquery.Value, schema bigquer if row[i] != nil { cts.JobLabels = strings.Split(row[i].(string), ",") } + case col == "job_symptoms": + if row[i] != nil { + cts.JobSymptoms = strings.Split(row[i].(string), ",") + } case col == "job_run_test_failure_count": if row[i] != nil { cts.TestFailures = int(row[i].(int64)) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 9a72b8222..bc61e2bcd 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -16,6 +16,7 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" log "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/util/sets" ) const ( @@ -41,6 +42,8 @@ type RegressionStore interface { UpsertRegressionView(regressionID uint, viewName string) error // DeactivateRolledOffViews sets active=false on regression_views rows for regressions that have rolled off a view. DeactivateRolledOffViews(regressionIDs []uint, activeViewMap map[uint][]string) error + // SyncTriageSymptoms upserts symptom associations for triages based on regression job run data. + SyncTriageSymptoms(regressions []*models.TestRegression) error } type PostgresRegressionStore struct { @@ -109,6 +112,10 @@ func (prs *PostgresRegressionStore) MergeJobRuns(regressionID uint, jobRuns []mo jobRuns[i].RegressionID = regressionID res := prs.dbc.DB. Where("regression_id = ? AND prow_job_run_id = ?", regressionID, jobRuns[i].ProwJobRunID). + Assign(models.RegressionJobRun{ + JobLabels: jobRuns[i].JobLabels, + JobSymptoms: jobRuns[i].JobSymptoms, + }). FirstOrCreate(&jobRuns[i]) if res.Error != nil { return fmt.Errorf("error merging job run %s for regression %d: %w", @@ -118,6 +125,64 @@ func (prs *PostgresRegressionStore) MergeJobRuns(regressionID uint, jobRuns []mo return nil } +// SyncTriageSymptoms upserts triage_symptoms junction rows by doing a full recount of +// symptoms across each regression's job runs. The resulting job_run_count is replaced +// (not incremented), making the operation idempotent and safe to call on every loader run. +func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.TestRegression) error { + if len(regressions) == 0 { + return nil + } + + regIDs := make([]uint, len(regressions)) + for i, r := range regressions { + regIDs[i] = r.ID + } + + var regs []models.TestRegression + res := prs.dbc.DB. + Preload("Triages"). + Preload("JobRuns"). + Where("id IN ?", regIDs). + Find(®s) + if res.Error != nil { + return fmt.Errorf("error loading regressions for symptom sync: %w", res.Error) + } + + for _, reg := range regs { + if len(reg.Triages) == 0 { + continue + } + symptomCounts := map[string]int{} + for _, jr := range reg.JobRuns { + seen := sets.New[string]() + for _, symptom := range jr.JobSymptoms { + if symptom != "" && !seen.Has(symptom) { + seen.Insert(symptom) + symptomCounts[symptom]++ + } + } + } + for symptomID, count := range symptomCounts { + for _, triage := range reg.Triages { + ts := models.TriageSymptom{ + TriageID: triage.ID, + SymptomID: symptomID, + RegressionID: reg.ID, + } + result := prs.dbc.DB.Where(ts).FirstOrCreate(&ts) + if result.Error != nil { + return fmt.Errorf("error syncing symptom %s to triage %d regression %d: %w", + symptomID, triage.ID, reg.ID, result.Error) + } + if err := prs.dbc.DB.Model(&ts).Update("job_run_count", count).Error; err != nil { + return fmt.Errorf("error updating symptom job run count: %w", err) + } + } + } + } + return nil +} + func (prs *PostgresRegressionStore) UpsertRegressionView(regressionID uint, viewName string) error { res := prs.dbc.DB.Exec( `INSERT INTO regression_views (test_regression_id, view_name, active, opened_at) @@ -359,6 +424,7 @@ func FailedJobRunsFromTestDetails(report testdetails.Report) []models.Regression StartTime: run.StartTime.In(time.UTC), TestFailures: run.TestFailures, JobLabels: pq.StringArray(run.JobLabels), + JobSymptoms: pq.StringArray(run.JobSymptoms), } jobRuns = append(jobRuns, jobRun) } diff --git a/pkg/api/componentreadiness/regressiontracker_test.go b/pkg/api/componentreadiness/regressiontracker_test.go index d833771fd..46f3cc1c1 100644 --- a/pkg/api/componentreadiness/regressiontracker_test.go +++ b/pkg/api/componentreadiness/regressiontracker_test.go @@ -135,6 +135,92 @@ func TestFailedJobRunsFromTestDetails(t *testing.T) { }, expectedCount: 0, }, + { + name: "preserves JobSymptoms", + report: testdetails.Report{ + Analyses: []testdetails.Analysis{ + { + JobStats: []testdetails.JobStats{ + { + SampleJobName: "job-a", + SampleJobRunStats: []testdetails.JobRunStats{ + { + JobRunID: "run-1", + StartTime: startTime1, + TestStats: crtest.Stats{FailureCount: 1}, + JobSymptoms: []string{"SymA", "SymB"}, + }, + }, + }, + }, + }, + }, + }, + expectedCount: 1, + expectedRunIDs: []string{"run-1"}, + checkFunc: func(t *testing.T, runs []models.RegressionJobRun) { + assert.Equal(t, []string{"SymA", "SymB"}, []string(runs[0].JobSymptoms)) + }, + }, + { + name: "empty JobSymptoms results in nil", + report: testdetails.Report{ + Analyses: []testdetails.Analysis{ + { + JobStats: []testdetails.JobStats{ + { + SampleJobName: "job-a", + SampleJobRunStats: []testdetails.JobRunStats{ + { + JobRunID: "run-1", + StartTime: startTime1, + TestStats: crtest.Stats{FailureCount: 1}, + }, + }, + }, + }, + }, + }, + }, + expectedCount: 1, + expectedRunIDs: []string{"run-1"}, + checkFunc: func(t *testing.T, runs []models.RegressionJobRun) { + assert.Nil(t, runs[0].JobSymptoms) + }, + }, + { + name: "mixed runs: only symptomatic run carries symptoms", + report: testdetails.Report{ + Analyses: []testdetails.Analysis{ + { + JobStats: []testdetails.JobStats{ + { + SampleJobName: "job-a", + SampleJobRunStats: []testdetails.JobRunStats{ + { + JobRunID: "run-1", + StartTime: startTime1, + TestStats: crtest.Stats{FailureCount: 1}, + JobSymptoms: []string{"SymA"}, + }, + { + JobRunID: "run-2", + StartTime: startTime2, + TestStats: crtest.Stats{FailureCount: 1}, + }, + }, + }, + }, + }, + }, + }, + expectedCount: 2, + expectedRunIDs: []string{"run-1", "run-2"}, + checkFunc: func(t *testing.T, runs []models.RegressionJobRun) { + assert.Equal(t, []string{"SymA"}, []string(runs[0].JobSymptoms)) + assert.Nil(t, runs[1].JobSymptoms) + }, + }, } for _, tt := range tests { diff --git a/pkg/api/componentreadiness/test_details.go b/pkg/api/componentreadiness/test_details.go index 4dd7b767e..61f17285d 100644 --- a/pkg/api/componentreadiness/test_details.go +++ b/pkg/api/componentreadiness/test_details.go @@ -567,6 +567,7 @@ func (c *ComponentReportGenerator) getJobRunStats(stats crstatus.TestJobRunRows) JobRunID: stats.ProwJobRunID, StartTime: stats.StartTime, JobLabels: stats.JobLabels, + JobSymptoms: stats.JobSymptoms, TestFailures: stats.TestFailures, } return jobRunStats diff --git a/pkg/api/componentreadiness/triage.go b/pkg/api/componentreadiness/triage.go index c95116f13..451a07c9d 100644 --- a/pkg/api/componentreadiness/triage.go +++ b/pkg/api/componentreadiness/triage.go @@ -10,6 +10,7 @@ import ( "net/url" "path" "slices" + "sort" "strings" "time" @@ -21,6 +22,7 @@ import ( v1 "github.com/openshift/sippy/pkg/apis/sippy/v1" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" + "github.com/openshift/sippy/pkg/db/models/jobrunscan" "github.com/openshift/sippy/pkg/db/query" log "github.com/sirupsen/logrus" "gorm.io/gorm" @@ -860,6 +862,85 @@ func generateTestDetailsURLFromRegression(regression *models.TestRegression, vie ) } +// TriageSymptomSummary represents a symptom found across a triage's regressions, +// with counts and percentages for the triage detail view. +type TriageSymptomSummary struct { + Symptom jobrunscan.Symptom `json:"symptom"` + RegressionCount int `json:"regression_count"` + TotalCount int `json:"total_count"` + Percentage float64 `json:"percentage"` + JobRunCount int `json:"job_run_count"` + RegressionIDs []uint `json:"regression_ids"` +} + +// GetTriageSymptomSummaries queries the triage_symptoms junction table to build +// per-symptom summaries for a triage detail response. +func GetTriageSymptomSummaries(dbc *db.DB, triageID uint, totalRegressions int) ([]TriageSymptomSummary, error) { + if totalRegressions == 0 { + return nil, nil + } + + type symptomCount struct { + SymptomID string `gorm:"column:symptom_id"` + RegressionCount int `gorm:"column:regression_count"` + JobRunCount int `gorm:"column:job_run_count"` + } + var counts []symptomCount + if err := dbc.DB.Model(&models.TriageSymptom{}). + Select("symptom_id, COUNT(DISTINCT regression_id) AS regression_count, SUM(job_run_count) AS job_run_count"). + Where("triage_id = ?", triageID). + Group("symptom_id"). + Order("regression_count DESC"). + Scan(&counts).Error; err != nil { + return nil, fmt.Errorf("error querying triage symptom counts: %w", err) + } + if len(counts) == 0 { + return nil, nil + } + + symptomIDs := make([]string, len(counts)) + for i, c := range counts { + symptomIDs[i] = c.SymptomID + } + var symptoms []jobrunscan.Symptom + if err := dbc.DB.Where("id IN ?", symptomIDs).Find(&symptoms).Error; err != nil { + return nil, fmt.Errorf("error loading symptoms: %w", err) + } + symptomMap := make(map[string]jobrunscan.Symptom, len(symptoms)) + for _, s := range symptoms { + symptomMap[s.ID] = s + } + + var tsRows []models.TriageSymptom + if err := dbc.DB.Where("triage_id = ?", triageID).Find(&tsRows).Error; err != nil { + return nil, fmt.Errorf("error loading triage symptom regressions: %w", err) + } + regIDsBySymptom := make(map[string][]uint) + for _, row := range tsRows { + regIDsBySymptom[row.SymptomID] = append(regIDsBySymptom[row.SymptomID], row.RegressionID) + } + + var summaries []TriageSymptomSummary + for _, c := range counts { + s, ok := symptomMap[c.SymptomID] + if !ok { + continue + } + summaries = append(summaries, TriageSymptomSummary{ + Symptom: s, + RegressionCount: c.RegressionCount, + TotalCount: totalRegressions, + Percentage: float64(c.RegressionCount) / float64(totalRegressions) * 100, + JobRunCount: c.JobRunCount, + RegressionIDs: regIDsBySymptom[c.SymptomID], + }) + } + sort.Slice(summaries, func(i, j int) bool { + return summaries[i].RegressionCount > summaries[j].RegressionCount + }) + return summaries, nil +} + // GetViewsForTriage returns the names of all active views associated with the triage's regressions. func GetViewsForTriage(triage *models.Triage) []string { if triage == nil { diff --git a/pkg/apis/api/componentreport/crstatus/types.go b/pkg/apis/api/componentreport/crstatus/types.go index 64fb48390..8831cd590 100644 --- a/pkg/apis/api/componentreport/crstatus/types.go +++ b/pkg/apis/api/componentreport/crstatus/types.go @@ -64,6 +64,7 @@ type TestJobRunRows struct { JiraComponent string `bigquery:"jira_component"` JiraComponentID *big.Rat `bigquery:"jira_component_id"` JobLabels []string `bigquery:"-" json:"job_labels,omitempty"` + JobSymptoms []string `bigquery:"-" json:"job_symptoms,omitempty"` TestFailures int `bigquery:"-" json:"test_failures"` Lifecycle string `bigquery:"lifecycle"` } diff --git a/pkg/apis/api/componentreport/testdetails/types.go b/pkg/apis/api/componentreport/testdetails/types.go index 1ea6ec2a5..d338194d5 100644 --- a/pkg/apis/api/componentreport/testdetails/types.go +++ b/pkg/apis/api/componentreport/testdetails/types.go @@ -114,5 +114,6 @@ type JobRunStats struct { // there are cases multiple junits are generated for the same test. TestStats crtest.Stats `json:"test_stats"` JobLabels []string `json:"job_labels,omitempty"` + JobSymptoms []string `json:"job_symptoms,omitempty"` TestFailures int `json:"test_failures"` } diff --git a/pkg/dataloader/regressioncacheloader/regressioncacheloader.go b/pkg/dataloader/regressioncacheloader/regressioncacheloader.go index 293405fe7..35d13b58f 100644 --- a/pkg/dataloader/regressioncacheloader/regressioncacheloader.go +++ b/pkg/dataloader/regressioncacheloader/regressioncacheloader.go @@ -156,8 +156,7 @@ func (l *RegressionCacheLoader) Load() { } } - // Close regressions per-release (only if no errors for that release), - // then resolve triages globally once all releases are processed. + // Close regressions per-release (only if no errors for that release) anyErrors := false for release, result := range releaseResults { if result.hadErrors { @@ -178,6 +177,20 @@ func (l *RegressionCacheLoader) Load() { } } + var allActiveRegs []*models.TestRegression + for _, result := range releaseResults { + for _, id := range result.activeIDs.UnsortedList() { + allActiveRegs = append(allActiveRegs, &models.TestRegression{ID: id}) + } + } + if len(allActiveRegs) > 0 { + l.logger.Infof("syncing triage symptoms for %d active regressions", len(allActiveRegs)) + if err := l.regressionStore.SyncTriageSymptoms(allActiveRegs); err != nil { + l.logger.WithError(err).Error("error syncing triage symptoms") + l.errs = append(l.errs, err) + } + } + // ResolveTriages is a global operation (not per-release), so we only run it // once after all releases have been processed, and only if no releases had errors. if !anyErrors { diff --git a/pkg/db/db.go b/pkg/db/db.go index f2b0b7bab..aa1cd6650 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -92,6 +92,7 @@ func (d *DB) UpdateSchema(reportEnd *time.Time) error { &models.RegressionJobRun{}, &models.RegressionView{}, &models.Triage{}, + &models.TriageSymptom{}, &models.AuditLog{}, &models.ChatRating{}, &models.ChatConversation{}, diff --git a/pkg/db/models/triage.go b/pkg/db/models/triage.go index 3881d6842..0fd8a6be8 100644 --- a/pkg/db/models/triage.go +++ b/pkg/db/models/triage.go @@ -48,6 +48,9 @@ type Triage struct { // If we could establish this, it may mean less data duplication. Regressions []TestRegression `json:"regressions" gorm:"constraint:OnDelete:CASCADE;many2many:triage_regressions;"` + // TriageSymptoms links symptoms discovered in regression job runs to this triage. + TriageSymptoms []TriageSymptom `json:"triage_symptoms,omitempty" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` + // Resolution is an important field presently set by a user indicating a claimed time this issue was resolved, // and thus all associated regressions should be fixed. // Setting this will immediately change the regressions icon to one indicate the issue is believed to @@ -259,4 +262,15 @@ type RegressionJobRun struct { TestFailed bool `json:"test_failed" gorm:"column:test_failed"` TestFailures int `json:"test_failures" gorm:"column:test_failures"` JobLabels pq.StringArray `json:"job_labels,omitempty" gorm:"column:job_labels;type:text[]"` + JobSymptoms pq.StringArray `json:"job_symptoms,omitempty" gorm:"column:job_symptoms;type:text[]"` +} + +// TriageSymptom records that a specific symptom was found on a specific regression +// belonging to a triage. JobRunCount tracks how many failed job runs on that +// regression exhibited the symptom. +type TriageSymptom struct { + TriageID uint `json:"triage_id" gorm:"primaryKey;column:triage_id"` + SymptomID string `json:"symptom_id" gorm:"primaryKey;column:symptom_id"` + RegressionID uint `json:"regression_id" gorm:"primaryKey;column:regression_id"` + JobRunCount int `json:"job_run_count" gorm:"column:job_run_count;not null;default:0"` } diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index e2e9ce593..b6b24e802 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1667,21 +1667,21 @@ func (s *Server) jsonGetTriages(w http.ResponseWriter, req *http.Request) { } // ExpandedTriage allows for additional information to be included in the triage response. -// Currently, this is only the associated ReportTestSummaries which are useful for linking to the test_details report. type ExpandedTriage struct { *models.Triage - // RegressedTests is a mapping of the view to the regressed_tests found there - RegressedTests map[string][]*componentreport.ReportTestSummary `json:"regressed_tests"` + RegressedTests map[string][]*componentreport.ReportTestSummary `json:"regressed_tests"` + SymptomSummaries []componentreadiness.TriageSymptomSummary `json:"symptom_summaries,omitempty"` } func (s *Server) jsonGetTriageByID(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) idStr := vars["id"] - var expandRegressions bool - expand := req.URL.Query().Get("expand") - if expand == "regressions" { - expandRegressions = true + expandFields := make(map[string]bool) + for _, field := range strings.Split(req.URL.Query().Get("expand"), ",") { + if f := strings.TrimSpace(field); f != "" { + expandFields[f] = true + } } triageID, err := strconv.Atoi(idStr) @@ -1699,36 +1699,47 @@ func (s *Server) jsonGetTriageByID(w http.ResponseWriter, req *http.Request) { failureResponse(w, http.StatusNotFound, "triage not found") return } - if !expandRegressions { + + if len(expandFields) == 0 { api.RespondWithJSON(http.StatusOK, w, triage) return } et := ExpandedTriage{ - Triage: triage, - RegressedTests: make(map[string][]*componentreport.ReportTestSummary), + Triage: triage, } - views := componentreadiness.GetViewsForTriage(triage) - for _, view := range views { - // Set the view in the request so that we can obtain the component report to get the regressed test(s) for display - q := req.URL.Query() - q.Set("view", view) - req.URL.RawQuery = q.Encode() - componentReport, err := s.getComponentReportFromRequest(req) + if expandFields["symptoms"] { + symptomSummaries, err := componentreadiness.GetTriageSymptomSummaries(s.db, triage.ID, len(triage.Regressions)) if err != nil { - failureResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get component report: %v", err)) - return + log.WithError(err).Errorf("error getting triage symptom summaries for triage %d", triage.ID) + } else { + et.SymptomSummaries = symptomSummaries } - var regressedTests []*componentreport.ReportTestSummary - for _, regression := range triage.Regressions { - regressedTest := componentreadiness.GetMatchingRegressedTestForRegression(regression, componentReport) - if regressedTest != nil { - regressedTests = append(regressedTests, regressedTest) + } + + if expandFields["regressions"] { + et.RegressedTests = make(map[string][]*componentreport.ReportTestSummary) + views := componentreadiness.GetViewsForTriage(triage) + for _, view := range views { + q := req.URL.Query() + q.Set("view", view) + req.URL.RawQuery = q.Encode() + componentReport, err := s.getComponentReportFromRequest(req) + if err != nil { + failureResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get component report: %v", err)) + return + } + var regressedTests []*componentreport.ReportTestSummary + for _, regression := range triage.Regressions { + regressedTest := componentreadiness.GetMatchingRegressedTestForRegression(regression, componentReport) + if regressedTest != nil { + regressedTests = append(regressedTests, regressedTest) + } + } + if len(regressedTests) > 0 { + et.RegressedTests[view] = regressedTests } - } - if len(regressedTests) > 0 { - et.RegressedTests[view] = regressedTests } } diff --git a/sippy-ng/src/component_readiness/CompReadyUtils.js b/sippy-ng/src/component_readiness/CompReadyUtils.js index 5fc5d1878..88a5ee14f 100644 --- a/sippy-ng/src/component_readiness/CompReadyUtils.js +++ b/sippy-ng/src/component_readiness/CompReadyUtils.js @@ -898,6 +898,27 @@ export function generateTestDetailsReportLink( return generatedUrl } +const SYMPTOM_COLORS = [ + '#1976d2', + '#d32f2f', + '#388e3c', + '#f57c00', + '#7b1fa2', + '#0097a7', + '#c2185b', + '#455a64', + '#5d4037', + '#303f9f', +] + +export function symptomColor(symptomId) { + let hash = 0 + for (let i = 0; i < symptomId.length; i++) { + hash = (hash * 31 + symptomId.charCodeAt(i)) | 0 + } + return SYMPTOM_COLORS[Math.abs(hash) % SYMPTOM_COLORS.length] +} + // Helper function to check if triage has any regressions with status -1000 (failed fix). // allRegressedTests is a map of view name to array of regressed tests. export function hasFailedFixRegression(triage, allRegressedTests) { diff --git a/sippy-ng/src/component_readiness/Triage.js b/sippy-ng/src/component_readiness/Triage.js index cca99015c..a15727c9c 100644 --- a/sippy-ng/src/component_readiness/Triage.js +++ b/sippy-ng/src/component_readiness/Triage.js @@ -1,4 +1,4 @@ -import { Box, Button, Tooltip } from '@mui/material' +import { Box, Button, Chip, Tooltip } from '@mui/material' import { CheckCircle, Error as ErrorIcon } from '@mui/icons-material' import { CompReadyVarsContext } from './CompReadyVars' import { formatDateToSeconds, relativeTime } from '../helpers' @@ -16,7 +16,7 @@ import AskSippyButton from '../chat/AskSippyButton' import CompSeverityIcon from './CompSeverityIcon' import LaunderedLink from '../components/Laundry' import PropTypes from 'prop-types' -import React, { Fragment, useContext } from 'react' +import React, { Fragment, useContext, useState } from 'react' import Table from '@mui/material/Table' import TableBody from '@mui/material/TableBody' import TableCell from '@mui/material/TableCell' @@ -24,6 +24,7 @@ import TableRow from '@mui/material/TableRow' import TriageAuditLogsModal from './TriageAuditLogsModal' import TriagedRegressionTestList from './TriagedRegressionTestList' import TriagePotentialMatches from './TriagePotentialMatches' +import TriageSymptoms from './TriageSymptoms' import UpsertTriageModal from './UpsertTriageModal' export default function Triage({ id }) { @@ -34,6 +35,7 @@ export default function Triage({ id }) { const [triage, setTriage] = React.useState({}) const [message, setMessage] = React.useState('') const [isUpdated, setIsUpdated] = React.useState(false) + const [symptomFilter, setSymptomFilter] = useState(null) const capabilitiesContext = React.useContext(SippyCapabilitiesContext) const triageEnabled = capabilitiesContext.includes('write_endpoints') const localDBEnabled = capabilitiesContext.includes('local_db') @@ -51,14 +53,14 @@ export default function Triage({ id }) { let triageFetch // triage entries will only be available when there is a postgres connection if (localDBEnabled) { - triageFetch = fetch(`${getTriagesAPIUrl(id)}?expand=regressions`).then( - (response) => { - if (response.status !== 200) { - throw new Error('API server returned ' + response.status) - } - return response.json() + triageFetch = fetch( + `${getTriagesAPIUrl(id)}?expand=regressions,symptoms` + ).then((response) => { + if (response.status !== 200) { + throw new Error('API server returned ' + response.status) } - ) + return response.json() + }) } else { triageFetch = Promise.resolve({}) } @@ -353,11 +355,32 @@ export default function Triage({ id }) { +

Included Tests

+ {symptomFilter && ( + + ss.symptom.id === symptomFilter + )?.symptom.summary || symptomFilter + }`} + onDelete={() => setSymptomFilter(null)} + color="primary" + variant="outlined" + /> + + )} ) diff --git a/sippy-ng/src/component_readiness/TriageSymptoms.js b/sippy-ng/src/component_readiness/TriageSymptoms.js new file mode 100644 index 000000000..980861f18 --- /dev/null +++ b/sippy-ng/src/component_readiness/TriageSymptoms.js @@ -0,0 +1,129 @@ +import { + Box, + Chip, + IconButton, + LinearProgress, + Tooltip, + Typography, +} from '@mui/material' +import { FilterList } from '@mui/icons-material' +import { symptomColor } from './CompReadyUtils' +import PropTypes from 'prop-types' +import React from 'react' +import Table from '@mui/material/Table' +import TableBody from '@mui/material/TableBody' +import TableCell from '@mui/material/TableCell' +import TableHead from '@mui/material/TableHead' +import TableRow from '@mui/material/TableRow' + +export default function TriageSymptoms({ + symptomSummaries, + symptomFilter, + setSymptomFilter, +}) { + return ( + <> + +

+ Symptoms + {symptomSummaries?.length > 0 && ` (${symptomSummaries.length})`} +

+ + {!symptomSummaries || symptomSummaries.length === 0 ? ( + + No symptoms detected + + ) : ( + + + + + + Symptom + + + + + Regressions + + + + + Percentage + + + + + Failed Runs + + + + + + {symptomSummaries.map((ss) => ( + + + + + + + {ss.regression_count} + + + setSymptomFilter( + symptomFilter === ss.symptom.id + ? null + : ss.symptom.id + ) + } + color={ + symptomFilter === ss.symptom.id + ? 'primary' + : 'default' + } + > + + + + + + + + + + {ss.percentage.toFixed(1)}% + + + + {ss.job_run_count} + + ))} + +
+ )} + + ) +} + +TriageSymptoms.propTypes = { + symptomSummaries: PropTypes.array, + symptomFilter: PropTypes.string, + setSymptomFilter: PropTypes.func.isRequired, +} diff --git a/sippy-ng/src/component_readiness/TriagedRegressionTestList.js b/sippy-ng/src/component_readiness/TriagedRegressionTestList.js index 2e17453d3..60ae0d9d5 100644 --- a/sippy-ng/src/component_readiness/TriagedRegressionTestList.js +++ b/sippy-ng/src/component_readiness/TriagedRegressionTestList.js @@ -1,10 +1,11 @@ import { applyFilterModel, shouldKeepFilterItem } from '../datagrid/filterUtils' +import { Chip, Tooltip, Typography } from '@mui/material' import { CompReadyVarsContext } from './CompReadyVars' import { DataGrid } from '@mui/x-data-grid' import { generateTestDetailsReportLink } from './CompReadyUtils' import { NumberParam, useQueryParam } from 'use-query-params' import { relativeTime, SafeJSONParam } from '../helpers' -import { Tooltip, Typography } from '@mui/material' +import { symptomColor } from './CompReadyUtils' import CompSeverityIcon from './CompSeverityIcon' import GridToolbar from '../datagrid/GridToolbar' import PropTypes from 'prop-types' @@ -139,6 +140,52 @@ export default function TriagedRegressionTestList(props) { ), }, + ...(props.symptomSummaries + ? [ + { + field: 'symptoms', + headerName: 'Symptoms', + flex: 10, + filterable: false, + sortable: false, + valueGetter: (params) => { + const symptomIds = regressionSymptomMap[params.row.id] + return symptomIds ? [...symptomIds] : [] + }, + renderCell: (params) => { + if (!params.value || params.value.length === 0) return null + return ( +
+ {params.value.map((symptomId) => { + const summary = + props.symptomSummaries?.find( + (ss) => ss.symptom.id === symptomId + )?.symptom.summary || symptomId + const label = + summary.length > 12 + ? summary.substring(0, 12) + '…' + : summary + return ( + + + + ) + })} +
+ ) + }, + }, + ] + : []), { field: 'opened', headerName: 'Regressed Since', @@ -233,10 +280,35 @@ export default function TriagedRegressionTestList(props) { : []), ] + const regressionSymptomMap = React.useMemo(() => { + const map = {} + if (props.symptomSummaries) { + for (const ss of props.symptomSummaries) { + for (const regId of ss.regression_ids || []) { + if (!map[regId]) map[regId] = new Set() + map[regId].add(ss.symptom.id) + } + } + } + return map + }, [props.symptomSummaries]) + + const symptomFilteredRegressions = React.useMemo(() => { + if (!props.symptomFilter || !props.symptomSummaries) { + return triagedRegressions + } + const match = props.symptomSummaries.find( + (ss) => ss.symptom.id === props.symptomFilter + ) + if (!match) return triagedRegressions + const matchingRegIds = new Set((match.regression_ids || []).map(Number)) + return triagedRegressions.filter((r) => matchingRegIds.has(r.id)) + }, [triagedRegressions, props.symptomFilter, props.symptomSummaries]) + // Apply client-side filtering using shared utility const filteredRegressions = React.useMemo( - () => applyFilterModel(triagedRegressions, filterModel, columns), - [triagedRegressions, filterModel, columns] + () => applyFilterModel(symptomFilteredRegressions, filterModel, columns), + [symptomFilteredRegressions, filterModel, columns] ) return ( @@ -292,4 +364,6 @@ TriagedRegressionTestList.propTypes = { allRegressedTests: PropTypes.object, filterVals: PropTypes.string, showOnLoad: PropTypes.bool, + symptomFilter: PropTypes.string, + symptomSummaries: PropTypes.array, } diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 8298399f9..6f5a39709 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -19,6 +19,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/sets" ) func cleanupAllRegressions(dbc *db.DB) { @@ -395,6 +396,87 @@ func Test_RegressionJobRuns(t *testing.T) { assert.Len(t, stored, 3, "should have 3 unique job runs after dedup") }) + t.Run("new job run with symptoms", func(t *testing.T) { + defer cleanupAllRegressions(dbc) + defer cleanupJobRuns(dbc) + + reg, err := tracker.OpenRegression(view, componentreport.ReportTestSummary{ + TestComparison: testdetails.TestComparison{ + BaseStats: &testdetails.ReleaseStats{Release: "4.18"}, + }, + Identification: crtest.Identification{ + RowIdentification: crtest.RowIdentification{ + Component: "comp", + Capability: "cap", + TestName: "symptom test", + TestID: "symptomtestid", + }, + ColumnIdentification: crtest.ColumnIdentification{ + Variants: map[string]string{"a": "b"}, + }, + }, + }) + require.NoError(t, err) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + { + ProwJobRunID: "run-sym-1", + ProwJobName: "job-1", + TestFailed: true, + JobSymptoms: pq.StringArray{"SymA"}, + }, + }) + require.NoError(t, err) + + var stored []models.RegressionJobRun + res := dbc.DB.Where("regression_id = ? AND prow_job_run_id = ?", reg.ID, "run-sym-1").Find(&stored) + require.NoError(t, res.Error) + require.Len(t, stored, 1) + assert.Equal(t, []string{"SymA"}, []string(stored[0].JobSymptoms)) + }) + + t.Run("existing job run gains symptoms on re-merge", func(t *testing.T) { + defer cleanupAllRegressions(dbc) + defer cleanupJobRuns(dbc) + + reg, err := tracker.OpenRegression(view, componentreport.ReportTestSummary{ + TestComparison: testdetails.TestComparison{ + BaseStats: &testdetails.ReleaseStats{Release: "4.18"}, + }, + Identification: crtest.Identification{ + RowIdentification: crtest.RowIdentification{ + Component: "comp", + Capability: "cap", + TestName: "symptom update test", + TestID: "symptomuptestid", + }, + ColumnIdentification: crtest.ColumnIdentification{ + Variants: map[string]string{"a": "b"}, + }, + }, + }) + require.NoError(t, err) + + // First merge without symptoms + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-update-1", ProwJobName: "job-1", TestFailed: true}, + }) + require.NoError(t, err) + + var stored models.RegressionJobRun + dbc.DB.Where("regression_id = ? AND prow_job_run_id = ?", reg.ID, "run-update-1").First(&stored) + assert.Nil(t, stored.JobSymptoms) + + // Second merge with symptoms + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-update-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + }) + require.NoError(t, err) + + dbc.DB.Where("regression_id = ? AND prow_job_run_id = ?", reg.ID, "run-update-1").First(&stored) + assert.Equal(t, []string{"SymA"}, []string(stored.JobSymptoms)) + }) + t.Run("job runs deleted when regression is deleted", func(t *testing.T) { defer cleanupAllRegressions(dbc) defer cleanupJobRuns(dbc) @@ -433,6 +515,248 @@ func Test_RegressionJobRuns(t *testing.T) { }) } +func cleanupTriageSymptoms(dbc *db.DB) { + res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) + if res.Error != nil { + log.Errorf("error deleting triage symptoms: %v", res.Error) + } +} + +func cleanupTriages(dbc *db.DB) { + dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") + dbc.DB.Where("1 = 1").Delete(&models.Triage{}) +} + +func Test_SyncTriageSymptoms(t *testing.T) { + dbc := util.CreateE2EPostgresConnection(t) + tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) + view := crview.View{ + Name: "4.19-main", + SampleRelease: reqopts.RelativeRelease{ + Release: reqopts.Release{Name: "4.19"}, + }, + } + + newRegSummary := func(testID string) componentreport.ReportTestSummary { + return componentreport.ReportTestSummary{ + TestComparison: testdetails.TestComparison{ + BaseStats: &testdetails.ReleaseStats{Release: "4.18"}, + }, + Identification: crtest.Identification{ + RowIdentification: crtest.RowIdentification{ + Component: "comp", + Capability: "cap", + TestName: "sync symptom test " + testID, + TestID: testID, + }, + ColumnIdentification: crtest.ColumnIdentification{ + Variants: map[string]string{"a": "b"}, + }, + }, + } + } + + cleanup := func() { + cleanupTriageSymptoms(dbc) + cleanupJobRuns(dbc) + cleanupTriages(dbc) + cleanupAllRegressions(dbc) + } + + dbCtx := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + + t.Run("links symptoms to triage", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("link-sym-1")) + require.NoError(t, err) + + triage := models.Triage{ + URL: "https://issues.redhat.com/browse/TEST-SYM-1", + Description: "symptom link test", + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{*reg}, + } + require.NoError(t, dbCtx.Create(&triage).Error) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA", "SymB"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var rows []models.TriageSymptom + dbc.DB.Where("triage_id = ?", triage.ID).Find(&rows) + assert.Len(t, rows, 2, "should have 2 symptom rows") + symptomIDs := sets.New[string]() + for _, row := range rows { + symptomIDs.Insert(row.SymptomID) + assert.Equal(t, reg.ID, row.RegressionID) + assert.Equal(t, 1, row.JobRunCount) + } + assert.True(t, symptomIDs.Has("SymA")) + assert.True(t, symptomIDs.Has("SymB")) + }) + + t.Run("idempotent", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("idempotent-1")) + require.NoError(t, err) + + triage := models.Triage{ + URL: "https://issues.redhat.com/browse/TEST-SYM-2", + Description: "idempotent test", + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{*reg}, + } + require.NoError(t, dbCtx.Create(&triage).Error) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var count1 int64 + dbc.DB.Model(&models.TriageSymptom{}).Where("triage_id = ?", triage.ID).Count(&count1) + + // Second sync + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var count2 int64 + dbc.DB.Model(&models.TriageSymptom{}).Where("triage_id = ?", triage.ID).Count(&count2) + assert.Equal(t, count1, count2, "row count should not change on re-sync") + + var row models.TriageSymptom + dbc.DB.Where("triage_id = ? AND symptom_id = ?", triage.ID, "SymA").First(&row) + assert.Equal(t, 1, row.JobRunCount, "job_run_count should remain the same") + }) + + t.Run("count accuracy", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("count-acc-1")) + require.NoError(t, err) + + triage := models.Triage{ + URL: "https://issues.redhat.com/browse/TEST-SYM-3", + Description: "count accuracy test", + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{*reg}, + } + require.NoError(t, dbCtx.Create(&triage).Error) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + {ProwJobRunID: "run-2", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + {ProwJobRunID: "run-3", ProwJobName: "job-1", TestFailed: true}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var row models.TriageSymptom + res := dbc.DB.Where("triage_id = ? AND symptom_id = ?", triage.ID, "SymA").First(&row) + require.NoError(t, res.Error) + assert.Equal(t, 2, row.JobRunCount, "job_run_count should be 2 (only runs with SymA)") + }) + + t.Run("count grows with new job runs", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("count-grow-1")) + require.NoError(t, err) + + triage := models.Triage{ + URL: "https://issues.redhat.com/browse/TEST-SYM-4", + Description: "count grows test", + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{*reg}, + } + require.NoError(t, dbCtx.Create(&triage).Error) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var row models.TriageSymptom + dbc.DB.Where("triage_id = ? AND symptom_id = ?", triage.ID, "SymA").First(&row) + assert.Equal(t, 1, row.JobRunCount) + + // Add another job run with the same symptom + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-2", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + dbc.DB.Where("triage_id = ? AND symptom_id = ?", triage.ID, "SymA").First(&row) + assert.Equal(t, 2, row.JobRunCount, "job_run_count should increment after new run") + }) + + t.Run("regression without triage is skipped", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("no-triage-1")) + require.NoError(t, err) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var count int64 + dbc.DB.Model(&models.TriageSymptom{}).Where("regression_id = ?", reg.ID).Count(&count) + assert.Equal(t, int64(0), count, "no triage_symptoms rows should exist for untriaged regression") + }) + + t.Run("multiple symptoms per run", func(t *testing.T) { + defer cleanup() + + reg, err := tracker.OpenRegression(view, newRegSummary("multi-sym-1")) + require.NoError(t, err) + + triage := models.Triage{ + URL: "https://issues.redhat.com/browse/TEST-SYM-6", + Description: "multi symptom test", + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{*reg}, + } + require.NoError(t, dbCtx.Create(&triage).Error) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"SymA", "SymB"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var rows []models.TriageSymptom + dbc.DB.Where("triage_id = ?", triage.ID).Find(&rows) + assert.Len(t, rows, 2, "both symptoms should get junction rows") + for _, row := range rows { + assert.Equal(t, 1, row.JobRunCount, "each symptom seen once") + } + }) +} + func cleanupRegressionViews(dbc *db.DB) { res := dbc.DB.Where("1 = 1").Delete(&models.RegressionView{}) if res.Error != nil { diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index c8e8459d0..8428d1708 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -18,6 +18,7 @@ import ( "github.com/openshift/sippy/pkg/apis/api/componentreport/testdetails" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" + "github.com/openshift/sippy/pkg/db/models/jobrunscan" "github.com/openshift/sippy/pkg/sippyserver" "github.com/openshift/sippy/test/e2e/util" log "github.com/sirupsen/logrus" @@ -40,6 +41,27 @@ var view = crview.View{ }, } +func cleanupTriageSymptoms(dbc *db.DB) { + res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) + if res.Error != nil { + log.Errorf("error deleting triage symptoms: %v", res.Error) + } +} + +func seedSymptom(t *testing.T, gormDB *gorm.DB, id, summary string) *jobrunscan.Symptom { + sym := &jobrunscan.Symptom{ + SymptomContent: jobrunscan.SymptomContent{ + ID: id, + Summary: summary, + MatcherType: jobrunscan.MatcherTypeString, + MatchString: "e2e-test-match", + }, + } + res := gormDB.Create(sym) + require.NoError(t, res.Error) + return sym +} + func cleanupAllTriages(dbc *db.DB) { // Delete all triage and test regressions in the e2e postgres db. dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") @@ -392,6 +414,139 @@ func Test_TriageAPI(t *testing.T) { assertTriageDataMatches(t, originalTriage, oldTriageData, "OldData") }) + t.Run("expanded triage includes symptom summaries", func(t *testing.T) { + defer cleanupAllTriages(dbc) + defer cleanupTriageSymptoms(dbc) + + symA := seedSymptom(t, dbc.DB, "e2e-sym-a", "E2E Symptom A") + defer dbc.DB.Delete(symA) + symB := seedSymptom(t, dbc.DB, "e2e-sym-b", "E2E Symptom B") + defer dbc.DB.Delete(symB) + + reg := createTestRegression(t, tracker, view, "sym-expand-test-1") + defer dbc.DB.Delete(reg) + + triage := models.Triage{ + URL: jiraBug.URL, + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{ + {ID: reg.ID}, + }, + } + var triageResp models.Triage + err := util.SippyPost("/api/component_readiness/triages", &triage, &triageResp) + require.NoError(t, err) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "sym-run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"e2e-sym-a", "e2e-sym-b"}}, + {ProwJobRunID: "sym-run-2", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"e2e-sym-a"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var et sippyserver.ExpandedTriage + err = util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d?expand=regressions,symptoms", triageResp.ID), &et) + require.NoError(t, err) + require.NotNil(t, et.SymptomSummaries) + require.Len(t, et.SymptomSummaries, 2, "should have 2 symptom summaries") + + symMap := make(map[string]componentreadiness.TriageSymptomSummary) + for _, ss := range et.SymptomSummaries { + symMap[ss.Symptom.ID] = ss + } + require.Contains(t, symMap, "e2e-sym-a") + assert.Equal(t, 1, symMap["e2e-sym-a"].RegressionCount) + assert.Equal(t, 2, symMap["e2e-sym-a"].JobRunCount) + assert.Contains(t, symMap["e2e-sym-a"].RegressionIDs, reg.ID) + + require.Contains(t, symMap, "e2e-sym-b") + assert.Equal(t, 1, symMap["e2e-sym-b"].RegressionCount) + assert.Equal(t, 1, symMap["e2e-sym-b"].JobRunCount) + assert.Contains(t, symMap["e2e-sym-b"].RegressionIDs, reg.ID) + }) + + t.Run("expand=symptoms only returns symptoms without regressed_tests", func(t *testing.T) { + defer cleanupAllTriages(dbc) + defer cleanupTriageSymptoms(dbc) + + sym := seedSymptom(t, dbc.DB, "e2e-sym-only", "E2E Symptom Only") + defer dbc.DB.Delete(sym) + + reg := createTestRegression(t, tracker, view, "sym-only-test-1") + defer dbc.DB.Delete(reg) + + triage := models.Triage{ + URL: jiraBug.URL, + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{ + {ID: reg.ID}, + }, + } + var triageResp models.Triage + err := util.SippyPost("/api/component_readiness/triages", &triage, &triageResp) + require.NoError(t, err) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "sym-only-run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"e2e-sym-only"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + var et sippyserver.ExpandedTriage + err = util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d?expand=symptoms", triageResp.ID), &et) + require.NoError(t, err) + require.NotNil(t, et.SymptomSummaries) + assert.Len(t, et.SymptomSummaries, 1) + assert.Nil(t, et.RegressedTests, "regressed_tests should be nil when only symptoms is expanded") + }) + + t.Run("delete triage cascades to triage_symptoms", func(t *testing.T) { + defer cleanupAllTriages(dbc) + defer cleanupTriageSymptoms(dbc) + + sym := seedSymptom(t, dbc.DB, "e2e-sym-cascade", "E2E Symptom Cascade") + defer dbc.DB.Delete(sym) + + reg := createTestRegression(t, tracker, view, "sym-cascade-test-1") + defer dbc.DB.Delete(reg) + + triage := models.Triage{ + URL: jiraBug.URL, + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{ + {ID: reg.ID}, + }, + } + var triageResp models.Triage + err := util.SippyPost("/api/component_readiness/triages", &triage, &triageResp) + require.NoError(t, err) + + err = tracker.MergeJobRuns(reg.ID, []models.RegressionJobRun{ + {ProwJobRunID: "cascade-run-1", ProwJobName: "job-1", TestFailed: true, JobSymptoms: pq.StringArray{"e2e-sym-cascade"}}, + }) + require.NoError(t, err) + + err = tracker.SyncTriageSymptoms([]*models.TestRegression{{ID: reg.ID}}) + require.NoError(t, err) + + // Verify junction row exists + var count int64 + dbc.DB.Model(&models.TriageSymptom{}).Where("triage_id = ?", triageResp.ID).Count(&count) + require.Equal(t, int64(1), count, "should have 1 junction row before delete") + + // Delete via API + err = util.SippyDelete(fmt.Sprintf("/api/component_readiness/triages/%d", triageResp.ID)) + require.NoError(t, err) + + // Verify junction rows are gone + dbc.DB.Model(&models.TriageSymptom{}).Where("triage_id = ?", triageResp.ID).Count(&count) + assert.Equal(t, int64(0), count, "triage_symptoms should be cascade deleted with triage") + }) + t.Run("audit endpoint returns full lifecycle operations", func(t *testing.T) { defer cleanupAllTriages(dbc) From b4e7c380099248874728c549d317b75ad24ea8f0 Mon Sep 17 00:00:00 2001 From: Stephen Goeddel Date: Tue, 5 May 2026 15:03:35 -0400 Subject: [PATCH 2/3] Add a foreign key from triage_symptoms.symptom_id to job_run_symptoms.id with ON DELETE CASCADE so deleting a symptom definition cleans up associated triage_symptoms rows. Also fix error propagation for symptom summary lookups and add accessibility attributes to the symptom filter button. --- .../componentreadiness/regressiontracker.go | 4 +- pkg/db/db.go | 48 ++++++++----------- pkg/sippyserver/server.go | 6 +-- .../src/component_readiness/TriageSymptoms.js | 4 ++ .../regressiontracker_test.go | 24 ++++++++++ 5 files changed, 54 insertions(+), 32 deletions(-) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index bc61e2bcd..c21e424da 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -162,8 +162,8 @@ func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.Tes } } } - for symptomID, count := range symptomCounts { - for _, triage := range reg.Triages { + for _, triage := range reg.Triages { + for symptomID, count := range symptomCounts { ts := models.TriageSymptom{ TriageID: triage.ID, SymptomID: symptomID, diff --git a/pkg/db/db.go b/pkg/db/db.go index aa1cd6650..52a900caf 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -107,13 +107,11 @@ func (d *DB) UpdateSchema(reportEnd *time.Time) error { } } - // TODO(sgoeddel): This is temporary migration logic to backfill closed regressions with their most likely view. - // It should be removed after running for the first time. - if err := backfillClosedRegressionViews(d.DB); err != nil { + if err := createAuditLogIndexes(d.DB); err != nil { return err } - if err := createAuditLogIndexes(d.DB); err != nil { + if err := ensureTriageSymptomCascade(d.DB); err != nil { return err } @@ -215,29 +213,6 @@ func syncSchema(db *gorm.DB, hashType SchemaHashType, name, desiredSchema, dropS return updateRequired, nil } -// backfillClosedRegressionViews associates closed regressions that predate the regression_views -// table with their most likely view (-main). Historically only -main views had regression -// tracking enabled, so this is our best approximation. Only targets regressions with no existing -// view associations; open regressions are handled naturally by the loader. -func backfillClosedRegressionViews(db *gorm.DB) error { - res := db.Exec(` - INSERT INTO regression_views (test_regression_id, view_name, active, opened_at, closed_at) - SELECT tr.id, tr.release || '-main', false, tr.opened, tr.closed - FROM test_regressions tr - WHERE tr.closed IS NOT NULL - AND NOT EXISTS ( - SELECT 1 FROM regression_views rv WHERE rv.test_regression_id = tr.id - ) - ON CONFLICT (test_regression_id, view_name) DO NOTHING`) - if res.Error != nil { - return fmt.Errorf("error backfilling closed regression views: %w", res.Error) - } - if res.RowsAffected > 0 { - log.Infof("backfilled %d closed regressions with release-main view associations", res.RowsAffected) - } - return nil -} - // createAuditLogIndexes creates GIN indexes for JSONB columns in audit_logs table // for efficient JSON querying operations. func createAuditLogIndexes(db *gorm.DB) error { @@ -252,6 +227,25 @@ func createAuditLogIndexes(db *gorm.DB) error { return nil } +// ensureTriageSymptomCascade adds a foreign key from triage_symptoms.symptom_id +// to job_run_symptoms.id so that deleting a symptom definition automatically +// cleans up the associated triage_symptoms rows. +func ensureTriageSymptomCascade(db *gorm.DB) error { + return db.Exec(` + DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'fk_triage_symptoms_symptom' + ) THEN + ALTER TABLE triage_symptoms + ADD CONSTRAINT fk_triage_symptoms_symptom + FOREIGN KEY (symptom_id) REFERENCES job_run_symptoms(id) + ON DELETE CASCADE; + END IF; + END $$`).Error +} + func ParseGormLogLevel(logLevel string) (gormlogger.LogLevel, error) { switch logLevel { case "info": diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index b6b24e802..3165f605c 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1712,10 +1712,10 @@ func (s *Server) jsonGetTriageByID(w http.ResponseWriter, req *http.Request) { if expandFields["symptoms"] { symptomSummaries, err := componentreadiness.GetTriageSymptomSummaries(s.db, triage.ID, len(triage.Regressions)) if err != nil { - log.WithError(err).Errorf("error getting triage symptom summaries for triage %d", triage.ID) - } else { - et.SymptomSummaries = symptomSummaries + failureResponse(w, http.StatusInternalServerError, fmt.Sprintf("error getting symptom summaries for triage %d: %v", triage.ID, err)) + return } + et.SymptomSummaries = symptomSummaries } if expandFields["regressions"] { diff --git a/sippy-ng/src/component_readiness/TriageSymptoms.js b/sippy-ng/src/component_readiness/TriageSymptoms.js index 980861f18..d9520f5fc 100644 --- a/sippy-ng/src/component_readiness/TriageSymptoms.js +++ b/sippy-ng/src/component_readiness/TriageSymptoms.js @@ -82,6 +82,10 @@ export default function TriageSymptoms({ setSymptomFilter( symptomFilter === ss.symptom.id diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 6f5a39709..502643423 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -15,6 +15,7 @@ import ( "github.com/openshift/sippy/pkg/apis/api/componentreport/testdetails" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" + "github.com/openshift/sippy/pkg/db/models/jobrunscan" "github.com/openshift/sippy/test/e2e/util" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -515,6 +516,25 @@ func Test_RegressionJobRuns(t *testing.T) { }) } +func seedSymptom(t *testing.T, dbc *db.DB, id, summary string) { + sym := &jobrunscan.Symptom{ + SymptomContent: jobrunscan.SymptomContent{ + ID: id, + Summary: summary, + MatcherType: jobrunscan.MatcherTypeString, + MatchString: "e2e-test-match", + }, + } + res := dbc.DB.Where("id = ?", id).FirstOrCreate(sym) + require.NoError(t, res.Error) +} + +func cleanupSymptoms(dbc *db.DB, ids ...string) { + for _, id := range ids { + dbc.DB.Where("id = ?", id).Delete(&jobrunscan.Symptom{}) + } +} + func cleanupTriageSymptoms(dbc *db.DB) { res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) if res.Error != nil { @@ -556,6 +576,10 @@ func Test_SyncTriageSymptoms(t *testing.T) { } } + seedSymptom(t, dbc, "SymA", "Symptom A") + seedSymptom(t, dbc, "SymB", "Symptom B") + defer cleanupSymptoms(dbc, "SymA", "SymB") + cleanup := func() { cleanupTriageSymptoms(dbc) cleanupJobRuns(dbc) From 36a882018346869dc53aca3675232c9cfdbd5844 Mon Sep 17 00:00:00 2001 From: Stephen Goeddel Date: Tue, 5 May 2026 20:23:54 -0400 Subject: [PATCH 3/3] Harden triage symptoms with FK cascades, slim API, and test dedup Add FK cascades from triage_symptoms to both job_run_symptoms and test_regressions. Use ON CONFLICT DO UPDATE for single-roundtrip upserts. Slim TriageSymptomSummary to expose only symptom ID and summary. Hide raw junction rows from API via json:"-". Deduplicate e2e test helpers into test/e2e/util. --- docs/plans/trt-2633-triage-symptoms.md | 208 +++++++++--------- .../componentreadiness/regressiontracker.go | 18 +- pkg/api/componentreadiness/triage.go | 23 +- pkg/db/db.go | 49 +++-- pkg/db/models/triage.go | 2 +- .../regressiontracker_test.go | 36 +-- .../triage/triageapi_test.go | 36 +-- test/e2e/util/db.go | 29 +++ 8 files changed, 199 insertions(+), 202 deletions(-) diff --git a/docs/plans/trt-2633-triage-symptoms.md b/docs/plans/trt-2633-triage-symptoms.md index 9f04f3d68..65a1cc1ab 100644 --- a/docs/plans/trt-2633-triage-symptoms.md +++ b/docs/plans/trt-2633-triage-symptoms.md @@ -83,9 +83,10 @@ indexed query) and avoids walking the full regression → job run → symptom gr The junction table also stores a per-regression job run count (`job_run_count` column), which is summed across regressions to produce `job_run_count` in the summary. -Junction rows are upserted and counts updated in place on each loader run. -Since job runs only accumulate, counts grow monotonically and can't -double-count. +`SyncTriageSymptoms` does a full recount of symptoms from each regression's job +runs and replaces `job_run_count` (not increments), making it idempotent. Since +the operation replaces the count entirely on each run, calling it multiple times +with the same data produces the same result without double-counting. ## Implementation Plan @@ -178,9 +179,9 @@ jobRun := models.RegressionJobRun{ **File:** `pkg/api/componentreadiness/regressiontracker.go` -`MergeJobRuns` currently uses `FirstOrCreate` which won't update existing -records. Extend it to update `JobSymptoms` (and `JobLabels`) on existing -records so that newly-detected symptoms are captured on subsequent loader runs: +`MergeJobRuns` uses `Assign` + `FirstOrCreate` so that `JobSymptoms` (and +`JobLabels`) are updated on existing records when newly-detected symptoms are +captured on subsequent loader runs: ```go func (prs *PostgresRegressionStore) MergeJobRuns(regressionID uint, jobRuns []models.RegressionJobRun) error { @@ -227,34 +228,52 @@ The composite key `(triage_id, symptom_id, regression_id)` records exactly which regression(s) surfaced each symptom on a given triage. `JobRunCount` stores how many failed job runs on that regression exhibited the symptom. -Rows are upserted during the regression cache loader run, matching the -`FirstOrCreate`-then-update pattern used by `MergeJobRuns`. Since job runs only -accumulate (never deleted), symptom counts are monotonically increasing and -can be safely updated in place without a full replace. +Rows are upserted during the regression cache loader run via +`INSERT ... ON CONFLICT DO UPDATE SET job_run_count = EXCLUDED.job_run_count`. +Since `SyncTriageSymptoms` does a full recount and replaces the count, the +operation is idempotent. #### Add association to Triage model -Since the junction table now includes `regression_id`, GORM's built-in -`many2many` tag no longer fits (it expects a two-column join). Instead, model -`TriageSymptom` as a standalone entity and query it directly. +Since the junction table includes `regression_id`, GORM's built-in `many2many` +tag no longer fits (it expects a two-column join). Instead, `TriageSymptom` is +modeled as a standalone entity with a `foreignKey:TriageID` relationship. ```go type Triage struct { // ... existing fields ... - TriageSymptoms []TriageSymptom `json:"triage_symptoms,omitempty" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` + TriageSymptoms []TriageSymptom `json:"-" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` } ``` +The `json:"-"` tag prevents raw junction rows from leaking into API responses — +the curated `symptom_summaries` on `ExpandedTriage` is the intended API surface. + The `OnDelete:CASCADE` GORM constraint ensures that deleting a triage automatically removes its `TriageSymptom` junction rows. This follows the same pattern used by `TestRegression.JobRuns` and `TestRegression.Views`. +#### Foreign key constraints + +**File:** `pkg/db/db.go` + +In addition to GORM's auto-migrated `TriageSymptom` table, manual FK constraints +are added via `ensureTriageSymptomCascade()` to handle cascades that GORM's +`foreignKey` tag can't express (since `TriageSymptom` is not "owned" by either +`Symptom` or `TestRegression` in the GORM model graph): + +- `fk_triage_symptoms_symptom`: `symptom_id` → `job_run_symptoms.id` ON DELETE CASCADE +- `fk_triage_symptoms_regression`: `regression_id` → `test_regressions.id` ON DELETE CASCADE + +These ensure that deleting a symptom definition or a regression automatically +cleans up the associated `triage_symptoms` rows. The constraints are idempotent +(guarded by `IF NOT EXISTS` on `pg_constraint`). + #### Auto-migration **File:** `pkg/db/db.go` -Add `TriageSymptom` to the auto-migrate list (this creates the -`triage_symptoms` table). +`TriageSymptom` is in the auto-migrate list (creates the `triage_symptoms` table). ### Phase 3: Automatic Linking During Regression Tracking @@ -265,67 +284,26 @@ Add `TriageSymptom` to the auto-migrate list (this creates the ```go type RegressionStore interface { // ... existing methods ... - // SyncTriageSymptoms upserts symptom associations for all triages - // that have regressions with the given symptom-bearing job runs. + // SyncTriageSymptoms upserts symptom associations for triages based on regression job run data. SyncTriageSymptoms(regressions []*models.TestRegression) error } ``` #### Implementation +`SyncTriageSymptoms` does a full recount of symptoms from each regression's job +runs and replaces `job_run_count` (not increments), making the operation +idempotent and safe to call on every loader run. The upsert uses raw SQL +`INSERT ... ON CONFLICT DO UPDATE` to perform each upsert in a single DB +round-trip (following the `UpsertRegressionView` pattern). + ```go func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.TestRegression) error { - regIDs := make([]uint, len(regressions)) - for i, r := range regressions { - regIDs[i] = r.ID - } - - var regs []models.TestRegression - res := prs.dbc.DB. - Preload("Triages"). - Preload("JobRuns"). - Where("id IN ?", regIDs). - Find(®s) - if res.Error != nil { - return fmt.Errorf("error loading regressions for symptom sync: %w", res.Error) - } - - for _, reg := range regs { - if len(reg.Triages) == 0 { - continue - } - // Count job runs per symptom for this regression - symptomCounts := map[string]int{} - for _, jr := range reg.JobRuns { - seen := sets.New[string]() - for _, s := range jr.JobSymptoms { - if s != "" && !seen.Has(s) { - seen.Insert(s) - symptomCounts[s]++ - } - } - } - // Upsert one junction row per (triage, symptom, regression) - for symptomID, count := range symptomCounts { - for _, triage := range reg.Triages { - ts := models.TriageSymptom{ - TriageID: triage.ID, - SymptomID: symptomID, - RegressionID: reg.ID, - } - result := prs.dbc.DB.Where(ts).FirstOrCreate(&ts) - if result.Error != nil { - return fmt.Errorf("error syncing symptom %s to triage %d regression %d: %w", - symptomID, triage.ID, reg.ID, result.Error) - } - // Update count whether newly created or already existed - if err := prs.dbc.DB.Model(&ts).Update("job_run_count", count).Error; err != nil { - return fmt.Errorf("error updating symptom count: %w", err) - } - } - } - } - return nil + // Load regressions with Triages and JobRuns preloaded + // For each regression with triages: + // Count job runs per symptom (deduplicating within each run via sets.New) + // For each (triage, symptom) pair: + // INSERT INTO triage_symptoms ... ON CONFLICT DO UPDATE SET job_run_count = EXCLUDED.job_run_count } ``` @@ -333,12 +311,10 @@ func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.Tes **File:** `pkg/dataloader/regressioncacheloader/regressioncacheloader.go` -After the per-release regression closing loop (around line 180), add a global -symptom sync step: +After the per-release regression closing loop, a global symptom sync step runs +unconditionally for all active regressions: ```go -// SyncTriageSymptoms runs unconditionally — it is additive and idempotent, -// so partial errors in individual views should not block symptom linking. var allActiveRegs []*models.TestRegression for _, result := range releaseResults { for _, id := range result.activeIDs.UnsortedList() { @@ -353,10 +329,10 @@ if len(allActiveRegs) > 0 { } ``` -This runs once per loader execution, after all views are processed, ensuring -symptoms from all views are linked. It is not gated behind `!anyErrors` because -the operation is additive (only upserts) and idempotent — partial view errors -should not block symptom linking for regressions that were successfully processed. +This runs once per loader execution, after all views are processed. It is not +gated behind `!anyErrors` because the operation is additive (only upserts) and +idempotent — partial view errors should not block symptom linking for +regressions that were successfully processed. ### Phase 4: API Changes @@ -366,15 +342,22 @@ should not block symptom linking for regressions that were successfully processe ```go type TriageSymptomSummary struct { - Symptom jobrunscan.Symptom `json:"symptom"` - RegressionCount int `json:"regression_count"` - TotalCount int `json:"total_count"` - Percentage float64 `json:"percentage"` - JobRunCount int `json:"job_run_count"` - RegressionIDs []uint `json:"regression_ids"` + Symptom struct { + ID string `json:"id"` + Summary string `json:"summary"` + } `json:"symptom"` + RegressionCount int `json:"regression_count"` + TotalCount int `json:"total_count"` + Percentage float64 `json:"percentage"` + JobRunCount int `json:"job_run_count"` + RegressionIDs []uint `json:"regression_ids"` } ``` +The `Symptom` field is a slim struct exposing only `ID` and `Summary` — matching +rules, filters, and other internal fields from the full `jobrunscan.Symptom` +model are not exposed in the API response. + `RegressionIDs` lists which regressions on this triage exhibit each symptom. The frontend uses this to build a per-regression symptom map and to filter the regression table by symptom — without walking job runs client-side. @@ -398,6 +381,9 @@ type ExpandedTriage struct { } ``` +Errors from `GetTriageSymptomSummaries` return a 500 status response (not +logged and swallowed). + #### Comma-separated `expand` parameter The `jsonGetTriageByID` handler parses `expand` as a comma-separated set of @@ -434,6 +420,7 @@ provide context for interpreting the regressions below. **Symptoms section structure:** - Section header: **"Symptoms"** with a count badge (e.g., "Symptoms (3)") + and a tooltip explaining that symptoms are synced periodically. - If `symptom_summaries` is empty or absent, show a muted "No symptoms detected" message — don't hide the section entirely, so users know it exists. - If populated, render a MUI `Table` (not DataGrid — the list will be small @@ -441,28 +428,30 @@ provide context for interpreting the regressions below. | Column | Source field | Display | |--------|-------------|---------| -| Symptom | `symptom.summary` | Text, clickable link to the symptom detail page | -| Regressions | `regression_count` / `total_count` | e.g., "2 / 5", with a filter button (see below) | +| Symptom | `symptom.summary` | Colored `Chip` with deterministic background color based on symptom ID | +| Regressions | `regression_count` | Count with a filter `IconButton` (with `aria-label` and `aria-pressed` for accessibility) | | Percentage | `percentage` | MUI `LinearProgress` bar with percentage label | | Failed Runs | `job_run_count` | Plain number — total failed job runs across regressions exhibiting this symptom | - Table is sorted by percentage descending (already sorted by the API). -- Each symptom row's summary text should link to the Job Artifact Query page - prefilled with that symptom, following the existing `JAQPrefilled` pattern - from `JobArtifactQuery.js`. + +**Component:** `sippy-ng/src/component_readiness/TriageSymptoms.js` + +A dedicated component renders the symptoms table. It receives +`symptomSummaries`, `symptomFilter`, and `setSymptomFilter` props. #### Filtering Regressions by Symptom -The Regressions column shows how many of the triage's regressions exhibit each -symptom (e.g., "2 / 5"). Next to the count, add a small MUI `IconButton` with -a filter icon (`FilterList`). Clicking it filters the "Included Tests" +The Regressions column includes a small MUI `IconButton` with a filter icon +(`FilterList`). Clicking it filters the "Included Tests" `TriagedRegressionTestList` DataGrid below to show only regressions that have -that symptom. +that symptom. Clicking again clears the filter (toggle behavior). **Implementation:** - `Triage.js` holds a `symptomFilter` state (symptom ID or `null`). -- Clicking the filter button sets `symptomFilter` to that symptom's ID. +- Clicking the filter button toggles `symptomFilter` to that symptom's ID + or back to `null`. - When a filter is active, show a `Chip` above the regressions table indicating the active filter (e.g., "Filtered by: ") with a clear (X) button that resets `symptomFilter` to `null`. @@ -511,7 +500,10 @@ Added to existing `Test_RegressionJobRuns`: **File:** `test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go` -New top-level `Test_SyncTriageSymptoms` function: +New top-level `Test_SyncTriageSymptoms` function. Tests seed `job_run_symptoms` +records for the test symptom IDs ("SymA", "SymB") via the shared +`util.SeedSymptom` helper so that the FK constraint +(`fk_triage_symptoms_symptom`) is satisfied. | Subtest | Setup | Assert | |---------|-------|--------| @@ -526,7 +518,8 @@ New top-level `Test_SyncTriageSymptoms` function: **File:** `test/e2e/componentreadiness/triage/triageapi_test.go` -Added to existing `Test_TriageAPI`: +Added to existing `Test_TriageAPI`. Tests seed `job_run_symptoms` records via +the shared `util.SeedSymptom` helper. | Subtest | Setup | Assert | |---------|-------|--------| @@ -534,18 +527,24 @@ Added to existing `Test_TriageAPI`: | expand=symptoms only returns symptoms without regressed_tests | Same setup, GET `?expand=symptoms` | `symptom_summaries` present, `regressed_tests` nil | | delete triage cascades to triage_symptoms | Create triage + symptoms, DELETE triage | Zero junction rows for that triage | -These tests seed `job_run_symptoms` (Symptom) records for the test symptom IDs -so `GetTriageSymptomSummaries` can resolve them via the join. +### Shared Test Helpers + +**File:** `test/e2e/util/db.go` + +Exported helpers used by both e2e test packages: + +- `SeedSymptom(t, dbc, id, summary)` — creates a `jobrunscan.Symptom` record (idempotent via `FirstOrCreate`) +- `CleanupSymptoms(dbc, ids...)` — deletes symptom records by ID +- `CleanupTriageSymptoms(dbc)` — deletes all `triage_symptoms` rows ## Cascade and Deletion Behavior -| Action | Result | -|--------|--------| -| Delete triage | Junction rows in `triage_symptoms` removed. Symptoms unaffected. | -| Soft-delete symptom | Junction rows persist. Symptom filtered from preloaded queries (GORM soft delete). | -| Hard-delete symptom | Junction rows removed (DB-level FK cascade or explicit cleanup). | -| Delete regression from triage | Junction rows for that triage/regression pair should be removed (FK cascade or explicit cleanup). | -| Close regression | No effect on junction. Symptoms remain as historical record. | +| Action | Result | Mechanism | +|--------|--------|-----------| +| Delete triage | Junction rows in `triage_symptoms` removed | GORM `constraint:OnDelete:CASCADE` on `Triage.TriageSymptoms` | +| Delete symptom definition | Junction rows in `triage_symptoms` removed | Manual FK `fk_triage_symptoms_symptom` in `db.go` | +| Delete regression | Junction rows in `triage_symptoms` removed | Manual FK `fk_triage_symptoms_regression` in `db.go` | +| Close regression | No effect on junction. Symptoms remain as historical record | — | ## Migration and Backward Compatibility @@ -563,7 +562,6 @@ so `GetTriageSymptomSummaries` can resolve them via the join. 4. **API backward compatibility.** The `symptom_summaries` field is additive on the `ExpandedTriage` response. Clients that don't use it are unaffected. - The `symptoms` field on `Triage` is also additive. ## Out of Scope (Future Work) diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index c21e424da..81a4982ea 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -164,18 +164,14 @@ func (prs *PostgresRegressionStore) SyncTriageSymptoms(regressions []*models.Tes } for _, triage := range reg.Triages { for symptomID, count := range symptomCounts { - ts := models.TriageSymptom{ - TriageID: triage.ID, - SymptomID: symptomID, - RegressionID: reg.ID, - } - result := prs.dbc.DB.Where(ts).FirstOrCreate(&ts) - if result.Error != nil { + if err := prs.dbc.DB.Exec( + `INSERT INTO triage_symptoms (triage_id, symptom_id, regression_id, job_run_count) + VALUES (?, ?, ?, ?) + ON CONFLICT (triage_id, symptom_id, regression_id) DO UPDATE + SET job_run_count = EXCLUDED.job_run_count`, + triage.ID, symptomID, reg.ID, count).Error; err != nil { return fmt.Errorf("error syncing symptom %s to triage %d regression %d: %w", - symptomID, triage.ID, reg.ID, result.Error) - } - if err := prs.dbc.DB.Model(&ts).Update("job_run_count", count).Error; err != nil { - return fmt.Errorf("error updating symptom job run count: %w", err) + symptomID, triage.ID, reg.ID, err) } } } diff --git a/pkg/api/componentreadiness/triage.go b/pkg/api/componentreadiness/triage.go index 451a07c9d..767bf7e1b 100644 --- a/pkg/api/componentreadiness/triage.go +++ b/pkg/api/componentreadiness/triage.go @@ -865,12 +865,15 @@ func generateTestDetailsURLFromRegression(regression *models.TestRegression, vie // TriageSymptomSummary represents a symptom found across a triage's regressions, // with counts and percentages for the triage detail view. type TriageSymptomSummary struct { - Symptom jobrunscan.Symptom `json:"symptom"` - RegressionCount int `json:"regression_count"` - TotalCount int `json:"total_count"` - Percentage float64 `json:"percentage"` - JobRunCount int `json:"job_run_count"` - RegressionIDs []uint `json:"regression_ids"` + Symptom struct { + ID string `json:"id"` + Summary string `json:"summary"` + } `json:"symptom"` + RegressionCount int `json:"regression_count"` + TotalCount int `json:"total_count"` + Percentage float64 `json:"percentage"` + JobRunCount int `json:"job_run_count"` + RegressionIDs []uint `json:"regression_ids"` } // GetTriageSymptomSummaries queries the triage_symptoms junction table to build @@ -926,14 +929,16 @@ func GetTriageSymptomSummaries(dbc *db.DB, triageID uint, totalRegressions int) if !ok { continue } - summaries = append(summaries, TriageSymptomSummary{ - Symptom: s, + summary := TriageSymptomSummary{ RegressionCount: c.RegressionCount, TotalCount: totalRegressions, Percentage: float64(c.RegressionCount) / float64(totalRegressions) * 100, JobRunCount: c.JobRunCount, RegressionIDs: regIDsBySymptom[c.SymptomID], - }) + } + summary.Symptom.ID = s.ID + summary.Symptom.Summary = s.Summary + summaries = append(summaries, summary) } sort.Slice(summaries, func(i, j int) bool { return summaries[i].RegressionCount > summaries[j].RegressionCount diff --git a/pkg/db/db.go b/pkg/db/db.go index 52a900caf..e77cdfb1a 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -227,23 +227,40 @@ func createAuditLogIndexes(db *gorm.DB) error { return nil } -// ensureTriageSymptomCascade adds a foreign key from triage_symptoms.symptom_id -// to job_run_symptoms.id so that deleting a symptom definition automatically -// cleans up the associated triage_symptoms rows. +// ensureTriageSymptomCascade adds foreign keys to triage_symptoms with ON DELETE CASCADE +// so that deleting a symptom definition or a regression automatically cleans up the +// associated triage_symptoms rows. func ensureTriageSymptomCascade(db *gorm.DB) error { - return db.Exec(` - DO $$ - BEGIN - IF NOT EXISTS ( - SELECT 1 FROM pg_constraint - WHERE conname = 'fk_triage_symptoms_symptom' - ) THEN - ALTER TABLE triage_symptoms - ADD CONSTRAINT fk_triage_symptoms_symptom - FOREIGN KEY (symptom_id) REFERENCES job_run_symptoms(id) - ON DELETE CASCADE; - END IF; - END $$`).Error + constraints := []struct { + name string + sql string + }{ + { + name: "fk_triage_symptoms_symptom", + sql: "ALTER TABLE triage_symptoms ADD CONSTRAINT fk_triage_symptoms_symptom FOREIGN KEY (symptom_id) REFERENCES job_run_symptoms(id) ON DELETE CASCADE", + }, + { + name: "fk_triage_symptoms_regression", + sql: "ALTER TABLE triage_symptoms ADD CONSTRAINT fk_triage_symptoms_regression FOREIGN KEY (regression_id) REFERENCES test_regressions(id) ON DELETE CASCADE", + }, + } + + for _, c := range constraints { + err := db.Exec(fmt.Sprintf(` + DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = '%s' + ) THEN + %s; + END IF; + END $$`, c.name, c.sql)).Error + if err != nil { + return err + } + } + return nil } func ParseGormLogLevel(logLevel string) (gormlogger.LogLevel, error) { diff --git a/pkg/db/models/triage.go b/pkg/db/models/triage.go index 0fd8a6be8..d56e7a179 100644 --- a/pkg/db/models/triage.go +++ b/pkg/db/models/triage.go @@ -49,7 +49,7 @@ type Triage struct { Regressions []TestRegression `json:"regressions" gorm:"constraint:OnDelete:CASCADE;many2many:triage_regressions;"` // TriageSymptoms links symptoms discovered in regression job runs to this triage. - TriageSymptoms []TriageSymptom `json:"triage_symptoms,omitempty" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` + TriageSymptoms []TriageSymptom `json:"-" gorm:"foreignKey:TriageID;constraint:OnDelete:CASCADE"` // Resolution is an important field presently set by a user indicating a claimed time this issue was resolved, // and thus all associated regressions should be fixed. diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 502643423..2dff90f1a 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -15,7 +15,7 @@ import ( "github.com/openshift/sippy/pkg/apis/api/componentreport/testdetails" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" - "github.com/openshift/sippy/pkg/db/models/jobrunscan" + "github.com/openshift/sippy/test/e2e/util" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -516,32 +516,6 @@ func Test_RegressionJobRuns(t *testing.T) { }) } -func seedSymptom(t *testing.T, dbc *db.DB, id, summary string) { - sym := &jobrunscan.Symptom{ - SymptomContent: jobrunscan.SymptomContent{ - ID: id, - Summary: summary, - MatcherType: jobrunscan.MatcherTypeString, - MatchString: "e2e-test-match", - }, - } - res := dbc.DB.Where("id = ?", id).FirstOrCreate(sym) - require.NoError(t, res.Error) -} - -func cleanupSymptoms(dbc *db.DB, ids ...string) { - for _, id := range ids { - dbc.DB.Where("id = ?", id).Delete(&jobrunscan.Symptom{}) - } -} - -func cleanupTriageSymptoms(dbc *db.DB) { - res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) - if res.Error != nil { - log.Errorf("error deleting triage symptoms: %v", res.Error) - } -} - func cleanupTriages(dbc *db.DB) { dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") dbc.DB.Where("1 = 1").Delete(&models.Triage{}) @@ -576,12 +550,12 @@ func Test_SyncTriageSymptoms(t *testing.T) { } } - seedSymptom(t, dbc, "SymA", "Symptom A") - seedSymptom(t, dbc, "SymB", "Symptom B") - defer cleanupSymptoms(dbc, "SymA", "SymB") + util.SeedSymptom(t, dbc, "SymA", "Symptom A") + util.SeedSymptom(t, dbc, "SymB", "Symptom B") + defer util.CleanupSymptoms(dbc, "SymA", "SymB") cleanup := func() { - cleanupTriageSymptoms(dbc) + util.CleanupTriageSymptoms(dbc) cleanupJobRuns(dbc) cleanupTriages(dbc) cleanupAllRegressions(dbc) diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index 8428d1708..a77526b91 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -18,7 +18,6 @@ import ( "github.com/openshift/sippy/pkg/apis/api/componentreport/testdetails" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" - "github.com/openshift/sippy/pkg/db/models/jobrunscan" "github.com/openshift/sippy/pkg/sippyserver" "github.com/openshift/sippy/test/e2e/util" log "github.com/sirupsen/logrus" @@ -41,27 +40,6 @@ var view = crview.View{ }, } -func cleanupTriageSymptoms(dbc *db.DB) { - res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) - if res.Error != nil { - log.Errorf("error deleting triage symptoms: %v", res.Error) - } -} - -func seedSymptom(t *testing.T, gormDB *gorm.DB, id, summary string) *jobrunscan.Symptom { - sym := &jobrunscan.Symptom{ - SymptomContent: jobrunscan.SymptomContent{ - ID: id, - Summary: summary, - MatcherType: jobrunscan.MatcherTypeString, - MatchString: "e2e-test-match", - }, - } - res := gormDB.Create(sym) - require.NoError(t, res.Error) - return sym -} - func cleanupAllTriages(dbc *db.DB) { // Delete all triage and test regressions in the e2e postgres db. dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") @@ -416,11 +394,11 @@ func Test_TriageAPI(t *testing.T) { t.Run("expanded triage includes symptom summaries", func(t *testing.T) { defer cleanupAllTriages(dbc) - defer cleanupTriageSymptoms(dbc) + defer util.CleanupTriageSymptoms(dbc) - symA := seedSymptom(t, dbc.DB, "e2e-sym-a", "E2E Symptom A") + symA := util.SeedSymptom(t, dbc, "e2e-sym-a", "E2E Symptom A") defer dbc.DB.Delete(symA) - symB := seedSymptom(t, dbc.DB, "e2e-sym-b", "E2E Symptom B") + symB := util.SeedSymptom(t, dbc, "e2e-sym-b", "E2E Symptom B") defer dbc.DB.Delete(symB) reg := createTestRegression(t, tracker, view, "sym-expand-test-1") @@ -469,9 +447,9 @@ func Test_TriageAPI(t *testing.T) { t.Run("expand=symptoms only returns symptoms without regressed_tests", func(t *testing.T) { defer cleanupAllTriages(dbc) - defer cleanupTriageSymptoms(dbc) + defer util.CleanupTriageSymptoms(dbc) - sym := seedSymptom(t, dbc.DB, "e2e-sym-only", "E2E Symptom Only") + sym := util.SeedSymptom(t, dbc, "e2e-sym-only", "E2E Symptom Only") defer dbc.DB.Delete(sym) reg := createTestRegression(t, tracker, view, "sym-only-test-1") @@ -506,9 +484,9 @@ func Test_TriageAPI(t *testing.T) { t.Run("delete triage cascades to triage_symptoms", func(t *testing.T) { defer cleanupAllTriages(dbc) - defer cleanupTriageSymptoms(dbc) + defer util.CleanupTriageSymptoms(dbc) - sym := seedSymptom(t, dbc.DB, "e2e-sym-cascade", "E2E Symptom Cascade") + sym := util.SeedSymptom(t, dbc, "e2e-sym-cascade", "E2E Symptom Cascade") defer dbc.DB.Delete(sym) reg := createTestRegression(t, tracker, view, "sym-cascade-test-1") diff --git a/test/e2e/util/db.go b/test/e2e/util/db.go index 194823399..9ea6bc55f 100644 --- a/test/e2e/util/db.go +++ b/test/e2e/util/db.go @@ -6,6 +6,8 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" + "github.com/openshift/sippy/pkg/db/models/jobrunscan" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "gorm.io/gorm/logger" ) @@ -27,3 +29,30 @@ func CreateE2EPostgresConnection(t *testing.T) *db.DB { return dbc } + +func SeedSymptom(t *testing.T, dbc *db.DB, id, summary string) *jobrunscan.Symptom { + sym := &jobrunscan.Symptom{ + SymptomContent: jobrunscan.SymptomContent{ + ID: id, + Summary: summary, + MatcherType: jobrunscan.MatcherTypeString, + MatchString: "e2e-test-match", + }, + } + res := dbc.DB.Where("id = ?", id).FirstOrCreate(sym) + require.NoError(t, res.Error) + return sym +} + +func CleanupSymptoms(dbc *db.DB, ids ...string) { + for _, id := range ids { + dbc.DB.Where("id = ?", id).Delete(&jobrunscan.Symptom{}) + } +} + +func CleanupTriageSymptoms(dbc *db.DB) { + res := dbc.DB.Where("1 = 1").Delete(&models.TriageSymptom{}) + if res.Error != nil { + log.Errorf("error deleting triage symptoms: %v", res.Error) + } +}