Count every FeatureGate tag in test names for unique_test_count#3368
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactored the feature gates SQL query to replace a single regex extraction pattern with two separate UNIONed data sources: one for tag-based feature gate matching using global regex matching, and one for install path matching, with updated LEFT JOIN logic to handle both cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/db/query/feature_gates.go (1)
48-48: Avoid extra join fan-out on install rows.Line 48 can also match tagged rows from
byTagwhenever the same test name containsinstall should succeed. SincebyInstallalready contributes a dedicatedNULLrow for that case, restricting the fallback tomt.gate_name IS NULLkeeps the special case explicit and avoids unnecessary duplicate join work.Suggested fix
- Joins("LEFT JOIN (?) AS mt ON fg.feature_gate = mt.gate_name OR (fg.feature_gate LIKE '%Install%' AND mt.name LIKE '%install should succeed%')", subQuery). + Joins(`LEFT JOIN (?) AS mt + ON fg.feature_gate = mt.gate_name + OR ( + mt.gate_name IS NULL + AND fg.feature_gate LIKE '%Install%' + AND mt.name LIKE '%install should succeed%' + )`, subQuery).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/query/feature_gates.go` at line 48, The LEFT JOIN condition currently allows matches from the byTag subQuery (aliased mt) when the test name contains "install should succeed", causing extra fan-out; update the join expression used in the Joins call so the fallback OR clause only applies when mt.gate_name IS NULL (i.e., change the OR branch to include "AND mt.gate_name IS NULL"), referencing the existing aliases fg.feature_gate and mt.gate_name and the subQuery to ensure the dedicated byInstall NULL row remains the only match for the install special-case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/db/query/feature_gates.go`:
- Line 48: The LEFT JOIN condition currently allows matches from the byTag
subQuery (aliased mt) when the test name contains "install should succeed",
causing extra fan-out; update the join expression used in the Joins call so the
fallback OR clause only applies when mt.gate_name IS NULL (i.e., change the OR
branch to include "AND mt.gate_name IS NULL"), referencing the existing aliases
fg.feature_gate and mt.gate_name and the subQuery to ensure the dedicated
byInstall NULL row remains the only match for the install special-case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0225e538-b652-4c05-9a06-c9741e2e177a
📒 Files selected for processing (1)
pkg/db/query/feature_gates.go
|
Scheduling required tests: |
1 similar comment
|
Scheduling required tests: |
|
/lgtm Verified locally 
|
|
/test lint |
deepsm007
force-pushed
the
fix/feature-gate-count-all-name-tags
branch
5 times, most recently
from
fb23e00 to
20a9201
Compare
deepsm007
force-pushed
the
fix/feature-gate-count-all-name-tags
branch
from
20a9201 to
7fa95c9
Compare
|
Lint is because of #3372, will be fixed soon |
|
/test lint |
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, neisw 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 |
|
@deepsm007: 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. |
4 participants
Problem
unique_test_counton feature gates only considered the first[FeatureGate:…]/[OCPFeatureGate:…]in each test name. Tests that list multiple gates could show 0 for gates that appear after the first tag, even when those tests had recent runs and appeared elsewhere in Sippy.Solution
regexp_matches(..., 'g')withCROSS JOIN LATERALso every gate tag in the test name contributes a row.UNION ALLrows withNULLgate_nameforinstall should succeed, with the same join behavior for Install-related gates.Table+Joins/Where; useRaw("? UNION ALL ?", …)only to merge the two subqueries (this GORM version has noUnionon*gorm.DB).Impact
Counts may increase when one test name references more than one gate; that is intended so each gate is attributed correctly.
https://redhat-internal.slack.com/archives/CBN38N3MW/p1774459082318689
Summary by CodeRabbit