TRT-2509: remove the view column from TestRegression#3236
TRT-2509: remove the view column from TestRegression#3236openshift-merge-bot[bot] merged 9 commits intoopenshift:mainfrom
view column from TestRegression#3236Conversation
… to allow for regression tracking from multiple views targeting the same release
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughSwitches regression tracking from a view-scoped model to a release-scoped model: DB drops the Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API_Server
participant Reports as Metrics/Reports
participant DB as Database
Client->>API: Request sync/list for release (base+sample)
activate API
API->>API: SyncRegressionsForRelease(release)
loop per-view in release
API->>Reports: Query component report for view (sample/base)
activate Reports
Reports-->>API: Report data
deactivate Reports
API->>DB: ListRegressions(release)
activate DB
DB-->>API: current regressions
deactivate DB
API->>API: FindOpenRegression(sampleRelease, testID, variants, regressions)
API->>DB: CreateOrUpdate regression (release-scoped)
end
API->>DB: ResolveTriages() / Close inactive regressions
activate DB
DB-->>API: acknowledgement
deactivate DB
API-->>Client: Return regressions (release-scoped, per-view map)
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sippy-ng/src/component_readiness/Triage.js (1)
149-149:⚠️ Potential issue | 🟡 MinorMissing
viewin useEffect dependency array.The
viewvariable is used within the effect (lines 115, 130, 136) but is not included in the dependency array. This could cause stale values to be used in the context data.- }, [isLoaded, triage, setPageContextForChat, unsetPageContextForChat]) + }, [isLoaded, triage, view, setPageContextForChat, unsetPageContextForChat])sippy-ng/src/component_readiness/RegressedTestsModal.js (1)
130-136:⚠️ Potential issue | 🔴 CriticalType mismatch:
allRegressedTestsobject passed where array expected.At line 132,
allRegressedTests(defined asPropTypes.objectat line 153) is passed toRegressedTestsPanelas theregressedTestsprop, which expectsPropTypes.array(line 392 of RegressedTestsPanel.js). This will cause errors when RegressedTestsPanel attempts to filter and search the data at lines 292 and 344, where it treatsregressedTestsas an array.Ensure
allRegressedTestsis converted to an array format before passing it toRegressedTestsPanel, or verify the PropTypes and data structure are aligned across both components.pkg/api/componentreadiness/regressiontracker.go (1)
403-413:⚠️ Potential issue | 🔴 CriticalNew regressions are not marked active, so they can be closed immediately.
SyncRegressionsForReportonly returns matched existing regressions. Newly opened regressions never enteractiveRegressions, so the close pass can close them in the same run.🛠️ Suggested fix
if !rt.dryRun { // Open a new regression: newReg, err := rt.backend.OpenRegression(view, regTest) if err != nil { rLog.WithError(err).Errorf("error opening new regression for: %v", regTest) return nil, errors.Wrapf(err, "error opening new regression: %v", regTest) } rLog.Infof("new regression opened with id: %d", newReg.ID) + matchedOpenRegressions = append(matchedOpenRegressions, newReg) }
🤖 Fix all issues with AI agents
In `@pkg/api/componentreadiness/regressiontracker.go`:
- Around line 227-309: The code uses the cumulative rt.errors slice to decide
whether to run per-release closing/resolving logic, which prevents later
releases from being processed if earlier releases had errors; change this by
creating a per-release error tracker (e.g., a local slice or boolean like
perReleaseHasErrors) inside the loop that processes each release: when you catch
errors from GetViewReleaseOptions, GetComponentReportFromBigQuery,
SyncRegressionsForReport, etc., still append them to rt.errors for global
reporting but also set/per-record the per-release tracker; then use that
per-release tracker (instead of len(rt.errors) == 0) to decide whether to run
ListCurrentRegressionsForRelease, UpdateRegression, and ResolveTriages for that
specific release (keep variable names activeRegressions, closedRegs, now, and
calls to rt.backend.ListCurrentRegressionsForRelease,
rt.backend.UpdateRegression, rt.backend.ResolveTriages the same).
In `@pkg/db/db.go`:
- Around line 106-112: The log message is incorrect: when checking and dropping
the "view" column you reference the wrong model in the log; update the log.Info
call that currently says "dropping Triage.View column" to accurately reference
the TestRegression model (e.g., "dropping TestRegression.view column") so the
message matches the actions in the block that uses
d.DB.Migrator().HasColumn(&models.TestRegression{}, "view") and
d.DB.Migrator().DropColumn(&models.TestRegression{}, "view").
In `@pkg/sippyserver/server.go`:
- Around line 1617-1649: The handler validates sampleRelease but not
baseRelease, causing getRegressedTestsForRegressions to surface a 500 when
baseRelease is missing; add validation for baseRelease (e.g., baseRelease :=
req.URL.Query().Get("baseRelease")) and if it's empty return failureResponse(w,
http.StatusBadRequest, "no baseRelease provided") before calling
componentreadiness.ListRegressions or s.getRegressedTestsForRegressions so both
required params (baseRelease and sampleRelease) are checked; update references
in the function handling triage (triage variable, regressions, and
s.getRegressedTestsForRegressions) to rely on the validated values.
- Around line 1692-1702: When a non-empty view is provided but release is empty
we must map that view to its sample release and fail fast if the view name is
unknown: inside the block that iterates s.views.ComponentReadiness (where
variables view, views and release are used) if you find v with v.Name == view
set views = []crview.View{v} and if release is empty set release =
v.SampleRelease (use the actual field name on the View struct for the sample
release); if no matching view is found return a 400/BadRequest error immediately
instead of calling componentreadiness.ListRegressions(s.db, release, views,
allReleases, s.crTimeRoundingFactor, req).
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 182-231: Sanitize viewName as before but ensure the column `field`
is unique by appending the map index (or another unique suffix) when building
columns in the viewNames.map callback (where `field` is computed as
`status_${viewName.replace(...)} `); for example compute `const sanitized =
viewName.replace(/[^a-zA-Z0-9_-]/g, '_')` and `const field =
\`status_${sanitized}_${idx}\``, keep `headerName` as the original viewName, and
leave `valueGetter`, `renderCell`, `generateTestDetailsReportLink`,
`props.filterVals`, and `expandEnvironment` usage unchanged so DataGrid column
names won't collide.
In `@sippy-ng/src/component_readiness/TriagePotentialMatches.js`:
- Around line 163-171: The useEffect that conditionally calls
findPotentialMatches checks isModalOpen but does not include it in the
dependency array, so add isModalOpen to the dependency list of the
React.useEffect (alongside selectedBaseRelease and selectedSampleRelease) to
ensure the effect re-runs when the modal open state changes; verify the effect
signature using the same React.useEffect and that findPotentialMatches remains
stable (or wrap it in useCallback) to avoid unnecessary re-renders.
- Around line 113-151: The useEffect in TriagePotentialMatches has a race where
the single-option branch may set selectedBaseRelease/selectedSampleRelease
before the mostFrequentPair check, and the check currently requires both to be
unset; also the effect's dependency array omits selectedBaseRelease and
selectedSampleRelease causing stale closures. Fix by (1) computing pairCounts
and mostFrequentPair as you already do, then set defaults individually: if
(mostFrequentPair) { const [base,sample]=...; if (!selectedBaseRelease)
setSelectedBaseRelease(base); if (!selectedSampleRelease)
setSelectedSampleRelease(sample); } so you don't require both to be unset, and
(2) include selectedBaseRelease and selectedSampleRelease in the useEffect
dependency array (or use functional updaters) to avoid stale closure issues;
reference the useEffect in TriagePotentialMatches and the setters
setSelectedBaseRelease/setSelectedSampleRelease and variables
selectedBaseRelease/selectedSampleRelease when making this change.
🧹 Nitpick comments (5)
config/views.yaml (1)
151-152: TODO comment lacks actionable detail.The comment
#TODO: lets see what happensis too informal to be useful. Consider documenting:
- What specific behavior you're observing/testing
- Acceptance criteria for keeping this enabled
- A follow-up issue or deadline to revisit
Also note:
4.21-hypershift-candidates(Line 777) does not have aregression_trackingsection at all—ensure this asymmetry is intentional.sippy-ng/src/component_readiness/TriageFields.js (1)
65-65: Unusedviewvariable after refactor.The
viewvariable is destructured from context but no longer used in this file after the change to usereleaseparameter instead. Consider removing it to avoid confusion.- const { view, sampleRelease } = useContext(CompReadyVarsContext) + const { sampleRelease } = useContext(CompReadyVarsContext)pkg/db/db.go (1)
106-106: Consider removing or addressing the TODO comment before merging.The TODO comment suggests uncertainty about whether additional migration commands are needed. Since this is a WIP PR, ensure this is resolved or the comment is removed before the PR is finalized.
pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go (1)
176-210: Consider simplifying the match collection logic.The function now collects matches into a slice (line 182-204) and then returns the first match (lines 206-208). Since only the first match is returned, this is equivalent to the commented-out early return on line 203.
If there's no planned future use for collecting multiple matches (e.g., selecting the best match based on criteria), this could be simplified to return immediately upon finding a match.
Suggested simplification
func FindOpenRegression(sampleRelease, baseRelease, testID string, variants map[string]string, regressions []*models.TestRegression) *models.TestRegression { - var matches []*models.TestRegression for _, tr := range regressions { if sampleRelease != tr.Release && baseRelease != tr.Release { continue } // We compare test ID not name, as names can change. if tr.TestID != testID { continue } found := true for key, value := range variants { if value != findVariant(key, tr) { found = false break } } if !found { continue } // If we made it this far, this appears to be a match: - //return tr - matches = append(matches, tr) - } - if len(matches) > 0 { - return matches[0] + return tr } return nil }pkg/api/componentreadiness/regressiontracker.go (1)
221-266: Optional:reportsslice is never read.If this is no longer needed, consider removing it to avoid extra allocations.
♻️ Suggested cleanup
- var reports []crtype.ComponentReport + // var reports []crtype.ComponentReport ... - reports = append(reports, report)
|
/test e2e |
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/api/componentreadiness/regressiontracker.go (1)
406-418:⚠️ Potential issue | 🟠 MajorNewly opened regressions are not added to the active set.
When a new regression is opened (lines 407-417), the newly created regression is not added to
matchedOpenRegressions. This means newly opened regressions could be immediately closed on the same sync cycle if they don't match an existing regression in subsequent iterations.However, looking at the flow:
matchedOpenRegressionsis returned and then added toactiveRegressions(line 275). The closing logic (lines 290-303) only closes regressions that are not inactiveRegressionsAND are not already closed. Since new regressions are just created, they won't be in theregressionslist fetched at line 284 (which was fetched before the new one was created).Actually, on closer inspection:
ListCurrentRegressionsForReleaseis called afterSyncRegressionsForReport, so newly created regressions would be included in the list. But they wouldn't be inactiveRegressions, so they could be closed.🐛 Proposed fix to include newly opened regressions
if !rt.dryRun { // Open a new regression: newReg, err := rt.backend.OpenRegression(view, regTest) if err != nil { rLog.WithError(err).Errorf("error opening new regression for: %v", regTest) return nil, errors.Wrapf(err, "error opening new regression: %v", regTest) } rLog.Infof("new regression opened with id: %d", newReg.ID) + matchedOpenRegressions = append(matchedOpenRegressions, newReg) }sippy-ng/src/component_readiness/TriagePotentialMatches.js (1)
176-188:⚠️ Potential issue | 🟡 MinorClear stale matches when response is empty or null.
When
matchesis empty or null,potentialMatchesretains previous data, which could confuse users when switching releases.🔧 Suggested fix
.then((matches) => { console.log('Potential matching regressions:', matches) - if (matches && matches.length > 0) { - const sortedMatches = matches.sort( - (a, b) => b.confidence_level - a.confidence_level - ) - setPotentialMatches(sortedMatches || []) + if (matches && matches.length > 0) { + const sortedMatches = [...matches].sort( + (a, b) => b.confidence_level - a.confidence_level + ) + setPotentialMatches(sortedMatches) + } else { + setPotentialMatches([]) }
🤖 Fix all issues with AI agents
In `@pkg/sippyserver/server.go`:
- Around line 1617-1621: The error log string is incorrect: when
componentreadiness.GetTriage(s.db, triageID, req) returns an error the code
calls failureResponse with "error getting releases" — update that message to
accurately reflect the failing operation (e.g., "error getting triage") in the
failureResponse call that follows the GetTriage call so the error refers to
componentreadiness.GetTriage, triageID and req.
In `@sippy-ng/src/component_readiness/TriagePotentialMatches.js`:
- Around line 167-169: The fetch call that builds the request URL in
TriagePotentialMatches.js currently interpolates selectedBaseRelease and
selectedSampleRelease directly into the query string, which will break for names
with spaces or special characters; update the URL construction in the fetch
invocation (the expression using triage.links.potential_matches) to URL-encode
those parameters (e.g., wrap selectedBaseRelease and selectedSampleRelease with
encodeURIComponent or build the query via URLSearchParams) before concatenation
so the resulting request is safe for values containing &, =, #, spaces, etc.
smg247
force-pushed
the
remove-view-from-regressions
branch
from
fd0f052 to
5a50d34
Compare
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/componentreadiness/regressiontracker.go (1)
404-416:⚠️ Potential issue | 🟠 MajorNewly opened regressions are not included in the returned
matchedOpenRegressions.When a new regression is opened (lines 409-414), the newly created
newRegis not appended tomatchedOpenRegressions. This means the caller'sactiveRegressionsset won't include newly opened regressions, which could cause them to be immediately closed in the subsequent closing logic.🐛 Proposed fix
} else { openedRegs++ rLog.Infof("opening new regression: %v", regTest) if !rt.dryRun { // Open a new regression: newReg, err := rt.backend.OpenRegression(view, regTest) if err != nil { rLog.WithError(err).Errorf("error opening new regression for: %v", regTest) return nil, errors.Wrapf(err, "error opening new regression: %v", regTest) } rLog.Infof("new regression opened with id: %d", newReg.ID) + matchedOpenRegressions = append(matchedOpenRegressions, newReg) } }
🤖 Fix all issues with AI agents
In
`@pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go`:
- Around line 175-187: In FindOpenRegression, the current release filter
incorrectly compares tr.Release twice; update the logic to ensure triegressions
match both sides of the release pair by checking tr.Release == sampleRelease AND
tr.BaseRelease == baseRelease (or add explicit handling for legacy rows if
needed) so that a regression is only considered when its Release equals
sampleRelease and its BaseRelease equals baseRelease; adjust the condition that
currently reads using sampleRelease/baseRelease vs tr.Release to reference
tr.BaseRelease for the second comparison.
In `@pkg/api/componentreadiness/regressiontracker.go`:
- Around line 229-241: The code logs and appends errors when
utils.GetViewReleaseOptions fails for the base or sample release (calls that
populate baseRelease and sampleRelease) but then continues processing the view
with potentially invalid release options; update the error blocks after each
GetViewReleaseOptions call (for "basis"/baseRelease and "sample"/sampleRelease)
to immediately skip processing this view by adding a continue, ensuring you
don't proceed to generate the component report with zero-valued/invalid release
options (references: GetViewReleaseOptions, baseRelease, sampleRelease, vLog,
errs, view, rt.cacheOpts.CRTimeRoundingFactor).
🧹 Nitpick comments (4)
sippy-ng/src/component_readiness/TriagePotentialMatches.js (2)
144-152: Consider wrappingfindPotentialMatchesinuseCallback.The
findPotentialMatchesfunction is called from multiple effects and event handlers but isn't wrapped inuseCallback. While the current implementation works because the effects include the state variables the function depends on, ESLint'sreact-hooks/exhaustive-depsrule may warn about the missing function dependency.♻️ Optional improvement
Wrap
findPotentialMatcheswithuseCallbackand include it in the dependency arrays:const findPotentialMatches = React.useCallback(() => { setIsLoading(true) setIsModalOpen(true) fetch( `${triage.links.potential_matches}?baseRelease=${encodeURIComponent(selectedBaseRelease)}&sampleRelease=${encodeURIComponent(selectedSampleRelease)}` ) // ... rest of function }, [selectedBaseRelease, selectedSampleRelease, triage.links.potential_matches, setMessage])Then update both useEffect dependency arrays to include
findPotentialMatches.
367-384: Minor: redundant variable assignment.
viewToUseon line 369 is immediately assigned fromviewand used nowhere else. Consider usingviewdirectly for clarity.valueGetter: (params) => { const regressedTest = params.row.regressed_test - const viewToUse = view - const filterVals = `?view=${viewToUse}` + const filterVals = `?view=${view}`pkg/api/componentreadiness/regressiontracker.go (1)
304-308:ResolveTriages()is called once per release, but operates globally.Looking at the
ResolveTriagesimplementation (lines 106-160), it queries all triages without filtering by release. Calling it multiple times (once per release in theLoadloop) will redundantly process the same triages. Consider movingResolveTriages()to be called once after all releases are processed in theLoadmethod, or make it release-scoped.♻️ Proposed refactor - move to Load method
Move
ResolveTriages()call toLoad()after all releases are processed:func (rt *RegressionTracker) Load() { ctx := context.Background() for _, release := range rt.releases { if err := rt.SyncRegressionsForRelease(ctx, release.Release); err != nil { rt.logger.WithError(err).Errorf("error syncing regressions for release %s", release.Release) } } + // Resolve triages once after all releases are synced + if len(rt.errors) == 0 { + rt.logger.Info("resolving triages that have had all of their regressions closed") + if err := rt.backend.ResolveTriages(); err != nil { + rt.logger.WithError(err).Error("error resolving triages") + rt.errors = append(rt.errors, err) + } + } }And remove from
SyncRegressionsForRelease:- rLog.Infof("resolving triages that have had all of their regressions closed") - if err := rt.backend.ResolveTriages(); err != nil { - rLog.WithError(err).Error("error resolving triages") - errs = append(errs, errors.Wrapf(err, "error resolving triages for release: %s", release)) - }pkg/sippyserver/server.go (1)
822-851: Potential duplicate regressed tests across views.When iterating over multiple matching views, the same regression could match a
ReportTestSummaryin multiple views, resulting in duplicates in the returned slice. If this is intentional (e.g., caller needs per-occurrence data), this is fine. Otherwise, consider deduplicating by regression ID or test ID.
pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go
Outdated
Show resolved
Hide resolved
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sippy-ng/src/component_readiness/TriagePotentialMatches.js`:
- Line 106: The destructured view from useContext(CompReadyVarsContext) may be
undefined and is later used to build filterVals; update the component to guard
against undefined by providing a sensible default or conditionally adding the
view query parameter. For example, set a default when extracting from context
(e.g., in the useContext line that defines expandEnvironment and view) or modify
the code that constructs filterVals so it only appends ?view=... when view is a
non-empty string (use view || '' or a conditional append). Ensure references to
view, useContext(CompReadyVarsContext), and the filterVals construction logic
are updated so the generated URL never contains literal "undefined" or an empty
view parameter.
- Around line 164-171: The fetch in findPotentialMatches assumes
triage.links.potential_matches exists and will throw if triage or triage.links
is undefined; add a defensive guard at the start of findPotentialMatches to
check triage && triage.links && triage.links.potential_matches and if missing,
do not call fetch—reset or set appropriate UI state (e.g., setIsLoading(false)
and keep/close modal via setIsModalOpen) and optionally log or surface an error;
ensure the guard references triage.links.potential_matches and prevents
executing the fetch URL construction.
🧹 Nitpick comments (2)
sippy-ng/src/component_readiness/TriagePotentialMatches.js (2)
144-162: MissingfindPotentialMatchesin useEffect dependency arrays.Both effects call
findPotentialMatches()but don't include it in their dependency arrays. SincefindPotentialMatchescapturestriage.links.potential_matchesin its closure, changes totriagewon't trigger a re-fetch with the updated URL.Consider either:
- Wrapping
findPotentialMatchesinuseCallbackwith appropriate dependencies and adding it to the effect deps- Adding
triage.links.potential_matchesdirectly to the dependency arrays
188-190: RedundantsetIsModalOpen(true)call.Line 190 sets
isModalOpentotrue, but it was already set totrueon line 166 at the start of the function. This second call is unnecessary.♻️ Suggested removal
setFilterSimilarNames(true) setFilterSameLastFailures(true) setFilterAlreadyTriaged(false) - setIsModalOpen(true) })
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/componentreadiness/regressiontracker.go (1)
406-418:⚠️ Potential issue | 🔴 CriticalNewly opened regressions are not tracked, causing immediate closure.
When a new regression is opened,
newRegis created but never added tomatchedOpenRegressions. This means:
- The new regression is created in the database with an ID
- It's not included in the returned slice
activeRegressions(line 276) won't contain the new IDListCurrentRegressionsForRelease(line 284) will return it- The closing logic (line 291) will immediately close it since it's not in
activeRegressions🐛 Proposed fix
} else { openedRegs++ rLog.Infof("opening new regression: %v", regTest) if !rt.dryRun { // Open a new regression: newReg, err := rt.backend.OpenRegression(view, regTest) if err != nil { rLog.WithError(err).Errorf("error opening new regression for: %v", regTest) return nil, errors.Wrapf(err, "error opening new regression: %v", regTest) } rLog.Infof("new regression opened with id: %d", newReg.ID) + matchedOpenRegressions = append(matchedOpenRegressions, newReg) } }
🤖 Fix all issues with AI agents
In `@pkg/api/componentreadiness/regressiontracker.go`:
- Around line 257-274: The block calling GetComponentReportFromBigQuery collects
reportErrs but continues processing an invalid report; after you log/append the
composed error (reportErrs -> err) you should skip the rest of the loop
iteration so SyncRegressionsForReport is not called with a bad report. Update
the logic around reportErrs in the same function (the code that constructs
strErrors, logs with vLog.WithError and appends to errs) to add an immediate
continue when len(reportErrs) > 0 so Rt.SyncRegressionsForReport(view, rLog,
&report) is only invoked for valid reports.
In `@pkg/sippyserver/server.go`:
- Around line 1699-1715: When a specific view is requested but release is empty,
the code filters s.views.ComponentReadiness to the matching crview.View but
never sets release, causing componentreadiness.ListRegressions to run
unfiltered; update the view-handling block in server.go (the loop that checks
s.views.ComponentReadiness and sets views = []crview.View{v}) to also set
release = v.SampleRelease.Name when a match is found so ListRegressions is
called with the view's sample release.
🧹 Nitpick comments (2)
pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go (1)
202-207: Consider returning early instead of collecting all matches.Since only the first match is returned, the intermediate slice is unnecessary. This simplifies the code and avoids allocations when a match is found.
♻️ Proposed simplification
// If we made it this far, this appears to be a match: - matches = append(matches, tr) - } - if len(matches) > 0 { - return matches[0] + return tr } return nil }This also allows removing the
matchesvariable declaration on line 182.pkg/sippyserver/server.go (1)
822-851: Consider passing releases as parameters instead of re-extracting from request.The caller
jsonTriagePotentialMatchingRegressionsalready validates and extractsbaseReleaseandsampleReleasefrom the request (lines 1628-1636), then this helper re-extracts them. While safe, this pattern is slightly redundant and makes the dependency on query params implicit.♻️ Suggested refactor
-func (s *Server) getRegressedTestsForRegressions(req *http.Request, regressions []models.TestRegression) ([]componentreport.ReportTestSummary, error) { - baseRelease := req.URL.Query().Get("baseRelease") - sampleRelease := req.URL.Query().Get("sampleRelease") - if baseRelease == "" || sampleRelease == "" { - return nil, fmt.Errorf("baseRelease and sampleRelease are required") - } +func (s *Server) getRegressedTestsForRegressions(req *http.Request, baseRelease, sampleRelease string, regressions []models.TestRegression) ([]componentreport.ReportTestSummary, error) { matchingViews := componentreadiness.ViewsMatchingReleases(baseRelease, sampleRelease, s.views.ComponentReadiness)Then update the caller at line 1648:
- regressedTests, err := s.getRegressedTestsForRegressions(req, regressions) + regressedTests, err := s.getRegressedTestsForRegressions(req, baseRelease, sampleRelease, regressions)
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
…s locally after 7pm EST
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/componentreadiness/regressiontracker.go (1)
407-420:⚠️ Potential issue | 🔴 CriticalNewly opened regressions are not tracked and will be immediately closed.
When a new regression is opened (line 412), it's not added to
matchedOpenRegressions. Since this return value is used to populateactiveRegressions(lines 276-278), newly opened regressions won't be in that set. Later, whenListCurrentRegressionsForRelease(line 285) fetches all regressions including the newly created ones, the closing logic at lines 291-304 will close them immediately because!activeRegressions.Has(regression.ID)will be true.🐛 Proposed fix - add newly opened regressions to matchedOpenRegressions
} else { openedRegs++ rLog.Infof("opening new regression: %v", regTest) if !rt.dryRun { // Open a new regression: newReg, err := rt.backend.OpenRegression(view, regTest) if err != nil { rLog.WithError(err).Errorf("error opening new regression for: %v", regTest) return nil, errors.Wrapf(err, "error opening new regression: %v", regTest) } rLog.Infof("new regression opened with id: %d", newReg.ID) + matchedOpenRegressions = append(matchedOpenRegressions, newReg) } }
🧹 Nitpick comments (2)
pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.go (1)
182-206: Consider early return instead of collecting matches.The
matchesslice collects all qualifying regressions but only the first one is returned. Since the function returns after the loop regardless, an early return on first match would be simpler and more efficient:♻️ Proposed simplification
- var matches []*models.TestRegression for _, tr := range regressions { if sampleRelease != tr.Release { continue } // We compare test ID not name, as names can change. if tr.TestID != testID { continue } found := true for key, value := range variants { if value != findVariant(key, tr) { found = false break } } if !found { continue } // If we made it this far, this appears to be a match: - matches = append(matches, tr) - } - if len(matches) > 0 { - return matches[0] + return tr } return nilpkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.go (1)
515-552: Test case combines multiple mismatch scenarios.The "release mismatch" test case contains two regressions: one with a different test ID and one with a different release. While functionally correct, the test name suggests it's specifically testing release mismatch, but the first regression fails due to test ID mismatch. Consider renaming to "no match when no regression matches both testID and release" or splitting into separate cases for clarity.
|
Scheduling required tests: |
… they were opened when only appearing in a single report
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling required tests: |
|
@smg247: This pull request references TRT-2509 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| enabled: false | ||
| regression_tracking: | ||
| enabled: false | ||
| - name: 4.22-techpreview-rhcos9-vs-rhcos10 |
There was a problem hiding this comment.
Omission is effectively the same as explicitly disabling.
We will likely turn this on in a follow up.
|
/retitle TRT-2509: remove the |
openshift-ci
bot
changed the title
view column from TestRegression view column from TestRegression
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold to manage the rollout |
|
/test e2e |
openshift-ci
bot
added
the
do-not-merge/hold
| api.RespondWithJSON(http.StatusOK, w, viewsCopy) | ||
| } | ||
|
|
||
| func (s *Server) getRegressedTestsForRegressions(req *http.Request, regressions []models.TestRegression) ([]componentreport.ReportTestSummary, error) { |
There was a problem hiding this comment.
I suck at naming and I don't have anything better to suggest, but at the same time I just can't not make some snarky comment about getRegressedTestsForRegressions, I mean what else would the regressed tests be for 😬 ok all done here, carrying on with the review.
There was a problem hiding this comment.
...yeah this is questionable. They are called regressed_tests in the report, and they contain a regression which is the serialized DB info. So it is accurate, but you also have to understand that for it to make any sense.
There was a problem hiding this comment.
I think I was more punch drunk from being on the my 5th pr review.. All good.
|
/test all |
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, smg247 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@smg247: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Necessary changes to allow for regression tracking from multiple views targeting the same release.
Config
Regression Tracking can now be enabled in multiple views for the same release in
views.yaml, so the validation preventing that has been removed.Regression Tracking
This now runs in a loop through each
sample release.OpenedandLastFailureare columns on thetest_regressionstable, so there is only one value across all views that share the regression. This means that theRegressed Sincecolumn will show the value of the first view it was spotted on, and theLast Failurecolumn will show the most recent failure across all views (with regression tracking enabled).Similarly, closing the regression will happen when it no longer is occurring in any view. It won't show in the regressed tests modal of views where it no longer is showing up though.
Triages are only closed when the regression has rolled off all views with regression tracking enabled.Frontend
Some display changes were necessary in order to properly surface that a
regressioncan show on multipleviews.The
Included Testslist on theTriage Detailspage now shows status icons and links totest_detailsreports for any view that the regression appears on. These are organized into columns with the-mainview displaying first:Potentially matching regression determination is now controlled by
baseandsamplerelease pairs rather than theview:Triaging regressions can now be done from any view, regression-tracking doesn't need to be enabled on the view to do so. The only caveat is that the Triage Details page will not show a column for views that don't have regression tracking enabled.
Migration
I have confirmed that the migration logic and regression-tracker are able to handle the case seamlessly where:
test_regressionsandtriagesexist utilizing the old logicmigrateis runcomponent-readiness-cacheandregression-trackerloaders are run (the same ones that thecache-primertriggers)This results in a fully functional component-readiness where existing regressions have their
viewcolumn dropped, but are still associated with the propertriageentries, and new regressions are able to be added to existingtriageentries.Caveat: once we migrate, it will be difficult to go back without restoring the DB from a backup. This means that we will likely have to fix any issues that come from this change, rather than do a revert.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor