TRT-1963: Sippy FeatureGate readiness dashboard is not filtering failures/flakes for the "2 day time period" - its showing older data as well#3754
Conversation
When a user selects the "2 day" period on the tests page and clicks links to Test Analysis or Job Runs pages, the period was not carried over, causing those pages to show data from a wider (7-day or 30-day) time range. This fixes the usability issue by propagating the period parameter through all navigation paths: - Helper functions (pathForExactTestAnalysis*, pathForJobRunsWith*) now accept an optional period parameter - The timestamp filter for job runs uses 2-day or 7-day window based on the period - TestTable passes the current period to all generated links - TestAnalysis reads period from URL params, passes it to the API, and forwards it to job runs links and the embedded TestTable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
ready-for-human-review
|
@openshift-trt-agent[bot]: This pull request references TRT-1963 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 "5.0.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-trt-agent[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @openshift-trt-agent[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
| @@ -81,10 +82,14 @@ export function TestAnalysis(props) { | |||
| } | |||
|
|
|||
| const filter = safeEncodeURIComponent(JSON.stringify(filterModel)) | |||
There was a problem hiding this comment.
Did you consider adding the period to the filterModel? Is there a reason why this cannot be done? Is it used in too many places that don't require it? Are there other similar params there?
There was a problem hiding this comment.
Good question. The filterModel is structured as a DataGrid-style filter with items that have columnField, operatorValue, and value — it maps to column-based filters like name and variants. The period parameter is fundamentally different: it controls the time range for the query (e.g. twoDay vs the default 7-day window), not a column filter. On the API side, filter and period are handled as separate query parameters (see TestTable.js lines 930-945 where they are appended independently). Embedding period in filterModel would conflate two different concerns — the filter toolbar UI (GridToolbarFilterMenu) would need special-casing to handle a non-column parameter, and the API deserialization would need to extract it back out. Keeping them separate is cleaner and consistent with how other non-filter params like sortField, sort, limit, and overall are handled.
There was a problem hiding this comment.
To follow up on this: I verified the approach and it is the right one. The filterModel is designed for DataGrid column filters (columnField/operatorValue/value items rendered by GridToolbarFilterMenu), while period is a query-level parameter like sortField, sort, limit, and overall — all of which are handled as separate query params rather than filter items. Mixing them would require special-casing in both the filter UI and the API deserialization layer.
I also fixed a missing period dependency in the setPageContextForChat useEffect — the period variable was used in the instructions template literal but was not listed in the dependency array.
|
@coderabbitai review |
|
/ok-to-test |
✅ Action performedReview finished.
|
openshift-ci
Bot
added
ok-to-test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds period-aware test analysis state, API requests, labels, and navigation URLs. Helper builders now accept period, append it to analysis links, and select two-day or seven-day job-run timestamp filters. ChangesPeriod-aware test analysis
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant TestAnalysis
participant API
participant helpers
participant TestTable
Browser->>TestAnalysis: provide period query parameter
TestAnalysis->>API: fetch test data with optional period
API-->>TestAnalysis: return test analysis data
TestAnalysis->>helpers: build period-aware navigation URLs
TestAnalysis->>TestTable: pass selected period
TestTable->>helpers: build analysis and job-run links
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Scheduling required tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sippy-ng/src/helpers.js`:
- Line 163: Remove the stray debug logging from pathForExactTestAnalysis in
helpers.js by deleting the console.log(excludedVariants) statement. Keep the
rest of the excludedVariants handling unchanged and ensure no other temporary
console output remains in that helper.
In `@sippy-ng/src/tests/TestAnalysis.js`:
- Around line 85-92: The page context text in setPageContextForChat is hardcoded
for the 7-day timeframe, but TestAnalysis also supports period="twoDay" via
periodParam, so the instructions can become stale. Update the
setPageContextForChat message to reflect the active period dynamically based on
the current period value, and make sure the wording changes for twoDay instead
of always describing a 7-day comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 990cd1ce-bc3e-4757-a4cd-1a5f25df57c5
📒 Files selected for processing (3)
sippy-ng/src/helpers.jssippy-ng/src/tests/TestAnalysis.jssippy-ng/src/tests/TestTable.js
Remove stray console.log(excludedVariants) left in pathForExactTestAnalysis and update setPageContextForChat instructions to dynamically reflect the active period (2-day vs 7-day) instead of hardcoding 7-day. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
|
/test agentic-staging |
Sippy Staging EnvironmentURL: https://palestinian-ins-expenses-sms.trycloudflare.com This environment is built from this PR and will remain available for approximately 60 minutes (until ~18:00 UTC). |
…ffect The period variable is used in the instructions template literal but was not listed in the dependency array. This ensures the page context text updates correctly when the period changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
2 participants
Summary
Fixes TRT-1963: The "2 day" period filter on the tests page was not propagated when navigating to linked pages (Test Analysis, Job Runs). Users selecting the twoDay period and clicking on test names, flake icons, or failure icons would see data from a wider time range (7 days or 30 days), defeating the purpose of the period filter.
Changes:
helpers.js: Renamedlast7DaysFilter()totimestampFilterForPeriod(period)so job runs timestamp filters use a 2-day or 7-day window based on the active period. Added an optionalperiodparameter topathForExactTestAnalysis,pathForExactTestAnalysisWithFilter,pathForJobRunsWithTest,pathForJobRunsWithTestFailure, andpathForJobRunsWithTestFlaketo append the period to generated URLs.TestTable.js: Passes the currentperiodvalue to all link-generating helper function calls (test name links, bugs links, failure icon links, flake icon links).TestAnalysis.js: Readsperiodfrom URL query params, passes it to the API call so the backend returns period-appropriate data, forwards it to "All Runs"/"Failed Runs"/"Flaked Runs" button links, passes it to the embedded TestTable component, and updates the SummaryCard label to show "2 Day Overall" when appropriate.Test Plan
make lintpasses with 0 issuesmake test(Jest + Go tests) passesperiod=twoDaygenerates links that include&period=twoDay(confirmed via DOM inspection)periodcontinue to work with the default 7-day behavior (parameter is optional, defaults to 7 days)Generated with Claude Code
Summary by CodeRabbit