diff --git a/pkg/api/README.md b/pkg/api/README.md index 35cf16475..1adcfb351 100644 --- a/pkg/api/README.md +++ b/pkg/api/README.md @@ -644,3 +644,74 @@ Updates an existing triage record. Endpoint: `DELETE /api/component_readiness/triages/{id}` Deletes a triage record. + +Endpoint: `POST /api/component_readiness/triages/{id}/force_close_regressions` + +Force closes the open regressions associated with a resolved triage that existed at its +resolution time (opened strictly before `resolved`). Force closed regressions are excluded from +the regression reuse window (regressionHysteresisDays), so they are not reopened for unrelated +failures. This prevents generic tests (for example "install should succeed") from staying open +for weeks with false "pants on fire" or "failed fix" status. + +Each regression is closed at the triage's resolution time and records, directly on the +regression row, that it was force closed, by which user, for what reason, and the triage that +drove the action. The operation is idempotent: regressions that opened at or after the +resolution time, or that are already closed, are left untouched. + +The triage must be resolved. If it is not, the endpoint returns `400 Bad Request` with the +message "Cannot force-close regressions for an unresolved triage. Resolve the triage first." +This is a write endpoint and requires the `write_endpoints` capability. + +### Request body + +| Field | Type | Description | Required | +|--------|--------|----------------------------------------------------------|----------| +| reason | String | The reason the regressions are being force closed. | Yes | + +`reason` is required and must be non-empty (a blank or whitespace-only value returns +`400 Bad Request`). A non-numeric or negative triage `id` in the path returns `400 Bad Request`, +and a triage `id` that does not exist returns `404 Not Found`. + +### Response + +| Field | Type | Description | +|-------------------------|-----------------|-----------------------------------------------------------------| +| closed_regression_ids | Array of number | IDs of the regressions that were open and got closed. | +| timestamp | String (time) | The closed time applied to the regressions (the resolution time). | +| links | Object | HATEOAS links (`self`, `triage`, `force_close`, `force_close_preview`). | + +The regression record returned by `GET /api/component_readiness/regressions/{id}` includes +`force_closed`, `force_closed_by`, `force_closed_reason`, and `force_closed_by_triage_id` +directly (no join is required, the data is stored on the regression). + +Endpoint: `GET /api/component_readiness/triages/{id}/force_close_preview` + +Previews (dry run) what `force_close_regressions` would do for a resolved triage, without +modifying anything. Use it to review which regressions would close and to spot any that kept +failing after the claimed resolution before committing. The triage must be resolved; otherwise +the endpoint returns `400 Bad Request` with the same message as the force close endpoint. A +non-numeric or negative triage `id` returns `400 Bad Request`, and a triage `id` that does not +exist returns `404 Not Found`. + +### Preview response + +| Field | Type | Description | +|-----------------|-----------------|---------------------------------------------------------------------| +| triage_id | Number | The triage being previewed. | +| resolved | String (time) | The triage's resolution time (the cutoff used for scoping). | +| would_close | Array of object | Open regressions that opened strictly before the resolution time (would close). | +| would_not_close | Array of object | Regressions opened at or after the resolution time (left untouched). | +| links | Object | HATEOAS links (`self`, `triage`, `force_close`, `force_close_preview`). | + +Each regression object in `would_close` / `would_not_close` includes: + +| Field | Type | Description | +|-------------------------------|-----------------|-----------------------------------------------------------------| +| regression_id | Number | The regression ID. | +| test_name | String | The regressed test name. | +| variants | Array of string | The regression's variants. | +| opened | String (time) | When the regression opened. | +| closed | String (time) | When the regression closed, if already closed. | +| last_failure_before_resolution| String (time) | Most recent failing job run at or before the resolution time. | +| first_failure_after_resolution| String (time) | Earliest failing job run after the resolution time, if any (a gap indicator that the test kept failing). | +| links | Object | HATEOAS links for the regression (`self` points to its detail endpoint). | diff --git a/pkg/api/componentreadiness/regressiontracker.go b/pkg/api/componentreadiness/regressiontracker.go index 81a4982ea..fe4e884b2 100644 --- a/pkg/api/componentreadiness/regressiontracker.go +++ b/pkg/api/componentreadiness/regressiontracker.go @@ -3,7 +3,9 @@ package componentreadiness import ( "context" "database/sql" + "errors" "fmt" + "strings" "time" "github.com/andygrunwald/go-jira" @@ -16,6 +18,7 @@ import ( "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/db/models" log "github.com/sirupsen/logrus" + "gorm.io/gorm/clause" "k8s.io/apimachinery/pkg/util/sets" ) @@ -36,6 +39,14 @@ type RegressionStore interface { UpdateRegression(reg *models.TestRegression) error // ResolveTriages sets the resolution time on any triages that no longer have active regressions ResolveTriages() error + // ForceCloseRegressions closes the open regressions associated with the given resolved triage that + // existed at its resolution time, marking them force closed so they are excluded from the reuse window + // and not reopened for unrelated failures. Returns ErrTriageNotResolved if the triage is not resolved. + // It is idempotent. + ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) + // ForceClosePreview returns a dry-run of what ForceCloseRegressions would do for the given triage, + // without modifying anything. Returns ErrTriageNotResolved if the triage is not resolved. + ForceClosePreview(triageID uint) (*ForceClosePreview, error) // MergeJobRuns upserts job runs for a regression, adding new ones and skipping duplicates. MergeJobRuns(regressionID uint, jobRuns []models.RegressionJobRun) error // UpsertRegressionView records that a regression was observed in a view, setting active=true. @@ -59,9 +70,11 @@ func (prs *PostgresRegressionStore) ListCurrentRegressionsForRelease(release str // List open regressions (no closed date), or those that closed within the last few days. This is to prevent flapping // and return more accurate opened dates when a test is falling in / out of the report. regressions := make([]*models.TestRegression, 0) + // Force closed regressions are excluded from the reuse window even if they closed recently, so they + // are not reopened for unrelated failures (TRT-2895). q := prs.dbc.DB.Table(testRegressionsTable). Where("release = ?", release). - Where("closed IS NULL OR closed > ?", time.Now().Add(-regressionHysteresisDays*24*time.Hour)) + Where("closed IS NULL OR (closed > ? AND force_closed = false)", time.Now().Add(-regressionHysteresisDays*24*time.Hour)) res := q.Scan(®ressions) return regressions, res.Error } @@ -217,7 +230,7 @@ func (prs *PostgresRegressionStore) ResolveTriages() error { subQuery := prs.dbc.DB.Table("triage_regressions tr"). Joins("JOIN test_regressions r ON tr.test_regression_id = r.id"). Where("tr.triage_id = triages.id"). - Where("r.closed IS NULL OR r.closed > ?", hysteresisTime). + Where("r.closed IS NULL OR (r.closed > ? AND r.force_closed = false)", hysteresisTime). Select("1") res := prs.dbc.DB.Table("triages"). @@ -265,6 +278,292 @@ func (prs *PostgresRegressionStore) ResolveTriages() error { return nil } +// ErrTriageNotResolved is returned by force close operations when the triage has not been resolved. +// Force closing needs a resolution time to scope which regressions to close and when to close them. +var ErrTriageNotResolved = errors.New("triage is not resolved") + +// ErrForceCloseReasonRequired is returned by ForceCloseRegressions when called without a reason. Force +// close records the reason on each regression for audit, so a reason is required regardless of caller. +var ErrForceCloseReasonRequired = errors.New("a reason is required to force close regressions") + +// ForceCloseResult summarizes the outcome of a force close operation. It is returned by the API +// so callers know which regressions were closed and at what time. +type ForceCloseResult struct { + // ClosedRegressionIDs are the IDs of the regressions that were open and got closed by this call. + // On an idempotent repeat call (regressions already closed) this will be empty. + ClosedRegressionIDs []uint `json:"closed_regression_ids"` + // Timestamp is the closed time applied to the regressions (the triage's resolution time). + Timestamp time.Time `json:"timestamp"` + // Links holds HATEOAS links for discoverability (for example self and the owning triage). + Links map[string]string `json:"links,omitempty"` +} + +// RegressionFailureGap describes the failure timing around a triage's resolution for a single regression. +// A non-nil FirstFailureAfterResolution means the test kept failing after the claimed resolution, a signal +// the triage may have been resolved prematurely and force closing warrants a closer look. +type RegressionFailureGap struct { + // LastFailureBeforeResolution is the most recent failing job run at or before the resolution time. + LastFailureBeforeResolution *time.Time `json:"last_failure_before_resolution,omitempty"` + // FirstFailureAfterResolution is the earliest failing job run after the resolution time, if any. + FirstFailureAfterResolution *time.Time `json:"first_failure_after_resolution,omitempty"` +} + +// ForceClosePreviewRegression describes a single regression in a force close preview, including the failure +// gap around the triage's resolution so a user can judge whether force closing is appropriate. +type ForceClosePreviewRegression struct { + RegressionID uint `json:"regression_id"` + TestName string `json:"test_name"` + Variants pq.StringArray `json:"variants"` + Opened time.Time `json:"opened"` + // Closed is set when the regression is already closed. + Closed *time.Time `json:"closed,omitempty"` + // RegressionFailureGap is embedded so its fields are promoted into this object's JSON. + RegressionFailureGap + // Links holds HATEOAS links for this regression (for example self, the regression detail endpoint). + Links map[string]string `json:"links,omitempty"` +} + +// ForceClosePreview is the dry-run result for force closing a triage's regressions. WouldClose lists the +// open regressions that opened strictly before the resolution time and would be closed; WouldNotClose lists +// regressions that opened at or after the resolution time and would be left untouched. +type ForceClosePreview struct { + TriageID uint `json:"triage_id"` + Resolved time.Time `json:"resolved"` + WouldClose []ForceClosePreviewRegression `json:"would_close"` + WouldNotClose []ForceClosePreviewRegression `json:"would_not_close"` + // Links holds HATEOAS links for discoverability (for example self and the owning triage). + Links map[string]string `json:"links,omitempty"` +} + +// ForceCloseRegressions closes the open regressions associated with the given resolved triage that existed +// at its resolution time (opened strictly before triage.Resolved), marking them force closed so they are +// excluded from the regression reuse window (regressionHysteresisDays) and never reopened for unrelated +// failures (TRT-2895). Each regression is closed at the triage's resolution time and records who force +// closed it and why, directly on the regression row. The triage must be resolved; otherwise +// ErrTriageNotResolved is returned. It is idempotent: already-closed regressions are left untouched. +func (prs *PostgresRegressionStore) ForceCloseRegressions(triageID uint, closedBy, reason string) (*ForceCloseResult, error) { + result := &ForceCloseResult{} + + // A reason is recorded on each force closed regression for audit, so refuse to proceed without one + // regardless of caller. The HTTP handler validates this earlier to return a precise status; this + // store-level check keeps the invariant for any caller. + if strings.TrimSpace(reason) == "" { + return nil, ErrForceCloseReasonRequired + } + + var triage models.Triage + if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { + return nil, fmt.Errorf("error loading triage %d for force close: %w", triageID, err) + } + if !triage.Resolved.Valid { + return nil, ErrTriageNotResolved + } + closeTime := triage.Resolved.Time + result.Timestamp = closeTime + + // Force close in a single atomic UPDATE. The eligible regressions are selected inside the + // statement via a subquery on the triage_regressions join table, so there is no window between + // finding the IDs and updating them in which a concurrent force close could see the same rows + // and overwrite each other's metadata. Eligibility: associated with this triage, still open + // (closed IS NULL), and opened strictly before the resolution time (opened < closeTime) so a + // regression opened at the exact resolution instant is left untouched. RETURNING id reports + // exactly which rows this call closed, keeping the operation idempotent (a repeat call matches + // nothing and returns an empty list). + // + // Updates() with a column map (not Save()) is deliberate: TestRegression has a many2many with + // Triage via triage_regressions, and Save() would rewrite that association join table. A column + // map only emits UPDATE SET for the named columns and never touches the join table. + var closed []models.TestRegression + res := prs.dbc.DB.Model(&closed). + Clauses(clause.Returning{Columns: []clause.Column{{Name: "id"}}}). + Where("test_regressions.id IN (?)", + prs.dbc.DB.Table("triage_regressions"). + Select("test_regression_id"). + Where("triage_id = ?", triageID)). + Where("test_regressions.closed IS NULL"). + Where("test_regressions.opened < ?", closeTime). + Updates(map[string]any{ + "closed": sql.NullTime{Valid: true, Time: closeTime}, + "force_closed": true, + "force_closed_by": closedBy, + "force_closed_reason": reason, + "force_closed_by_triage_id": triageID, + }) + if res.Error != nil { + return nil, fmt.Errorf("error force closing regressions for triage %d: %w", triageID, res.Error) + } + for i := range closed { + result.ClosedRegressionIDs = append(result.ClosedRegressionIDs, closed[i].ID) + } + + // Do not log the user identity: log only the triage ID and how many regressions were closed. + log.WithField("triageID", triageID). + WithField("closedRegressions", len(result.ClosedRegressionIDs)).Info("force closed regressions for triage") + return result, nil +} + +// queryRegressionFailureGaps computes, for each of the given regressions, the last failing job run at +// or before resolutionTime and the first failing job run after resolutionTime using the +// regression_job_runs table. It runs a single grouped query per direction (rather than a query per +// regression) to avoid an N+1 pattern. Regressions with no matching failing run are absent from the +// corresponding map entry / field. +func (prs *PostgresRegressionStore) queryRegressionFailureGaps(regressionIDs []uint, resolutionTime time.Time) (map[uint]RegressionFailureGap, error) { + gaps := make(map[uint]RegressionFailureGap, len(regressionIDs)) + if len(regressionIDs) == 0 { + return gaps, nil + } + + // gapRow captures the grouped aggregate (MAX/MIN start_time) per regression_id. + type gapRow struct { + RegressionID uint + FailureTime sql.NullTime + } + + // Last failing run at or before the resolution time, one row per regression. + var lastRows []gapRow + if err := prs.dbc.DB.Table("regression_job_runs"). + Select("regression_id, MAX(start_time) AS failure_time"). + Where("regression_id IN ? AND start_time <= ? AND test_failed = true", regressionIDs, resolutionTime). + Group("regression_id"). + Scan(&lastRows).Error; err != nil { + return nil, fmt.Errorf("error querying last failures before resolution for regressions %v: %w", regressionIDs, err) + } + for _, row := range lastRows { + if row.FailureTime.Valid { + t := row.FailureTime.Time + gap := gaps[row.RegressionID] + gap.LastFailureBeforeResolution = &t + gaps[row.RegressionID] = gap + } + } + + // First failing run after the resolution time, one row per regression. + var firstRows []gapRow + if err := prs.dbc.DB.Table("regression_job_runs"). + Select("regression_id, MIN(start_time) AS failure_time"). + Where("regression_id IN ? AND start_time > ? AND test_failed = true", regressionIDs, resolutionTime). + Group("regression_id"). + Scan(&firstRows).Error; err != nil { + return nil, fmt.Errorf("error querying first failures after resolution for regressions %v: %w", regressionIDs, err) + } + for _, row := range firstRows { + if row.FailureTime.Valid { + t := row.FailureTime.Time + gap := gaps[row.RegressionID] + gap.FirstFailureAfterResolution = &t + gaps[row.RegressionID] = gap + } + } + + return gaps, nil +} + +// ForceClosePreview returns a dry-run of what ForceCloseRegressions would do for the given triage without +// modifying anything. The triage must be resolved; otherwise ErrTriageNotResolved is returned. Each entry +// includes the failure gap around the resolution time so callers can spot regressions that kept failing. +func (prs *PostgresRegressionStore) ForceClosePreview(triageID uint) (*ForceClosePreview, error) { + var triage models.Triage + if err := prs.dbc.DB.First(&triage, triageID).Error; err != nil { + return nil, fmt.Errorf("error loading triage %d for force close preview: %w", triageID, err) + } + if !triage.Resolved.Valid { + return nil, ErrTriageNotResolved + } + resolved := triage.Resolved.Time + preview := &ForceClosePreview{TriageID: triageID, Resolved: resolved} + + // Load only the regressions the preview reports on: those still open (would close / would not close + // depending on when they opened) or those opened at or after the resolution time. Already-closed + // regressions that opened before resolution are omitted (force closing would not change them), so we + // filter them out in the query rather than loading every regression ever associated with the triage. + var regs []models.TestRegression + if err := prs.dbc.DB.Table(testRegressionsTable). + Select("test_regressions.*"). + Joins("JOIN triage_regressions ON triage_regressions.test_regression_id = test_regressions.id"). + Where("triage_regressions.triage_id = ?", triageID). + Where("test_regressions.closed IS NULL OR test_regressions.opened >= ?", resolved). + Find(®s).Error; err != nil { + return nil, fmt.Errorf("error loading regressions for triage %d force close preview: %w", triageID, err) + } + + regIDs := make([]uint, len(regs)) + for i := range regs { + regIDs[i] = regs[i].ID + } + gaps, err := prs.queryRegressionFailureGaps(regIDs, resolved) + if err != nil { + return nil, err + } + + for i := range regs { + reg := ®s[i] + entry := ForceClosePreviewRegression{ + RegressionID: reg.ID, + TestName: reg.TestName, + Variants: reg.Variants, + Opened: reg.Opened, + RegressionFailureGap: gaps[reg.ID], + } + if reg.Closed.Valid { + c := reg.Closed.Time + entry.Closed = &c + } + switch { + case !reg.Closed.Valid && reg.Opened.Before(resolved): + // Open and opened strictly before the resolution time: this is what force close would close. + preview.WouldClose = append(preview.WouldClose, entry) + case !reg.Opened.Before(resolved): + // Opened at or after the resolution time: force close would leave it untouched. A regression + // opened at the exact resolution instant is intentionally excluded from closing (strict <). + preview.WouldNotClose = append(preview.WouldNotClose, entry) + } + } + return preview, nil +} + +// forceCloseTriageLinks builds the HATEOAS links common to force close responses: the owning triage +// and the two force close endpoints for the triage. +func forceCloseTriageLinks(baseURL string, triageID uint) map[string]string { + return map[string]string{ + "triage": fmt.Sprintf("%s/api/component_readiness/triages/%d", baseURL, triageID), + "force_close": fmt.Sprintf("%s/api/component_readiness/triages/%d/force_close_regressions", baseURL, triageID), + "force_close_preview": fmt.Sprintf("%s/api/component_readiness/triages/%d/force_close_preview", baseURL, triageID), + } +} + +// regressionDetailLink builds the HATEOAS link to a single regression's detail endpoint. +func regressionDetailLink(baseURL string, regressionID uint) map[string]string { + return map[string]string{ + "self": fmt.Sprintf("%s/api/component_readiness/regressions/%d", baseURL, regressionID), + } +} + +// InjectForceCloseHATEOASLinks populates HATEOAS links on a force close result so clients can +// discover the owning triage and the related force close endpoints. +func InjectForceCloseHATEOASLinks(result *ForceCloseResult, baseURL string, triageID uint) { + if result == nil { + return + } + result.Links = forceCloseTriageLinks(baseURL, triageID) + result.Links["self"] = result.Links["force_close"] +} + +// InjectForceClosePreviewHATEOASLinks populates HATEOAS links on a force close preview and on each +// previewed regression (linking to its detail endpoint) for discoverability. +func InjectForceClosePreviewHATEOASLinks(preview *ForceClosePreview, baseURL string) { + if preview == nil { + return + } + preview.Links = forceCloseTriageLinks(baseURL, preview.TriageID) + preview.Links["self"] = preview.Links["force_close_preview"] + for i := range preview.WouldClose { + preview.WouldClose[i].Links = regressionDetailLink(baseURL, preview.WouldClose[i].RegressionID) + } + for i := range preview.WouldNotClose { + preview.WouldNotClose[i].Links = regressionDetailLink(baseURL, preview.WouldNotClose[i].RegressionID) + } +} + // SyncRegressionsForReport compares regressed tests from a component report against known // regressions in the database, opening new ones, reopening recently closed ones, and updating // stats on existing ones. Returns the list of active regressions after sync. diff --git a/pkg/db/models/triage.go b/pkg/db/models/triage.go index d56e7a179..879812b28 100644 --- a/pkg/db/models/triage.go +++ b/pkg/db/models/triage.go @@ -229,6 +229,23 @@ type TestRegression struct { // disappear on their own. MaxFailures int `json:"max_failures"` + // ForceClosed indicates this regression was force closed via a resolved triage. Force closed regressions + // are excluded from the regression reuse window (regressionHysteresisDays) so they are not reopened for + // unrelated failures (TRT-2895). All force close metadata is stored directly on the regression so the + // record is self-contained and needs no join back to the triage. + ForceClosed bool `json:"force_closed" gorm:"not null;default:false"` + + // ForceClosedBy records the user who force closed this regression. It is nil when the regression + // was not force closed (NULL means not applicable). + ForceClosedBy *string `json:"force_closed_by"` + + // ForceClosedReason records the user supplied reason for force closing this regression. It is nil + // when the regression was not force closed (NULL means not applicable). + ForceClosedReason *string `json:"force_closed_reason"` + + // ForceClosedByTriageID references the triage whose resolution drove the force close, if any. + ForceClosedByTriageID *uint `json:"force_closed_by_triage_id"` + // JobRuns accumulates the unique set of all job runs ever observed while this regression was open. // As the 7-day sample window slides, old runs roll off and new ones appear, but this list retains all of them. JobRuns []RegressionJobRun `json:"job_runs,omitempty" gorm:"foreignKey:RegressionID;constraint:OnDelete:CASCADE;"` diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 2b04a63f3..d9db029e5 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1966,6 +1966,98 @@ func (s *Server) jsonUpdateTriage(w http.ResponseWriter, req *http.Request) { api.RespondWithJSON(http.StatusOK, w, triage) } +// forceCloseRegressionsRequest is the request body for the force close regressions endpoint. +type forceCloseRegressionsRequest struct { + // Reason is a required user supplied explanation for force closing the triage's regressions. + Reason string `json:"reason"` +} + +func (s *Server) jsonForceCloseRegressions(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + idStr := vars["id"] + // ParseUint rejects negative and non-numeric IDs before we touch the database. + triageID, err := strconv.ParseUint(idStr, 10, 64) + if err != nil { + failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) + return + } + + user := getUserForRequest(req) + // Force closing records who closed each regression for attribution and audit, so we refuse to + // proceed when we cannot determine the user rather than recording an empty ForceClosedBy. The + // user identity is intentionally not logged. + if strings.TrimSpace(user) == "" { + failureResponse(w, http.StatusUnauthorized, "cannot determine user for force close request; authentication is required") + return + } + + var forceCloseReq forceCloseRegressionsRequest + decoder := json.NewDecoder(req.Body) + if err := decoder.Decode(&forceCloseReq); err != nil { + log.WithError(err).Error("error parsing force close regressions request") + failureResponse(w, http.StatusBadRequest, "invalid request body") + return + } + // Reject any trailing data after the JSON object so bodies like {"reason":"first"}{"reason":"second"} + // are not silently accepted with only the first object honored. A well formed body decodes to exactly + // one value, after which the next decode must report io.EOF. + if err := decoder.Decode(&struct{}{}); err != io.EOF { + failureResponse(w, http.StatusBadRequest, "request body must contain a single JSON object") + return + } + if strings.TrimSpace(forceCloseReq.Reason) == "" { + failureResponse(w, http.StatusBadRequest, "reason is required to force close regressions") + return + } + + tracker := componentreadiness.NewPostgresRegressionStore(s.db, s.jiraClient) + result, err := tracker.ForceCloseRegressions(uint(triageID), user, forceCloseReq.Reason) // nolint:gosec + if err != nil { + if errors.Is(err, componentreadiness.ErrTriageNotResolved) { + failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") + return + } + if errors.Is(err, gorm.ErrRecordNotFound) { + failureResponse(w, http.StatusNotFound, fmt.Sprintf("triage %d not found", triageID)) + return + } + log.WithError(err).Error("error force closing regressions") + failureResponse(w, http.StatusInternalServerError, "failed to force close regressions") + return + } + componentreadiness.InjectForceCloseHATEOASLinks(result, api.GetBaseURL(req), uint(triageID)) // nolint:gosec + api.RespondWithJSON(http.StatusOK, w, result) +} + +func (s *Server) jsonForceClosePreview(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + idStr := vars["id"] + // ParseUint rejects negative and non-numeric IDs before we touch the database. + triageID, err := strconv.ParseUint(idStr, 10, 64) + if err != nil { + failureResponse(w, http.StatusBadRequest, "invalid ID format: "+idStr) + return + } + + tracker := componentreadiness.NewPostgresRegressionStore(s.db, s.jiraClient) + preview, err := tracker.ForceClosePreview(uint(triageID)) // nolint:gosec + if err != nil { + if errors.Is(err, componentreadiness.ErrTriageNotResolved) { + failureResponse(w, http.StatusBadRequest, "Cannot force-close regressions for an unresolved triage. Resolve the triage first.") + return + } + if errors.Is(err, gorm.ErrRecordNotFound) { + failureResponse(w, http.StatusNotFound, fmt.Sprintf("triage %d not found", triageID)) + return + } + log.WithError(err).Error("error building force close preview") + failureResponse(w, http.StatusInternalServerError, "failed to build force close preview") + return + } + componentreadiness.InjectForceClosePreviewHATEOASLinks(preview, api.GetBaseURL(req)) + api.RespondWithJSON(http.StatusOK, w, preview) +} + func (s *Server) jsonDeleteTriage(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) idStr := vars["id"] @@ -2876,6 +2968,20 @@ func (s *Server) Serve() { Capabilities: []string{LocalDBCapability, ComponentReadinessCapability, WriteEndpointsCapability}, HandlerFunc: s.jsonDeleteTriage, }, + { + EndpointPath: "/api/component_readiness/triages/{id}/force_close_regressions", + Description: "Force close the open regressions that existed at a resolved triage's resolution time so they are excluded from the regression reuse window", + Methods: []string{http.MethodPost}, + Capabilities: []string{LocalDBCapability, ComponentReadinessCapability, WriteEndpointsCapability}, + HandlerFunc: s.jsonForceCloseRegressions, + }, + { + EndpointPath: "/api/component_readiness/triages/{id}/force_close_preview", + Description: "Preview which regressions would be force closed for a resolved triage, with failure gap data, without modifying anything", + Methods: []string{http.MethodGet}, + Capabilities: []string{LocalDBCapability, ComponentReadinessCapability}, + HandlerFunc: s.jsonForceClosePreview, + }, { EndpointPath: "/api/component_readiness/triages/{id}/matches", Description: "List potential matching regressions for a given triage.", diff --git a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go index 6a9debe42..c2381dca2 100644 --- a/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go +++ b/test/e2e/componentreadiness/regressiontracker/regressiontracker_test.go @@ -3,6 +3,8 @@ package regressiontracker import ( "context" "database/sql" + "errors" + "fmt" "testing" "time" @@ -20,6 +22,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gorm.io/gorm" "k8s.io/apimachinery/pkg/util/sets" ) @@ -273,6 +276,88 @@ func Test_RegressionTracker(t *testing.T) { } +func Test_ForceCloseRegressions(t *testing.T) { + dbc := util.CreateE2EPostgresConnection(t) + tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + createTriageForRegressions := func(t *testing.T, url string, regs ...*models.TestRegression) models.Triage { + t.Helper() + triage := models.Triage{ + URL: url, + Description: "force close triage", + Type: models.TriageTypeProduct, + } + dbWithContext := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + // Create the triage first without regressions, then link the existing + // regressions via the association API. Embedding the regressions in the + // Create() call makes GORM upsert them, which writes the partial objects + // (nil Variants) and violates the NOT NULL constraint on variants. + require.NoError(t, dbWithContext.Create(&triage).Error) + require.NoError(t, dbWithContext.Model(&triage).Association("Regressions").Append(regs)) + return triage + } + + createResolvedTriageForRegressions := func(t *testing.T, url string, resolved time.Time, regs ...*models.TestRegression) models.Triage { + t.Helper() + triage := createTriageForRegressions(t, url, regs...) + dbWithContext := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "e2e-test")) + require.NoError(t, dbWithContext.Model(&triage).Update("resolved", sql.NullTime{Valid: true, Time: resolved}).Error) + return triage + } + + cleanup := func() { + assert.NoError(t, cleanupTriages(dbc)) + cleanupAllRegressions(dbc) + } + + t.Run("records force close details on each regression", func(t *testing.T) { + defer cleanup() + + resolved := time.Now().Truncate(time.Second) + reg, err := rawCreateRegression(dbc, "4.19", "fc-fields", "force close test fc-fields", + []string{"a:b"}, resolved.Add(-10*24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createResolvedTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-1", resolved, reg) + + result, err := tracker.ForceCloseRegressions(triage.ID, "developer", "generic test, unrelated failures") + require.NoError(t, err) + require.NotNil(t, result) + assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) + assert.WithinDuration(t, resolved, result.Timestamp, time.Second, "timestamp should be the resolution time") + + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.True(t, checkReg.Closed.Valid, "regression should be closed") + assert.WithinDuration(t, resolved, checkReg.Closed.Time, time.Second, "regression should close at the resolution time") + assert.True(t, checkReg.ForceClosed, "regression should be force closed") + require.NotNil(t, checkReg.ForceClosedBy, "regression should record who force closed it") + assert.Equal(t, "developer", *checkReg.ForceClosedBy, "regression should record who force closed it") + require.NotNil(t, checkReg.ForceClosedReason, "regression should record the reason") + assert.Equal(t, "generic test, unrelated failures", *checkReg.ForceClosedReason, "regression should record the reason") + require.NotNil(t, checkReg.ForceClosedByTriageID) + assert.Equal(t, triage.ID, *checkReg.ForceClosedByTriageID) + }) + + t.Run("returns ErrTriageNotResolved for an unresolved triage", func(t *testing.T) { + defer cleanup() + + reg, err := rawCreateRegression(dbc, "4.19", "fc-unresolved", "force close test fc-unresolved", + []string{"a:b"}, time.Now().Add(-10*24*time.Hour), time.Time{}) + require.NoError(t, err) + triage := createTriageForRegressions(t, "https://redhat.atlassian.net/browse/TEST-FC-UNRESOLVED", reg) + + _, err = tracker.ForceCloseRegressions(triage.ID, "developer", "should fail") + require.Error(t, err) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + + // The regression must remain untouched. + var checkReg models.TestRegression + require.NoError(t, dbc.DB.First(&checkReg, reg.ID).Error) + assert.False(t, checkReg.Closed.Valid, "regression should remain open") + assert.False(t, checkReg.ForceClosed, "regression should not be force closed") + }) +} + func cleanupJobRuns(dbc *db.DB) { res := dbc.DB.Where("1 = 1").Delete(&models.RegressionJobRun{}) if res.Error != nil { @@ -516,9 +601,19 @@ func Test_RegressionJobRuns(t *testing.T) { }) } -func cleanupTriages(dbc *db.DB) { - dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1") - dbc.DB.Where("1 = 1").Delete(&models.Triage{}) +// cleanupTriages removes triage_regressions join rows before the triages so no association outlives a +// referenced row. It returns an error rather than only logging so a deferred cleanup can assert on it and +// surface leaks instead of silently letting rows bleed into later tests. +func cleanupTriages(dbc *db.DB) error { + // gorm.ErrRecordNotFound simply means there was nothing to delete, which is a valid + // cleanup state rather than a failure, so it is filtered out on each step. + if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { + return fmt.Errorf("error deleting triage_regressions: %w", err) + } + if err := dbc.DB.Where("1 = 1").Delete(&models.Triage{}).Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { + return fmt.Errorf("error deleting triage records: %w", err) + } + return nil } func Test_SyncTriageSymptoms(t *testing.T) { @@ -557,7 +652,7 @@ func Test_SyncTriageSymptoms(t *testing.T) { cleanup := func() { util.CleanupTriageSymptoms(dbc) cleanupJobRuns(dbc) - cleanupTriages(dbc) + assert.NoError(t, cleanupTriages(dbc)) cleanupAllRegressions(dbc) } diff --git a/test/e2e/componentreadiness/triage/triageapi_test.go b/test/e2e/componentreadiness/triage/triageapi_test.go index 64cdd6565..615f9ef07 100644 --- a/test/e2e/componentreadiness/triage/triageapi_test.go +++ b/test/e2e/componentreadiness/triage/triageapi_test.go @@ -41,8 +41,11 @@ var view = crview.View{ } 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") + // Delete the triage_regressions join rows before the triages so no association outlives a + // referenced row. Errors are logged rather than swallowed so cleanup failures are visible. + if err := dbc.DB.Exec("DELETE FROM triage_regressions WHERE 1=1").Error; err != nil { + log.Errorf("error deleting triage_regressions: %v", err) + } res := dbc.DB.Where("1 = 1").Delete(&models.Triage{}) if res.Error != nil { log.Errorf("error deleting triage records: %v", res.Error) @@ -727,6 +730,154 @@ func Test_TriageAPI(t *testing.T) { } +func Test_ForceCloseRegressionsAPI(t *testing.T) { + dbc := util.CreateE2EPostgresConnection(t) + tracker := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + jiraBug := createBug(t, dbc.DB) + defer func() { + if err := dbc.DB.Delete(jiraBug).Error; err != nil { + log.Errorf("error deleting jira bug: %v", err) + } + }() + + release := view.SampleRelease.Name + + // cleanupForceClose removes triage associations before the given regressions so the + // triage_regressions -> test_regressions FK ordering is respected (deleting a regression while a + // join row still references it would fail). It checks each delete so cleanup failures surface + // instead of silently leaking rows into later subtests. Defer it once per subtest; do not also + // defer a bare regression delete, which would run first (LIFO) and hit the FK. + cleanupForceClose := func(regs ...*models.TestRegression) { + cleanupAllTriages(dbc) + for _, reg := range regs { + if reg == nil { + continue + } + if err := dbc.DB.Where("test_regression_id = ?", reg.ID).Delete(&models.RegressionView{}).Error; err != nil { + log.Errorf("error deleting regression views for %d: %v", reg.ID, err) + } + if err := dbc.DB.Delete(reg).Error; err != nil { + log.Errorf("error deleting test regression %d: %v", reg.ID, err) + } + } + } + + // createRawRegression creates a regression with a caller-controlled opened time so tests can exercise + // the resolution-time scoping. + createRawRegression := func(t *testing.T, testID string, opened time.Time) *models.TestRegression { + t.Helper() + reg := &models.TestRegression{ + Release: release, + TestID: testID, + TestName: "force close api test " + testID, + Variants: pq.StringArray{"a:b"}, + Opened: opened, + } + require.NoError(t, dbc.DB.Create(reg).Error) + return reg + } + + // resolveTriage resolves a triage via the API so force close has a resolution time to scope against. + resolveTriage := func(t *testing.T, triageResp models.Triage, resolved time.Time) { + t.Helper() + triageResp.Resolved = sql.NullTime{Valid: true, Time: resolved} + var updated models.Triage + require.NoError(t, util.SippyPut(fmt.Sprintf("/api/component_readiness/triages/%d", triageResp.ID), &triageResp, &updated)) + } + + t.Run("force close on an unresolved triage is rejected", func(t *testing.T) { + reg := createTestRegression(t, tracker, view, "fc-api-unresolved") + defer cleanupForceClose(reg) + triageResponse := createAndValidateTriageRecord(t, jiraBug.URL, reg) + + // Force closing an unresolved triage must be rejected. + var result componentreadiness.ForceCloseResult + err := util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResponse.ID), + &map[string]string{"reason": "should be rejected"}, &result) + require.Error(t, err, "force closing an unresolved triage should fail") + + // Preview of an unresolved triage is likewise rejected. + var preview componentreadiness.ForceClosePreview + err = util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_preview", triageResponse.ID), &preview) + require.Error(t, err, "previewing an unresolved triage should fail") + }) + + t.Run("create, resolve, preview, force close, and verify over the API", func(t *testing.T) { + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + + // One regression opened before the resolution time (force close should close it) and one opened + // after (force close should leave it open), so the happy path exercises resolution-time scoping. + wouldClose := createRawRegression(t, "fc-api-close", resolved.Add(-10*24*time.Hour)) + wouldNotClose := createRawRegression(t, "fc-api-open", resolved.Add(24*time.Hour)) + defer cleanupForceClose(wouldClose, wouldNotClose) + // Associate the closing regression with a view so the regression detail endpoint can build links. + require.NoError(t, tracker.UpsertRegressionView(wouldClose.ID, view.Name)) + + // Failures before and after the resolution time drive the preview gap indicator. + require.NoError(t, tracker.MergeJobRuns(wouldClose.ID, []models.RegressionJobRun{ + {ProwJobRunID: "api-before", ProwJobName: "job-1", StartTime: resolved.Add(-2 * 24 * time.Hour), TestFailed: true}, + {ProwJobRunID: "api-after", ProwJobName: "job-1", StartTime: resolved.Add(2 * 24 * time.Hour), TestFailed: true}, + })) + + // Create the triage over both regressions, then resolve it via the API. + triage := models.Triage{ + URL: jiraBug.URL, + Type: models.TriageTypeProduct, + Regressions: []models.TestRegression{ + {ID: wouldClose.ID}, + {ID: wouldNotClose.ID}, + }, + } + var triageResp models.Triage + require.NoError(t, util.SippyPost("/api/component_readiness/triages", &triage, &triageResp)) + resolveTriage(t, triageResp, resolved) + + // Preview classifies the regressions and reports the failure gap around the resolution time. + var preview componentreadiness.ForceClosePreview + require.NoError(t, util.SippyGet(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_preview", triageResp.ID), &preview)) + require.Len(t, preview.WouldClose, 1, "regression opened before resolution should be in would_close") + assert.Equal(t, wouldClose.ID, preview.WouldClose[0].RegressionID) + require.NotNil(t, preview.WouldClose[0].LastFailureBeforeResolution, "should report last failure before resolution") + assert.WithinDuration(t, resolved.Add(-2*24*time.Hour), *preview.WouldClose[0].LastFailureBeforeResolution, time.Second) + require.NotNil(t, preview.WouldClose[0].FirstFailureAfterResolution, "should report first failure after resolution") + assert.WithinDuration(t, resolved.Add(2*24*time.Hour), *preview.WouldClose[0].FirstFailureAfterResolution, time.Second) + require.Len(t, preview.WouldNotClose, 1, "regression opened after resolution should be in would_not_close") + assert.Equal(t, wouldNotClose.ID, preview.WouldNotClose[0].RegressionID) + + // Force close over the API and confirm exactly the eligible regression closed. + var result componentreadiness.ForceCloseResult + require.NoError(t, util.SippyPost(fmt.Sprintf("/api/component_readiness/triages/%d/force_close_regressions", triageResp.ID), + &map[string]string{"reason": "generic test, unrelated failures"}, &result)) + assert.ElementsMatch(t, []uint{wouldClose.ID}, result.ClosedRegressionIDs) + assert.False(t, result.Timestamp.IsZero()) + + // The force closed regression is excluded from the reuse list. + regressions, err := tracker.ListCurrentRegressionsForRelease(release) + require.NoError(t, err) + for _, r := range regressions { + assert.NotEqual(t, wouldClose.ID, r.ID, "force closed regression should not appear in reuse list") + } + + // Force close metadata is recorded on the regression row and surfaced by the detail endpoint. + var detail models.TestRegression + require.NoError(t, util.SippyGet(fmt.Sprintf("/api/component_readiness/regressions/%d", wouldClose.ID), &detail)) + assert.True(t, detail.ForceClosed, "regression detail should report force_closed") + require.NotNil(t, detail.ForceClosedByTriageID) + assert.Equal(t, triageResp.ID, *detail.ForceClosedByTriageID) + require.NotNil(t, detail.ForceClosedBy, "detail should include force_closed_by directly from the regression") + assert.Equal(t, "developer", *detail.ForceClosedBy) + require.NotNil(t, detail.ForceClosedReason, "detail should include force_closed_reason directly from the regression") + assert.Equal(t, "generic test, unrelated failures", *detail.ForceClosedReason) + + // The regression opened after the resolution time was left open. + var openReg models.TestRegression + require.NoError(t, dbc.DB.First(&openReg, wouldNotClose.ID).Error) + assert.False(t, openReg.Closed.Valid, "regression opened after resolution should remain open") + assert.False(t, openReg.ForceClosed, "regression opened after resolution should not be force closed") + }) +} + func Test_RegressionAPI(t *testing.T) { dbc := util.CreateE2EPostgresConnection(t) // jiraClient is intentionally nil to prevent commenting on jiras diff --git a/test/integration/regression_forceclose_test.go b/test/integration/regression_forceclose_test.go new file mode 100644 index 000000000..07c891872 --- /dev/null +++ b/test/integration/regression_forceclose_test.go @@ -0,0 +1,317 @@ +package integration + +import ( + "database/sql" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/openshift/sippy/pkg/api/componentreadiness" + "github.com/openshift/sippy/pkg/db/models" + intutil "github.com/openshift/sippy/test/integration/util" +) + +// These tests exercise the force close regression store methods against a real PostgreSQL clone. +// pgContainer and TestMain live in jobs_test.go (same package), so each test just calls +// intutil.NewTestDB(t, pgContainer) for an isolated database. + +func TestForceCloseRegressions(t *testing.T) { + t.Run("basic close records force close metadata on the regression", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "basic-close", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/BASIC-1", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + result, err := store.ForceCloseRegressions(triage.ID, "developer", "unrelated failures") + require.NoError(t, err) + require.NotNil(t, result) + assert.ElementsMatch(t, []uint{reg.ID}, result.ClosedRegressionIDs) + assert.WithinDuration(t, resolved, result.Timestamp, time.Second, "close time should be the resolution time") + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.True(t, got.Closed.Valid, "regression should be closed") + assert.WithinDuration(t, resolved, got.Closed.Time, time.Second, "regression should close at the resolution time") + assert.True(t, got.ForceClosed) + require.NotNil(t, got.ForceClosedBy) + assert.Equal(t, "developer", *got.ForceClosedBy) + require.NotNil(t, got.ForceClosedReason) + assert.Equal(t, "unrelated failures", *got.ForceClosedReason) + require.NotNil(t, got.ForceClosedByTriageID) + assert.Equal(t, triage.ID, *got.ForceClosedByTriageID) + }) + + t.Run("time scoping closes only regressions opened before the resolution time", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + before := intutil.CreateTestRegression(t, dbc, "scope-before", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + // Opened at the exact resolution instant: the exclusive boundary (opened < resolved) means it + // must NOT be closed. + atBoundary := intutil.CreateTestRegression(t, dbc, "scope-boundary", "4.19", + intutil.WithOpened(resolved)) + after := intutil.CreateTestRegression(t, dbc, "scope-after", "4.19", + intutil.WithOpened(resolved.Add(24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/SCOPE-1", + intutil.WithResolved(resolved), intutil.WithRegressions(before, atBoundary, after)) + + result, err := store.ForceCloseRegressions(triage.ID, "developer", "scoped close") + require.NoError(t, err) + assert.ElementsMatch(t, []uint{before.ID}, result.ClosedRegressionIDs, + "only the regression opened before the resolution time should be closed") + + var gotBefore, gotBoundary, gotAfter models.TestRegression + require.NoError(t, dbc.DB.First(&gotBefore, before.ID).Error) + require.NoError(t, dbc.DB.First(&gotBoundary, atBoundary.ID).Error) + require.NoError(t, dbc.DB.First(&gotAfter, after.ID).Error) + + assert.True(t, gotBefore.ForceClosed, "regression opened before resolution should be force closed") + assert.WithinDuration(t, resolved, gotBefore.Closed.Time, time.Second) + + assert.False(t, gotBoundary.Closed.Valid, "regression opened exactly at resolution should remain open") + assert.False(t, gotBoundary.ForceClosed, "boundary regression should not be force closed") + + assert.False(t, gotAfter.Closed.Valid, "regression opened after resolution should remain open") + assert.False(t, gotAfter.ForceClosed) + }) + + t.Run("unresolved triage returns ErrTriageNotResolved and touches nothing", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + reg := intutil.CreateTestRegression(t, dbc, "guard-open", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/GUARD-1", intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", "should fail") + require.Error(t, err) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.False(t, got.Closed.Valid, "regression should remain open") + assert.False(t, got.ForceClosed, "regression should not be force closed") + }) + + t.Run("idempotent repeat call closes nothing new and leaves the close time unchanged", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "idem", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/IDEM-1", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + first, err := store.ForceCloseRegressions(triage.ID, "developer", "first call") + require.NoError(t, err) + require.Len(t, first.ClosedRegressionIDs, 1) + + var afterFirst models.TestRegression + require.NoError(t, dbc.DB.First(&afterFirst, reg.ID).Error) + originalClose := afterFirst.Closed.Time + + second, err := store.ForceCloseRegressions(triage.ID, "developer", "second call") + require.NoError(t, err) + assert.Empty(t, second.ClosedRegressionIDs, "repeat call should close no additional regressions") + + var afterSecond models.TestRegression + require.NoError(t, dbc.DB.First(&afterSecond, reg.ID).Error) + assert.WithinDuration(t, originalClose, afterSecond.Closed.Time, time.Second, + "closed time should not change on a repeat call") + }) +} + +func TestForceClosePreview(t *testing.T) { + t.Run("classifies would-close and would-not-close with failure gap data", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Add(-5 * 24 * time.Hour).Truncate(time.Second) + + wouldClose := intutil.CreateTestRegression(t, dbc, "prev-close", "4.19", + intutil.WithOpened(resolved.Add(-10*24*time.Hour))) + // Opened exactly at the resolution instant belongs in would_not_close (exclusive boundary). + atBoundary := intutil.CreateTestRegression(t, dbc, "prev-boundary", "4.19", + intutil.WithOpened(resolved)) + wouldNotClose := intutil.CreateTestRegression(t, dbc, "prev-open", "4.19", + intutil.WithOpened(resolved.Add(24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/PREV-1", + intutil.WithResolved(resolved), intutil.WithRegressions(wouldClose, atBoundary, wouldNotClose)) + + // Failing runs before and after the resolution time drive the gap indicator on the closing regression. + lastBefore := resolved.Add(-2 * 24 * time.Hour) + firstAfter := resolved.Add(2 * 24 * time.Hour) + require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ + RegressionID: wouldClose.ID, ProwJobRunID: "gap-before", ProwJobName: "job-1", StartTime: lastBefore, TestFailed: true, + }).Error) + require.NoError(t, dbc.DB.Create(&models.RegressionJobRun{ + RegressionID: wouldClose.ID, ProwJobRunID: "gap-after", ProwJobName: "job-1", StartTime: firstAfter, TestFailed: true, + }).Error) + + preview, err := store.ForceClosePreview(triage.ID) + require.NoError(t, err) + assert.WithinDuration(t, resolved, preview.Resolved, time.Second) + + require.Len(t, preview.WouldClose, 1, "only the regression opened before resolution should be in would_close") + assert.Equal(t, wouldClose.ID, preview.WouldClose[0].RegressionID) + require.NotNil(t, preview.WouldClose[0].LastFailureBeforeResolution, "should report the last failure before resolution") + assert.WithinDuration(t, lastBefore, *preview.WouldClose[0].LastFailureBeforeResolution, time.Second) + require.NotNil(t, preview.WouldClose[0].FirstFailureAfterResolution, "should report the first failure after resolution") + assert.WithinDuration(t, firstAfter, *preview.WouldClose[0].FirstFailureAfterResolution, time.Second) + + notCloseIDs := make([]uint, 0, len(preview.WouldNotClose)) + for _, r := range preview.WouldNotClose { + notCloseIDs = append(notCloseIDs, r.RegressionID) + } + assert.ElementsMatch(t, []uint{atBoundary.ID, wouldNotClose.ID}, notCloseIDs, + "regressions opened at or after resolution should be in would_not_close") + }) + + t.Run("unresolved triage returns ErrTriageNotResolved", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + reg := intutil.CreateTestRegression(t, dbc, "prev-guard", "4.19") + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/PREV-GUARD", intutil.WithRegressions(reg)) + + _, err := store.ForceClosePreview(triage.ID) + assert.ErrorIs(t, err, componentreadiness.ErrTriageNotResolved) + }) +} + +func TestListCurrentRegressionsForReleaseForceClose(t *testing.T) { + t.Run("force closed regressions are excluded from the reuse window", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + now := time.Now() + recentlyClosed := now.Add(-24 * time.Hour) // within the 5-day hysteresis reuse window + by := "developer" + reason := "excluded from reuse" + + open := intutil.CreateTestRegression(t, dbc, "list-open", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour))) + recentNormal := intutil.CreateTestRegression(t, dbc, "list-recent-normal", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour)), intutil.WithClosed(recentlyClosed)) + forceClosed := intutil.CreateTestRegression(t, dbc, "list-forceclosed", "4.19", + intutil.WithOpened(now.Add(-10*24*time.Hour)), intutil.WithClosed(recentlyClosed), + intutil.WithForceClosed(true), intutil.WithForceClosedBy(&by), intutil.WithForceClosedReason(&reason)) + oldClosed := intutil.CreateTestRegression(t, dbc, "list-old-closed", "4.19", + intutil.WithOpened(now.Add(-30*24*time.Hour)), intutil.WithClosed(now.Add(-10*24*time.Hour))) + otherRelease := intutil.CreateTestRegression(t, dbc, "list-other-release", "4.18", + intutil.WithOpened(now.Add(-24*time.Hour))) + + got, err := store.ListCurrentRegressionsForRelease("4.19") + require.NoError(t, err) + + gotIDs := make([]uint, 0, len(got)) + for _, r := range got { + gotIDs = append(gotIDs, r.ID) + } + assert.Contains(t, gotIDs, open.ID, "open regression should be listed") + assert.Contains(t, gotIDs, recentNormal.ID, "recently closed (non-force) regression should be within the reuse window") + assert.NotContains(t, gotIDs, forceClosed.ID, "force closed regression should be excluded even though it closed recently") + assert.NotContains(t, gotIDs, oldClosed.ID, "regression closed beyond the hysteresis window should be excluded") + assert.NotContains(t, gotIDs, otherRelease.ID, "regression from another release should be excluded") + }) +} + +func TestResolveTriagesForceClose(t *testing.T) { + t.Run("force closed regressions do not block triage auto-resolution", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + // Closed recently, well within the 5-day hysteresis window: a non-force close would still block + // auto-resolution, but a force close should not. + recentlyClosed := time.Now().Add(-1 * time.Hour).Truncate(time.Second) + + // Triage A: its only regression was force closed. Force closed regressions no longer count as + // active, so the triage should auto-resolve despite the recent close. + forceClosedReg := intutil.CreateTestRegression(t, dbc, "resolve-forceclosed", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triageA := intutil.CreateTriage(t, dbc, "https://issues.example.com/RESOLVE-A", + intutil.WithRegressions(forceClosedReg)) + require.NoError(t, dbc.DB.Model(&models.TestRegression{}).Where("id = ?", forceClosedReg.ID). + Updates(map[string]any{ + "closed": sql.NullTime{Valid: true, Time: recentlyClosed}, + "force_closed": true, + "force_closed_by": "developer", + "force_closed_reason": "force closed", + }).Error) + + // Triage B: its only regression was closed recently but NOT force closed, so it still blocks + // auto-resolution until the hysteresis window passes. + normalReg := intutil.CreateTestRegression(t, dbc, "resolve-normal", "4.19", + intutil.WithOpened(time.Now().Add(-10*24*time.Hour))) + triageB := intutil.CreateTriage(t, dbc, "https://issues.example.com/RESOLVE-B", + intutil.WithRegressions(normalReg)) + require.NoError(t, dbc.DB.Model(&models.TestRegression{}).Where("id = ?", normalReg.ID). + Update("closed", sql.NullTime{Valid: true, Time: recentlyClosed}).Error) + + require.NoError(t, store.ResolveTriages()) + + var gotA, gotB models.Triage + require.NoError(t, dbc.DB.First(&gotA, triageA.ID).Error) + require.NoError(t, dbc.DB.First(&gotB, triageB.ID).Error) + + assert.True(t, gotA.Resolved.Valid, "triage with only a force closed regression should be auto-resolved") + assert.WithinDuration(t, recentlyClosed, gotA.Resolved.Time, time.Second, + "resolution time should match the regression close time") + assert.False(t, gotB.Resolved.Valid, + "triage with a recently, non-force closed regression should still be blocked") + }) +} + +func TestForceCloseRegressionsErrorPaths(t *testing.T) { + t.Run("nonexistent triage returns a not found error", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + _, err := store.ForceCloseRegressions(999999, "developer", "valid reason") + require.Error(t, err) + assert.ErrorIs(t, err, gorm.ErrRecordNotFound) + }) + + t.Run("empty reason is rejected and closes nothing", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "empty-reason", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/REASON-EMPTY", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", "") + assert.ErrorIs(t, err, componentreadiness.ErrForceCloseReasonRequired) + + var got models.TestRegression + require.NoError(t, dbc.DB.First(&got, reg.ID).Error) + assert.False(t, got.Closed.Valid, "regression must not be closed when the reason is rejected") + assert.False(t, got.ForceClosed) + }) + + t.Run("whitespace only reason is rejected", func(t *testing.T) { + dbc := intutil.NewTestDB(t, pgContainer) + store := componentreadiness.NewPostgresRegressionStore(dbc, nil) + + resolved := time.Now().Truncate(time.Second) + reg := intutil.CreateTestRegression(t, dbc, "ws-reason", "4.19", + intutil.WithOpened(resolved.Add(-24*time.Hour))) + triage := intutil.CreateTriage(t, dbc, "https://issues.example.com/REASON-WS", + intutil.WithResolved(resolved), intutil.WithRegressions(reg)) + + _, err := store.ForceCloseRegressions(triage.ID, "developer", " ") + assert.ErrorIs(t, err, componentreadiness.ErrForceCloseReasonRequired) + }) +} diff --git a/test/integration/util/fixtures.go b/test/integration/util/fixtures.go index 8c28f1477..fadba71e4 100644 --- a/test/integration/util/fixtures.go +++ b/test/integration/util/fixtures.go @@ -1,6 +1,8 @@ package util import ( + "context" + "database/sql" "fmt" "testing" "time" @@ -365,3 +367,101 @@ func LinkReleaseTagPullRequests(t *testing.T, dbc *db.DB, tag *models.ReleaseTag } require.NoError(t, dbc.DB.Model(tag).Association("PullRequests").Append(prPtrs), "linking PRs to ReleaseTag %q", tag.ReleaseTag) } + +// TestRegressionOption customizes a TestRegression before creation. +type TestRegressionOption func(*models.TestRegression) + +// WithOpened sets the regression's opened time. Force close scopes on this, closing only regressions +// that opened strictly before the triage's resolution time. +func WithOpened(opened time.Time) TestRegressionOption { + return func(r *models.TestRegression) { r.Opened = opened } +} + +// WithClosed marks the regression closed at the given time. +func WithClosed(closed time.Time) TestRegressionOption { + return func(r *models.TestRegression) { r.Closed = sql.NullTime{Valid: true, Time: closed} } +} + +// WithForceClosed sets the force_closed flag directly, for seeding already-force-closed regressions. +func WithForceClosed(forceClosed bool) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosed = forceClosed } +} + +// WithForceClosedBy sets who force closed the regression. +func WithForceClosedBy(by *string) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosedBy = by } +} + +// WithForceClosedReason sets the recorded force close reason. +func WithForceClosedReason(reason *string) TestRegressionOption { + return func(r *models.TestRegression) { r.ForceClosedReason = reason } +} + +// CreateTestRegression creates a test_regressions row with the required non-null fields populated +// (TestName, TestID, Release, and a non-null Variants array), defaulting Opened to now. Callers +// override any field with the With* options. +func CreateTestRegression(t *testing.T, dbc *db.DB, testName, release string, opts ...TestRegressionOption) *models.TestRegression { + t.Helper() + reg := &models.TestRegression{ + TestName: testName, + TestID: testName + "-id", + Release: release, + Variants: pq.StringArray{"variant:integration"}, + Opened: time.Now(), + } + for _, opt := range opts { + opt(reg) + } + require.NoError(t, dbc.DB.Create(reg).Error, "creating TestRegression %q", testName) + return reg +} + +// triageConfig collects the desired triage state plus the regressions to associate after creation. +type triageConfig struct { + triage models.Triage + regressions []*models.TestRegression +} + +// TriageOption customizes a Triage before creation. +type TriageOption func(*triageConfig) + +// WithResolved marks the triage resolved at the given time, giving force close a resolution time to +// scope against. +func WithResolved(resolved time.Time) TriageOption { + return func(c *triageConfig) { c.triage.Resolved = sql.NullTime{Valid: true, Time: resolved} } +} + +// WithTriageType overrides the triage type (defaults to product). +func WithTriageType(triageType string) TriageOption { + return func(c *triageConfig) { c.triage.Type = models.TriageType(triageType) } +} + +// WithRegressions links the given existing regressions to the triage. The link is made via the GORM +// Association API after the triage is created, never by embedding the regressions in the Create call: +// embedding makes GORM upsert them, rewriting partial in-memory objects (for example nil Variants) and +// violating the NOT NULL constraint on variants. +func WithRegressions(regs ...*models.TestRegression) TriageOption { + return func(c *triageConfig) { c.regressions = append(c.regressions, regs...) } +} + +// CreateTriage creates a triage row and links any requested regressions via the Association API. The +// triage is created under a user context so the audit-log AfterCreate hook has a user to attribute. +func CreateTriage(t *testing.T, dbc *db.DB, url string, opts ...TriageOption) models.Triage { + t.Helper() + cfg := &triageConfig{ + triage: models.Triage{ + URL: url, + Type: models.TriageTypeProduct, + }, + } + for _, opt := range opts { + opt(cfg) + } + dbCtx := dbc.DB.WithContext(context.WithValue(context.Background(), models.CurrentUserKey, "integration-test")) + require.NoError(t, dbCtx.Create(&cfg.triage).Error, "creating Triage %q", url) + if len(cfg.regressions) > 0 { + require.NoError(t, dbCtx.Model(&cfg.triage).Association("Regressions").Append(cfg.regressions), + "linking regressions to Triage %q", url) + } + return cfg.triage +}