CNTRLPLANE-3174: Add unit tests for CPO controller packages#8184
CNTRLPLANE-3174: Add unit tests for CPO controller packages#8184openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis pull request adds many unit tests across the control-plane-operator repository (provider config helpers, manifest constructors, CSI/KubeVirt reconciliation, CVO payloads and removal lists, imageprovider, MCS machine config pools, OpenShift API services, clusterpolicy config adaptation, and multiple v2 components including etcd, ingress, router, snapshotcontroller, endpoint resolver, kms, and storage env replacement). It also updates ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
9368643 to
9351417
Compare
openshift-ci
bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
bot
added
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8184 +/- ##
==========================================
+ Coverage 32.73% 33.91% +1.17%
==========================================
Files 767 767
Lines 92686 93161 +475
==========================================
+ Hits 30342 31591 +1249
+ Misses 59761 58940 -821
- Partials 2583 2630 +47 🚀 New features to boost your workflow:
|
Exclude mock files and files with no executable code from codecov: - Mock files: **/*_mock.go, **/*mock*.go - 18 struct/constant-only files across the repo Ref: CNTRLPLANE-3174 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
abfdc37 to
ce1e299
Compare
bryan-cox
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-3174 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 task 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
removed
the
do-not-merge/work-in-progress
|
@bryan-cox: This pull request references CNTRLPLANE-3174 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 task 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.
🧹 Nitpick comments (2)
control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go (1)
14-16: Avoid parallel execution in tests that share env-sensitive behavior.
UseHCPRouteris influenced byMANAGED_SERVICE(Line 120), so keepingTestUseHCPRouterparallel (Line 15, Line 73) makes this file more brittle to future case additions and refactors.♻️ Suggested change
func TestUseHCPRouter(t *testing.T) { - t.Parallel() - tests := []struct { name string hcp *hyperv1.HostedControlPlane expected bool }{ @@ for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - t.Parallel() g := NewWithT(t) g.Expect(UseHCPRouter(tc.hcp)).To(Equal(tc.expected)) }) } }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 72-74, 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go` around lines 14 - 16, TestUseHCPRouter should not run in parallel because UseHCPRouter's behavior depends on the MANAGED_SERVICE environment variable; remove the t.Parallel() call (or otherwise disable parallelization) from TestUseHCPRouter so the test runs serially and cannot race with other env-sensitive tests, and also remove or avoid t.Parallel() in any other tests in this file that exercise UseHCPRouter or read MANAGED_SERVICE (e.g., the tests around lines referenced) to ensure deterministic environment-dependent behavior.control-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.go (1)
18-22: Replace string-switched assertions with per-case validators.Dispatching assertions by
tc.nameis fragile; a renamed case can silently skip validation. You already have avalidatefield—use it to keep assertions strongly bound to each case.♻️ Suggested structure
tests := []struct { name string platform hyperv1.PlatformType - validate func(g Gomega, resources []interface{ GetName() string }) + validate func(g Gomega, resources []client.Object) }{ { name: "When platform is IBMCloud, it should return 4 resources", platform: hyperv1.IBMCloudPlatform, + validate: func(g Gomega, resources []client.Object) { + g.Expect(resources).To(HaveLen(4)) + }, }, // ...other cases with inline validate funcs... } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) resources := ResourcesToRemove(tc.platform) - switch tc.name { - // ... - } + tc.validate(g, resources) }) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 76-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.go` around lines 18 - 22, The table-driven test currently dispatches assertions by switching on tc.name which is brittle; instead, use the existing validate field per test case: populate each test case's validate func (signature validate func(g Gomega, resources []interface{ GetName() string })) with the assertions specific to that case, then in the test loop call tc.validate(g, resources) rather than branching on tc.name; update all cases (including those covered in lines ~76-172) to assign concrete validators and remove the switch-on-name logic so each case's assertions are strongly bound to the test case definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@control-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.go`:
- Around line 18-22: The table-driven test currently dispatches assertions by
switching on tc.name which is brittle; instead, use the existing validate field
per test case: populate each test case's validate func (signature validate
func(g Gomega, resources []interface{ GetName() string })) with the assertions
specific to that case, then in the test loop call tc.validate(g, resources)
rather than branching on tc.name; update all cases (including those covered in
lines ~76-172) to assign concrete validators and remove the switch-on-name logic
so each case's assertions are strongly bound to the test case definition.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go`:
- Around line 14-16: TestUseHCPRouter should not run in parallel because
UseHCPRouter's behavior depends on the MANAGED_SERVICE environment variable;
remove the t.Parallel() call (or otherwise disable parallelization) from
TestUseHCPRouter so the test runs serially and cannot race with other
env-sensitive tests, and also remove or avoid t.Parallel() in any other tests in
this file that exercise UseHCPRouter or read MANAGED_SERVICE (e.g., the tests
around lines referenced) to ensure deterministic environment-dependent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 01d04e67-31e0-44d7-acb2-a3f900a650e8
📒 Files selected for processing (20)
codecov.ymlcontrol-plane-operator/controllers/hostedcontrolplane/cloud/aws/providerconfig_test.gocontrol-plane-operator/controllers/hostedcontrolplane/cloud/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/common/manifests_test.gocontrol-plane-operator/controllers/hostedcontrolplane/csi/kubevirt/kubevirt_test.gocontrol-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile_test.gocontrol-plane-operator/controllers/hostedcontrolplane/oapi/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/clusterpolicy/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/configoperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go
|
/verified by ut |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Add behavior-driven unit tests for top-level control-plane-operator controller packages: - imageprovider: GetImage, ImageExist, NewFromImages, GetMissingImages, ComponentImages - cloud: ProviderConfigKey for AWS/Azure/OpenStack/unknown/empty - cloud/aws: AWSKMSCredsSecret name and namespace - cvo: ResourcesToRemove for IBMCloud/PowerVS vs default platforms - common: PullSecret, DefaultServiceAccount, KubeadminPasswordSecret, volume factory functions - oapi: ReconcileOpenShiftAPIService, ReconcileOAuthAPIService, ReconcileOLMPackageServerService - mcs: masterConfigPool, workerConfigPool factory functions - csi/kubevirt: getStorageDriverType, storage class reconcilers, daemonset image lookup, CSI driver, volume snapshot classes, RBAC All tests use Gherkin-style naming, gomega assertions, table-driven patterns, and parallel execution. Ref: CNTRLPLANE-3174 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
ce1e299 to
508c853
Compare
openshift-ci-robot
removed
the
verified
|
Good catch — replaced the Updated in the latest push. |
|
@bryan-cox: This pull request references CNTRLPLANE-3174 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 task 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: 1
🧹 Nitpick comments (3)
control-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go (1)
61-63: Rebindtcin parallel subtests for clarity and portability.Lines 61, 134, and 254 use the pattern
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel(). While this is safe with Go 1.25.3 (the repository's version), explicitly rebindingtcat the loop level clarifies intent and protects against future go.mod edits.Suggested patch
for _, tc := range testCases { + tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel()Also applies to: 134–136, 254–256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go` around lines 61 - 63, The loop variable tc in the table-driven tests over testCases should be explicitly rebound to avoid capture issues in the parallel subtests; inside each loop before calling t.Run (where the closure calls t.Parallel()), add a short rebind like tc := tc so the closure captures the loop-local copy. Update each occurrence that follows the pattern for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) — specifically the blocks referencing testCases and t.Run — to include the rebind immediately prior to t.Run.control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go (1)
194-199: Consider using namespace-qualified resource keys in assertions.
extractResourceNamesloses namespace/kind, which can make tests pass incorrectly if duplicate names are introduced later. Prefer keys likenamespace/name(and optionally kind) for stronger assertions.As per coding guidelines, "
**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go` around lines 194 - 199, The helper extractResourceNames currently returns only object names which can collide across namespaces; change extractResourceNames to produce namespace-qualified keys (e.g., obj.GetNamespace()+"/"+obj.GetName()) and update any assertions that consume it to compare against "namespace/name" strings (optionally include kind if tests need stronger uniqueness). Locate and modify the extractResourceNames function and the tests that call it to assert the new namespace-qualified keys instead of plain names.control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go (1)
14-16: Optional: explicitly set MANAGED_SERVICE env var in TestUseHCPRouter for clarityWhile the suggested fix is good practice for self-contained tests, the claimed risk of nondeterminism is overstated. Go's test runner executes top-level test functions sequentially—only subtests marked with
t.Parallel()run concurrently with other subtests from the same parent.TestUseHCPRouterandTestUseHCPRouterAroHCPdo not interleave. Additionally,TestUseHCPRouterAroHCPintentionally avoids parallelism (as documented in lines 80–82) and scopest.Setenv()to individual subtests, preventing env var leakage.The suggestion to explicitly set
MANAGED_SERVICEinTestUseHCPRouterremains valid for clarity and test isolation, but it is not required to fix an actual correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go` around lines 14 - 16, The test TestUseHCPRouter should explicitly set the MANAGED_SERVICE environment variable to a deterministic value to make the test self-contained and clear: inside TestUseHCPRouter, call t.Setenv("MANAGED_SERVICE", "<appropriate-value>") (using the same sentinel value convention as other tests) before invoking the code under test; this mirrors the scoping used in TestUseHCPRouterAroHCP and prevents any ambiguity about environment assumptions when locating the logic tied to TestUseHCPRouter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/component_test.go`:
- Around line 124-126: The test currently only calls
t.Setenv(rhobsmonitoring.EnvironmentVariable, tc.rhobsEnvValue) when
tc.rhobsEnvValue != "", which lets ambient process env leak into cases expecting
an unset value; change the test so every subtest unconditionally calls
t.Setenv(rhobsmonitoring.EnvironmentVariable, tc.rhobsEnvValue) (so an empty
string will clear the variable for that subtest) — update the code around the
rhobsmonitoring.EnvironmentVariable usage in the component_test.go test helper
where tc.rhobsEnvValue is handled.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go`:
- Around line 194-199: The helper extractResourceNames currently returns only
object names which can collide across namespaces; change extractResourceNames to
produce namespace-qualified keys (e.g., obj.GetNamespace()+"/"+obj.GetName())
and update any assertions that consume it to compare against "namespace/name"
strings (optionally include kind if tests need stronger uniqueness). Locate and
modify the extractResourceNames function and the tests that call it to assert
the new namespace-qualified keys instead of plain names.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go`:
- Around line 14-16: The test TestUseHCPRouter should explicitly set the
MANAGED_SERVICE environment variable to a deterministic value to make the test
self-contained and clear: inside TestUseHCPRouter, call
t.Setenv("MANAGED_SERVICE", "<appropriate-value>") (using the same sentinel
value convention as other tests) before invoking the code under test; this
mirrors the scoping used in TestUseHCPRouterAroHCP and prevents any ambiguity
about environment assumptions when locating the logic tied to TestUseHCPRouter.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go`:
- Around line 61-63: The loop variable tc in the table-driven tests over
testCases should be explicitly rebound to avoid capture issues in the parallel
subtests; inside each loop before calling t.Run (where the closure calls
t.Parallel()), add a short rebind like tc := tc so the closure captures the
loop-local copy. Update each occurrence that follows the pattern for _, tc :=
range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() ... }) —
specifically the blocks referencing testCases and t.Run — to include the rebind
immediately prior to t.Run.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c59c7c35-bc25-4914-bb37-bce50adfe947
📒 Files selected for processing (19)
control-plane-operator/controllers/hostedcontrolplane/cloud/aws/providerconfig_test.gocontrol-plane-operator/controllers/hostedcontrolplane/cloud/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/common/manifests_test.gocontrol-plane-operator/controllers/hostedcontrolplane/csi/kubevirt/kubevirt_test.gocontrol-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile_test.gocontrol-plane-operator/controllers/hostedcontrolplane/oapi/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/clusterpolicy/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/configoperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go
✅ Files skipped from review due to trivial changes (8)
- control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/configoperator/deployment_test.go
- control-plane-operator/controllers/hostedcontrolplane/common/manifests_test.go
- control-plane-operator/controllers/hostedcontrolplane/oapi/service_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/clusterpolicy/config_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd_test.go
- control-plane-operator/controllers/hostedcontrolplane/csi/kubevirt/kubevirt_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- control-plane-operator/controllers/hostedcontrolplane/cloud/aws/providerconfig_test.go
- control-plane-operator/controllers/hostedcontrolplane/cloud/config_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/cvo/reconcile_test.go
Add behavior-driven unit tests for v2 control-plane-operator controller packages using the CPO component framework: - v2/kas/kms: NewAWSKMSProvider, GenerateKMSEncryptionConfig, GenerateKMSPodConfig - v2/storage: setVersions, replaceEnvVars, setOperatorImageReferences (data plane vs control plane image routing) - v2/configoperator: isExternalInfraKubevirt credential checks - v2/cvo: preparePayloadScript, resourcesToRemove, isManagementClusterMetricsAccessEnabled (RHOBS/ROSA detection) - v2/etcd: buildEtcdInitContainer, buildEtcdDefragControllerContainer, isManagedETCD, defragControllerPredicate - v2/snapshotcontroller: isStorageAndCSIManaged platform predicate - v2/clusterpolicy: adaptConfig TLS/cipher/feature gate configuration - v2/router/util: UseHCPRouter platform and endpoint access logic - v2/endpoint_resolver: predicate annotation-based monitoring toggle - v2/ingressoperator: isIngressCapabilityEnabled capability check All tests use Gherkin-style naming, gomega assertions, table-driven patterns, and parallel execution where compatible. Ref: CNTRLPLANE-3174 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
508c853 to
de76040
Compare
|
@bryan-cox: This pull request references CNTRLPLANE-3174 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 task 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.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go (1)
31-33: Tighten the negative matcher to the removal commandLine 32 currently asserts the filename is absent anywhere in the script, which is broader than the behavior under test. Matching the full
rm -f ...command (like Line 41) will reduce false failures from unrelated occurrences of the filename.Suggested patch
- g.Expect(script).NotTo(ContainSubstring("0000_50_console-operator_01-oauth.yaml")) + g.Expect(script).NotTo(ContainSubstring("rm -f /var/payload/release-manifests/0000_50_console-operator_01-oauth.yaml"))As per coding guidelines,
Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go` around lines 31 - 33, The test's assertions lambda currently checks only that "0000_50_console-operator_01-oauth.yaml" is absent, which is too broad; update the negative matcher inside the assertions func (the anonymous assertions function that takes g Gomega and script string) to assert the full removal command is absent instead (e.g., match the exact "rm -f 0000_50_console-operator_01-oauth.yaml" string used elsewhere in the diff) so the test only fails if the actual rm command is present and not for unrelated occurrences of the filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.go`:
- Around line 31-33: The test's assertions lambda currently checks only that
"0000_50_console-operator_01-oauth.yaml" is absent, which is too broad; update
the negative matcher inside the assertions func (the anonymous assertions
function that takes g Gomega and script string) to assert the full removal
command is absent instead (e.g., match the exact "rm -f
0000_50_console-operator_01-oauth.yaml" string used elsewhere in the diff) so
the test only fails if the actual rm command is present and not for unrelated
occurrences of the filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 253aedb2-2cfe-4353-a39b-0b24955d852b
📒 Files selected for processing (11)
control-plane-operator/controllers/hostedcontrolplane/v2/clusterpolicy/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/configoperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/envreplace_test.go
✅ Files skipped from review due to trivial changes (3)
- control-plane-operator/controllers/hostedcontrolplane/v2/cvo/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- control-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/configoperator/deployment_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component_test.go
|
/lgtm |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
Scheduling tests matching the |
|
/verified ut |
|
@bryan-cox: The 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. |
|
/verified by ut |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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 Resultse2e-aks
e2e-aws
|
|
@bryan-cox: 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
What this PR does / why we need it:
Adds behavior-driven unit tests for control-plane-operator controller packages that previously had zero test coverage, and updates
codecov.ymlto exclude files with no executable code.Commit 1: codecov exclusions
**/*_mock.go,**/*mock*.go) from coverageCommit 2: Top-level CPO controller tests — 8 files
imageproviderimageprovider_test.goGetImage,ImageExist,NewFromImages,GetMissingImages,ComponentImagescloudconfig_test.goProviderConfigKeyfor AWS/Azure/OpenStack/unknown/emptycloud/awsproviderconfig_test.goAWSKMSCredsSecretname and namespacecvoreconcile_test.goResourcesToRemovefor IBMCloud/PowerVS vs default platformscommonmanifests_test.goPullSecret,DefaultServiceAccount,KubeadminPasswordSecret, volume factory functionsoapiservice_test.goReconcileOpenShiftAPIService,ReconcileOAuthAPIService,ReconcileOLMPackageServerServicemcsreconcile_test.gomasterConfigPool,workerConfigPoolfactory functionscsi/kubevirtkubevirt_test.gogetStorageDriverType, storage class reconcilers, daemonset image lookup, CSI driver, volume snapshot classes, RBAC bindingsCommit 3: v2 CPO framework controller tests — 11 files
v2/kas/kmsaws_test.goNewAWSKMSProvider,GenerateKMSEncryptionConfig,GenerateKMSPodConfigv2/storageenvreplace_test.gosetVersions,replaceEnvVars,setOperatorImageReferences(data plane vs control plane image routing)v2/configoperatordeployment_test.goisExternalInfraKubevirtnil-safe credential checksv2/cvodeployment_test.gopreparePayloadScript(manifest filtering, oauth, feature sets),resourcesToRemovev2/cvocomponent_test.goisManagementClusterMetricsAccessEnabled(RHOBS/ROSA detection)v2/etcdetcd_test.gobuildEtcdInitContainer,buildEtcdDefragControllerContainer,isManagedETCD,defragControllerPredicatev2/snapshotcontrollercomponent_test.goisStorageAndCSIManagedplatform predicatev2/clusterpolicyconfig_test.goadaptConfigTLS/cipher/feature gate configurationv2/router/utilutil_test.goUseHCPRouterplatform and endpoint access logicv2/endpoint_resolvercomponent_test.gopredicateannotation-based monitoring togglev2/ingressoperatorcomponent_test.goisIngressCapabilityEnabledcapability checkTotal: 170 new test cases across 19 files
Testing approach
All tests generated using the
behavior-driven-testingClaude Code skill:"When ..., it should ...")NewWithT(t)t.Parallel()at both levelsHow to verify
make test make lintRef: CNTRLPLANE-3174
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores