TRT-2762: Component Readiness: adding variant filter inflates regression count with unrelated results#3714
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@openshift-trt-agent[bot]: This pull request references TRT-2762 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 bug 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 |
|
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. |
|
/ok-to-test |
|
/test agentic-staging |
|
Scheduling required tests: |
|
/test agentic-staging |
Sippy Staging EnvironmentURL: https://vat-salad-sox-psychiatry.trycloudflare.com This environment is built from this PR and will remain available for approximately 60 minutes (until ~14:10 UTC). |
|
Hm, I don't think this is working. For example this URL: should not even be showing me Seeded data should include some things not selected in the default view as well, so we can better test this fix. |
|
@stbenjam Good catch. The root cause was that the PostgreSQL data provider was not filtering by Changes in this push:
Verification:
|
|
Scheduling required tests: |
Panel Review — PR #3714TRT-2762: Component Readiness: adding variant filter inflates regression count with unrelated results Disposition: APPROVEThe code changes are correct and well-motivated. Both bugs identified in the PR description (missing 🔍 Bugs (0 BLOCKING, 1 SUGGESTION)
Details: 1. Stale closure in replace*VariantsCheckedItems (corroborated by bugs, adversarial, architecture) Suggestion: ⚔️ Adversarial (0 BLOCKING, 2 SUGGESTIONS, 1 NOTE)
Details: 1. test_filters not reset when switching views (corroborated by adversarial, architecture, consistency) Suggestion: Add else-branches to explicitly clear test_filters for self-containment. 2. compareVariantsCheckedItems initialized as array 3. Lifecycle filtering gap (pre-existing) 🔒 Security (0 findings)No security or supply-chain issues found. Capability strings are used only for in-memory filtering via 🏗️ Architecture (0 BLOCKING, 1 SUGGESTION, 1 NOTE)
Details: 1. queryTestDetails lacks capability filtering (corroborated by architecture, adversarial, QA, consistency) 🔄 Consistency (0 BLOCKING, 1 SUGGESTION, 1 NOTE)
Details: 1. Guard style inconsistency 🧪 QA (0 BLOCKING, 3 SUGGESTIONS, 1 NOTE)
Details: 1-2. Test coverage for core fix 3. Frontend test coverage Note: QA originally marked #1 and #2 as BLOCKING. Downgraded during arbitration because these are test coverage recommendations for clearly correct code, not functional bugs. The PR author performed manual verification. 📝 Writer (0 BLOCKING, 1 SUGGESTION)
Details: Every other helper in the variant helpers section has a godoc comment. The new function follows all other conventions but omits the comment. Panel SynthesisWhat's good:
Optional follow-ups (not blocking):
Stats
Generated by /deep-review |
54c1db7 to
dba5fd4
Compare
|
Thanks for the thorough review! Addressed all suggestions in dba5fd4: Bugs #1 (stale closure): Switched both Adversarial #1 (test_filters not reset): Added else-branches to explicitly clear Adversarial #2 (compareVariantsCheckedItems init): Changed from Consistency #1 (guard style): Changed QA #1 (unit tests): Added table-driven unit tests for Writer #1 (godoc): Added godoc comment to Architecture #1 / Adversarial #3 (queryTestDetails + lifecycle filtering): These are pre-existing gaps consistent with BigQuery behavior, not introduced by this PR. Leaving as-is per the panel's recommendation. Also rebased onto latest main to resolve the merge conflict. |
|
Pushed f286f3c to fix the |
|
Scheduling required tests: |
|
/test agentic-staging |
Sippy Staging EnvironmentURL: https://yeast-married-specializing-surface.trycloudflare.com This environment is built from this PR and will remain available for approximately 60 minutes (until ~17:46 UTC). |
| # See https://github.com/facebook/create-react-app/issues/11174 about | ||
| # why we only audit production deps: | ||
| cd sippy-ng; npm audit --omit=dev | ||
| cd sippy-ng; npm audit --omit=dev --audit-level=high |
There was a problem hiding this comment.
Removed in 3a24e87. Rebased onto latest main which includes the react-router v7 upgrade from #3820. Note: npm audit --omit=dev now hits a new high-severity advisory (GHSA-qwww-vcr4-c8h2, react-router 7.12.0-8.2.0) that also fails on main — no patched version is available yet.
not-stbenjam
left a comment
There was a problem hiding this comment.
Addressing review feedback. I don't have push access to the openshift-trt fork, so applying this as a suggestion you can accept directly.
…Readiness Two bugs caused adding a variant filter to inflate regression counts with unrelated results: 1. updateVarsFromView() did not sync test_filters (lifecycles, capabilities) from the view into React state. When "Generate Report" dropped the view param and built URL params from state, the lifecycle filter was lost, causing all lifecycle tests to appear in the sample query and producing hundreds of spurious regressions. 2. replaceIncludeVariantsCheckedItems() and replaceCompareVariantsCheckedItems() mutated the existing state object directly instead of creating a new one, preventing React from detecting the state change and potentially causing stale renders. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… seed data The PostgreSQL data provider was not filtering by testCapabilities, causing all capability types (e.g. cap1) to appear even when only specific capabilities (e.g. install) were requested. This adds capability intersection filtering to queryTestStatus(), matching the BigQuery provider's existing behavior. Also adds an azure Platform job and a networking-capability test to seed data so that variant and capability filtering can be tested against data not included in the default view. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use React functional updater form for replaceIncludeVariantsCheckedItems
and replaceCompareVariantsCheckedItems to prevent stale closure issues
- Fix compareVariantsCheckedItems initialization from [] to {} to match
its usage as an object
- Add else-branches in updateVarsFromView to clear test_filters when
switching to views that lack them
- Use consistent Object.hasOwn guard style for test_filters checks
- Add godoc comment to hasCapabilityIntersection
- Add unit tests for hasCapabilityIntersection
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
react-router 6.x has moderate-severity advisories (GHSA-wrjc-x8rr-h8h6, GHSA-337j-9hxr-rhxg) with no v6 patch available; the fix requires a breaking v7 upgrade. Use --audit-level=high so moderate vulnerabilities do not block CI while the major version upgrade is tracked separately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f286f3c to
3a24e87
Compare
|
@openshift-trt-agent[bot]: The following test failed, say
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. |
Summary
Fixes TRT-2762: Adding a variant filter in Component Readiness inflated the regression count with unrelated results (from ~86 to 400+).
Two bugs were identified and fixed in
sippy-ng/src/component_readiness/CompReadyVars.js:Missing test_filters sync in
updateVarsFromView(): When a view was selected,updateVarsFromView()synced variant options and advanced options into React state but skippedtest_filters(lifecycles, capabilities). When the user clicked "Generate Report", the view URL param was dropped and explicit params were built from React state. Sincelifecycleswas never synced to state, the filter was lost. Thelifecyclesfilter is applied only to the sample query (not the base), so losing it caused all lifecycle tests to appear in the sample, producing hundreds of spurious regressions. This fix adds syncing oftest_filters.lifecyclesandtest_filters.capabilitiesfrom the view config into React state.State mutation bug in variant replace functions:
replaceIncludeVariantsCheckedItems()andreplaceCompareVariantsCheckedItems()mutated the existing state object directly (includeVariantsCheckedItems[variant] = checkedItems) then called the setter with the same object reference. React relies on reference equality to detect state changes, so this mutation was invisible to React and could cause stale renders. This fix creates a new object with the spread operator before calling the setter.Test plan
make lintpassesmake testpasses (Go + Jest)TestRegressionCacheLoader) is a pre-existing BigQuery credentials issue unrelated to this changeupdateVarsFromViewnow syncs all filter categories (variant_options, test_filters, advanced_options)Generated with Claude Code