OCPBUGS-84961: append suite names to external extension binary tests#31251
Draft
bshaw7 wants to merge 2 commits into
Draft
OCPBUGS-84961: append suite names to external extension binary tests#31251bshaw7 wants to merge 2 commits into
bshaw7 wants to merge 2 commits into
Conversation
TestBinaries.ListTests() loads tests from external extension binaries but never calls appendSuiteNames() on them. Without [Suite:openshift/conformance/...] tags, the conformance suite filter in NewQualifiersFilter excludes these tests, causing the openshift/conformance suite to drop from ~4000 to ~190 tests on OCP 4.21+. Call appendSuiteNames() on the aggregated external binary test specs after loading them. The function already guards against double-tagging tests that already have [Suite:] in their name. Tested on OCP 4.21.8 GA cluster: - Before fix: 184 tests in openshift/conformance suite - After fix: 3895 tests in openshift/conformance suite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@bshaw7: This pull request references Jira Issue OCPBUGS-84961, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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)
WalkthroughThe ChangesSuite Name Augmentation in Test Listing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bshaw7 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 |
![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/test/extensions/binary.go (1)
950-954: ⚡ Quick winConsider adding a comment explaining the suite name augmentation.
This code extracts the vendor-type specs, calls
appendSuiteNames, and relies on pointer semantics to propagate the Name modifications back toallTests. A brief comment would help future maintainers understand why this step is necessary and prevent accidental removal.📝 Suggested comment
+ // Augment external binary specs with suite names (e.g., "[Suite:openshift/conformance/...]") + // so downstream filters can properly include/exclude tests by suite tag. var baseSpecs extensiontests.ExtensionTestSpecs for _, spec := range allTests { baseSpecs = append(baseSpecs, spec.ExtensionTestSpec) } appendSuiteNames(baseSpecs)🤖 Prompt for 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. In `@pkg/test/extensions/binary.go` around lines 950 - 954, This loop builds a slice of vendor-type specs (baseSpecs) from allTests and calls appendSuiteNames to mutate their Name fields via pointer semantics, but there's no comment explaining that dependency; add a short comment above the block clarifying that baseSpecs intentionally references the ExtensionTestSpec objects from allTests so appendSuiteNames can update their Name fields in-place (mentioning baseSpecs, allTests, appendSuiteNames and ExtensionTestSpecs/ExtensionTestSpec) to prevent future maintainers from removing or refactoring this apparent no-op.
🤖 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.
Nitpick comments:
In `@pkg/test/extensions/binary.go`:
- Around line 950-954: This loop builds a slice of vendor-type specs (baseSpecs)
from allTests and calls appendSuiteNames to mutate their Name fields via pointer
semantics, but there's no comment explaining that dependency; add a short
comment above the block clarifying that baseSpecs intentionally references the
ExtensionTestSpec objects from allTests so appendSuiteNames can update their
Name fields in-place (mentioning baseSpecs, allTests, appendSuiteNames and
ExtensionTestSpecs/ExtensionTestSpec) to prevent future maintainers from
removing or refactoring this apparent no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 55ae1310-e70d-4b53-bc41-4cde1b809b65
📒 Files selected for processing (1)
pkg/test/extensions/binary.go
openshift-ci
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d9d3e22
New tests seen in this PR at sha: d9d3e22
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
… tests External binary tests loaded via TestBinaries.ListTests() were missing the processing pipeline that origin built-in tests go through in InitializeOpenShiftTestsExtensionFramework(). This caused external binary tests to lack [Suite:openshift/conformance/...] tags, dropping the openshift/conformance suite from ~4000 to ~190 tests on OCP 4.21+. Apply the same processing steps to external binary test specs: - filterOutDisabledSpecs(): removes tests known to be broken - addEnvironmentSelectors(): adds [Skipped:] markers for platform/ network/topology-specific tests - addLabelsToSpecs(): adds labels like [Serial] where needed - appendSuiteNames(): adds [Suite:openshift/conformance/...] tags All four functions have guards to skip tests that already have the relevant annotations, so they are safe to call on specs that may already be partially processed by their source binary. Tested on OCP 4.21.8 GA cluster: - Before fix: 184 tests in openshift/conformance suite - After fix: 3895 tests in openshift/conformance suite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
Scheduling required tests: |
Contributor
|
@bshaw7: The following tests 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 14c7947
New tests seen in this PR at sha: 14c7947
|
Member
|
Tests can be part of multiple suites, the Suite name thing in tests at all we should get rid of. It's clear from the artifacts what suite was run.
Context of this? Suites are working fine in CI. All 4.21+ jobs are running their normal quantity of tests. I'm not sure what |
stbenjam
reviewed
Jun 3, 2026
Comment on lines
+950
to
+957
| var baseSpecs extensiontests.ExtensionTestSpecs | ||
| for _, spec := range allTests { | ||
| baseSpecs = append(baseSpecs, spec.ExtensionTestSpec) | ||
| } | ||
| baseSpecs = filterOutDisabledSpecs(baseSpecs) | ||
| addEnvironmentSelectors(baseSpecs) | ||
| addLabelsToSpecs(baseSpecs) | ||
| appendSuiteNames(baseSpecs) |
Member
There was a problem hiding this comment.
Changing test names breaks significant amount of tooling for historical comparison, you would have to account for these renames in openshift-eng/ci-test-mapping.
However, there is not a 1-to-1 linkage of "suite name" to "test". Tests are composable into many different kinds of suites, the pattern of including the name in tests should be something that goes away
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TestBinaries.ListTests()loads tests from external extension binaries (k8s-tests-ext, ovn-kubernetes-tests-ext, etc.) but never callsappendSuiteNames()on them[Suite:openshift/conformance/...]tags, the conformance suite filter excludes these testsopenshift/conformancesuite to drop from ~4000 to ~190 tests on OCP 4.21+Root Cause
appendSuiteNames()is called for origin's built-in tests duringInitializeOpenShiftTestsExtensionFramework()(line 104), but theTestBinaries.ListTests()code path for external binaries never calls it. Pre-4.20, all tests were built-in so this was never an issue. Starting in 4.20+, tests were moved to external binaries and the tagging step was missed.Fix
Call
appendSuiteNames()on the aggregated external binary test specs afterListTests()returns them. The function already has a guard (if strings.Contains(spec.Name, "[Suite:")) that skips tests already tagged, so it's safe for tests that already have suite tags from their own binaries.Test Plan
openshift-testsbinary fromrelease-4.21branchopenshift-tests run openshift/conformance --dry-runwith stock binary: 184 testsgo test ./pkg/test/extensions/...🤖 Generated with Claude Code
Summary by CodeRabbit