CNTRLPLANE-3174: Add unit tests for v2 CPO controller packages#8215
Merged
openshift-merge-bot[bot] merged 7 commits intoopenshift:mainfrom Apr 15, 2026
Merged
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
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. |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a large suite of unit tests across the hosted control plane operator. New table-driven, parallelized tests (using Gomega) were added for many packages and components (konnectivity, konnectivity_agent, CAPI manager/provider, cloud-controller-managers, cloud-credential-operator, dns-operator, feature-gate, ignitionserver proxy, Karpenter, kube-controller-manager, machine approver, node-tuning, oauth-apiserver, OLM components, PKI operator, and others). Tests cover predicates, component option methods, and adaptation logic for Deployments, Services, Secrets, ConfigMaps, Jobs, CronJobs, ServiceMonitors, manifest-loading helpers, and related helpers. No production code or exported/public API declarations were changed. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@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
added
area/control-plane-operator
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
f4b530e to
c6339f1
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
control-plane-operator/controllers/hostedcontrolplane/konnectivity/params_test.go (1)
40-50: Consider validating theReferencefield in the second test case.The second test case validates that
paramsandparams.OwnerRefare not nil, but doesn't check theReferencefield like the first test case does. Consider adding validation for theReferencefield to ensure it handles empty metadata correctly (e.g., checking ifReference.Nameis empty whenObjectMetais empty).💡 Suggested enhancement
validate: func(t *testing.T, params *KonnectivityServiceParams) { g := NewWithT(t) g.Expect(params).ToNot(BeNil()) g.Expect(params.OwnerRef).ToNot(BeNil()) + g.Expect(params.OwnerRef.Reference).ToNot(BeNil()) + g.Expect(params.OwnerRef.Reference.Name).To(BeEmpty()) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/konnectivity/params_test.go` around lines 40 - 50, The second test case for KonnectivityServiceParams (the table entry named "When HCP has empty metadata it should still create params") currently only asserts params and params.OwnerRef are non-nil; update its validate function to also assert the params.Reference exists and that params.Reference.Name is empty (or matches the expected empty-name behavior) to mirror the first test's Reference validation and ensure empty HostedControlPlane.ObjectMeta is handled correctly.control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment_test.go (1)
462-487: Stabilize parallel subtests by avoiding in-place mutation of shared test case state.The test runs subtests in parallel (line 464:
t.Parallel()) within an outer parallel test (line 30), creating a data race when multiple subtests concurrently modify the sharedtc.existingObjectsslice (line 478). Each subtest should work with its own copy to remain independent.Create a local copy and use it instead:
Proposed refactor
for _, tc := range tests { + tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() scheme := runtime.NewScheme() _ = corev1.AddToScheme(scheme) + existingObjects := append([]client.Object(nil), tc.existingObjects...) // Add feature-gate configmap if not already in existingObjects hasFeatureGateConfigMap := false - for _, obj := range tc.existingObjects { + for _, obj := range existingObjects { if cm, ok := obj.(*corev1.ConfigMap); ok && cm.Name == "feature-gate" { hasFeatureGateConfigMap = true break } } if !hasFeatureGateConfigMap { - tc.existingObjects = append(tc.existingObjects, &corev1.ConfigMap{ + existingObjects = append(existingObjects, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "feature-gate", Namespace: tc.hcp.Namespace, }, Data: map[string]string{ "feature-gate.yaml": `{"apiVersion":"config.openshift.io/v1","kind":"FeatureGate","spec":{},"status":{"featureGates":[]}}`, }, }) } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.existingObjects...).Build() + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingObjects...).Build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment_test.go` around lines 462 - 487, The subtests call t.Parallel() but mutate the shared tc.existingObjects in-place when appending the "feature-gate" ConfigMap, causing a data race; make a local copy of tc.existingObjects at the start of the subtest (e.g., localExisting := append([]runtime.Object(nil), tc.existingObjects...)) and operate on that copy when checking/adding the ConfigMap so the test uses localExisting instead of mutating tc.existingObjects; update any subsequent references in the subtest to use the local copy (references: tc.existingObjects, t.Parallel(), feature-gate ConfigMap creation).control-plane-operator/controllers/hostedcontrolplane/v2/kcm/config_test.go (1)
88-90: The explicittc := tcshadowing is optional and not required in Go 1.22+.While the pattern can improve code clarity, it is not necessary for deterministic behavior with Go 1.25.3. Go 1.22 (released February 2024) fixed range-loop variable scoping semantics, ensuring variables are properly scoped per iteration. Additionally, the project convention shows this pattern is not used elsewhere in the codebase—other parallel table-driven tests like those in
token-minter/tokenminter_test.gofollow the same structure without shadowing. You may add it for explicit clarity if preferred, but it is not a 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/kcm/config_test.go` around lines 88 - 90, The explicit per-iteration shadowing (tc := tc) in the table-driven test loop is unnecessary with Go 1.22+ and inconsistent with project convention; remove the redundant "tc := tc" from the test body around t.Run so the loop uses the loop variable directly (the surrounding code to change is the table loop over tests and the t.Run/t.Parallel block).control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go (1)
108-119: TightenNO_PROXYassertions to exact host set.Validating only host substrings can miss extra or malformed entries. Exact token comparison would make these cases more robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go` around lines 108 - 119, The test currently checks NO_PROXY by substring containment; instead parse the found noProxyEnv.Value into comma-separated tokens, trim whitespace and drop empty entries, then assert the token set exactly matches tc.expectedNoProxyHosts (use a set-aware matcher such as Gomega's ConsistOf or compare sorted slices) so extra or malformed entries are detected; locate the logic around olmOperatorContainer.Env, noProxyEnv and tc.expectedNoProxyHosts in deployment_test.go and replace the per-host ContainSubstring loop with the exact-token comparison described.control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go (1)
144-155: Prefer exactNO_PROXYtoken validation over substring checks.Current
ContainSubstringchecks can pass even with malformed values or unexpected extra hosts. Consider asserting exact comma-token contents for stronger behavior verification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go` around lines 144 - 155, Replace the fragile substring assertions for the NO_PROXY env var in deployment_test.go with exact token validation: locate the NO_PROXY env var in packageServerContainer.Env, read noProxyEnv.Value, split by commas, trim whitespace from each token, and compare the resulting slice/set to tc.expectedNoProxyHosts using an equality or set-equality assertion (e.g., comparing sorted slices or using Gomega's ConsistOf) so the test verifies exact comma-separated hosts regardless of order or extra spaces.control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/deployment_test.go (1)
140-143: Prefer name-based volume assertions over index-based checks.Line [140]-Line [143] depends on volume ordering, which makes the test fragile to harmless manifest reorderings.
Refactor idea
- g.Expect(deployment.Spec.Template.Spec.Volumes).To(HaveLen(3)) - g.Expect(deployment.Spec.Template.Spec.Volumes[0].Secret.SecretName).To(Equal("other-secret")) - g.Expect(deployment.Spec.Template.Spec.Volumes[1].Secret.SecretName).To(Equal("updated-creds")) - g.Expect(deployment.Spec.Template.Spec.Volumes[2].ConfigMap.Name).To(Equal("some-configmap")) + vols := map[string]corev1.Volume{} + for _, v := range deployment.Spec.Template.Spec.Volumes { + vols[v.Name] = v + } + g.Expect(vols).To(HaveLen(3)) + g.Expect(vols["other-volume"].Secret.SecretName).To(Equal("other-secret")) + g.Expect(vols[cloudCredsVolumeName].Secret.SecretName).To(Equal("updated-creds")) + g.Expect(vols["yet-another-volume"].ConfigMap.Name).To(Equal("some-configmap"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/deployment_test.go` around lines 140 - 143, The test is fragile because it asserts volumes by index on deployment.Spec.Template.Spec.Volumes; replace index-based checks with name-based assertions that locate volumes by their volume.Name (or by matching Secret.SecretName/ConfigMap.Name) and then assert their contents. Update the assertions in deployment_test.go to either use a helper like findVolumeByName(deployment.Spec.Template.Spec.Volumes, "volume-name") and then check the returned volume's Secret.SecretName/ConfigMap.Name, or use Gomega's ContainElement with a matcher that checks volume.Name and the nested Secret/ConfigMap fields for "other-secret", "updated-creds", and "some-configmap" so ordering no longer matters.control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.go (1)
63-64: Select containers by name instead of fixed index.Using
Containers[0]makes these tests order-dependent and can validate the wrong container if manifest ordering changes.♻️ Suggested refactor
- container := deploymentObj.Spec.Template.Spec.Containers[0] + var container *corev1.Container + for i := range deploymentObj.Spec.Template.Spec.Containers { + if deploymentObj.Spec.Template.Spec.Containers[i].Name == ComponentName { + container = &deploymentObj.Spec.Template.Spec.Containers[i] + break + } + } + g.Expect(container).ToNot(BeNil())Also applies to: 112-113, 131-132, 155-156, 189-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.go` around lines 63 - 64, Tests currently index into deploymentObj.Spec.Template.Spec.Containers by position (Containers[0]) which is order-dependent; change each assertion to locate the container by its Name field (e.g., find the container where c.Name == "<expected-container-name>") and fail the test if not found, then assert on c.Image (and other fields) for that named container. Update all occurrences in deployment_test.go that reference Containers[0] (including the other similar assertions) to use this name-based lookup (or a small helper function like findContainerByName(deploymentObj, name)) so the tests validate the intended container regardless of ordering.control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment_test.go (1)
174-177: Assert concrete proxy env vars, not just non-empty env list.Current check can pass for unrelated env entries and miss proxy injection regressions.
🔍 Suggested assertion upgrade
// Check that proxy env vars are set on the container container := deployment.Spec.Template.Spec.Containers[0] - g.Expect(len(container.Env)).To(BeNumerically(">", 0)) + var hasHTTP, hasHTTPS, hasNO bool + for _, e := range container.Env { + switch e.Name { + case "HTTP_PROXY": + hasHTTP = true + case "HTTPS_PROXY": + hasHTTPS = true + case "NO_PROXY": + hasNO = true + } + } + g.Expect(hasHTTP).To(BeTrue()) + g.Expect(hasHTTPS).To(BeTrue()) + g.Expect(hasNO).To(BeTrue())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment_test.go` around lines 174 - 177, The test currently only asserts container.Env is non-empty; change it to assert for specific proxy env vars on the container (inspect deployment.Spec.Template.Spec.Containers[0] -> container and its container.Env slice) by checking that entries with Name "HTTP_PROXY", "HTTPS_PROXY" and "NO_PROXY" (and their lowercase variants "http_proxy", "https_proxy", "no_proxy" if you expect both) exist and have the expected values; implement this by iterating container.Env or using the test helpers to assert the presence of Env entries with those Name fields (and assert their Value equals the expected proxy string if available) instead of the len(container.Env) > 0 check.control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go (1)
160-163: Strengthen the zero-NodePort assertion for the no-port case.When
expectedPort == 0, the test currently skips port assertion; assertingsvc.Spec.Ports[0].NodePort == 0would harden the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go` around lines 160 - 163, The test currently only asserts NodePort when tc.expectedPort > 0; update the assertion logic around svc.Spec.Ports to explicitly check the no-port case by asserting svc.Spec.Ports has length 1 and that svc.Spec.Ports[0].NodePort == 0 when tc.expectedPort == 0, while keeping the existing equality assertion when tc.expectedPort > 0 (referencing tc.expectedPort and svc.Spec.Ports[0].NodePort).
🤖 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/capi_manager/deployment_test.go`:
- Around line 70-73: The test for version "4.18.0" currently only validates
presence of expectedArgs and misses asserting the absence of the
MachineSetPreflightChecks feature-gate; update the test in deployment_test.go
(the case with version "4.18.0" and expectedArgs == []string{}) to explicitly
assert that none of the actual container args contain
"--feature-gates=MachineSetPreflightChecks=false" (i.e., after the existing
positive checks, add a negative check that scans the actual args and fails if
any arg contains "MachineSetPreflightChecks=false"); reference the test case's
expectedArgs variable and the MachineSetPreflightChecks flag when adding this
assertion.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cloud_credential_operator/deployment_test.go`:
- Around line 92-101: Test environment leaks because the test only sets
HTTP_PROXY/HTTPS_PROXY/NO_PROXY when tc.httpProxy, tc.httpsProxy, tc.noProxy are
non-empty, allowing host runner values to bleed into "no proxy" cases; update
the test setup in deployment_test.go to explicitly clear/unset these proxy env
vars when the test case value is empty (use t.Setenv("HTTP_PROXY", tc.httpProxy)
etc. with empty string for unset or call os.Unsetenv when tc.* is empty) so
every test case deterministically controls HTTP_PROXY, HTTPS_PROXY and NO_PROXY
regardless of runner environment.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/deployment_test.go`:
- Around line 89-104: The test only asserts the presence of the trust-bundle
mount when tc.expectTrustBundleMount is true; add the complementary negative
assertion when tc.expectTrustBundleMount is false by checking the same
container(s) for absence of a mount with name tc.expectedVolumeName (e.g.,
inspect deployment.Spec.Template.Spec.Containers[0].VolumeMounts or iterate
containers and their VolumeMounts) and assert foundMount is false with
g.Expect(foundMount).To(BeFalse()) so regressions that inadvertently add the
trusted-ca mount are caught.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/kubeconfig_test.go`:
- Around line 109-111: The call to corev1.AddToScheme(scheme) currently ignores
its error; update the test to capture and check that error (e.g., err :=
corev1.AddToScheme(scheme)) and fail the test if non-nil using the test helper
in this file (e.g., t.Fatalf or require.NoError) before creating fakeClient with
fake.NewClientBuilder().WithScheme(scheme).WithObjects(...).Build(); this
ensures scheme registration problems are surfaced immediately when running
kubeconfig_test.go.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/deployment_test.go`:
- Around line 140-151: The Guest-path NO_PROXY assertions only check for
presence of expected hosts and miss verifying that management-only hosts are
excluded; update the test in deployment_test.go to, after locating noProxyEnv
via catalogOperatorContainer.Env and asserting it is not nil and contains all
tc.expectedNoProxyHosts, also assert that the NO_PROXY string does NOT contain
any management-only/catalog-excluded hosts (add assertions against the known
management host names used in other tests or a new tc.unexpectedNoProxyHosts
list) so the guest case fails if management hosts are mistakenly included.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment_test.go`:
- Around line 84-157: The test currently calls the helper
checkCatalogImageOverides directly (reference checkCatalogImageOverides) so it
never exercises getCatalogImagesOverrides; either change the test to call
getCatalogImagesOverrides (reference getCatalogImagesOverrides) and supply the
necessary test doubles (a fake client or a minimal object with annotations and
the capabilityImageStream flag / image metadata provider) so the
annotation-to-map wiring and capability-driven branches are executed, or rename
the test to reflect it only validates the helper (e.g.,
TestCheckCatalogImageOverides) and keep the existing assertions; update the test
setup to construct the inputs required by getCatalogImagesOverrides (annotations
map on the resource and any mocked dependencies) and adjust the validate
closures to assert the returned map/error from getCatalogImagesOverrides
accordingly.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go`:
- Around line 157-161: The test misses the "no replica override" assertion: when
tc.expectedReplicas is nil you should assert that deployment.Spec.Replicas is
nil (or not set) to cover the negative path. Update the assertions around
tc.expectedReplicas in deployment_test.go so the if/else covers both
branches—keep the existing if (tc.expectedReplicas != nil) checks that
*deployment.Spec.Replicas equals *tc.expectedReplicas, and add an else that
calls g.Expect(deployment.Spec.Replicas).To(BeNil()) for the "it should not
override replicas" case (referencing tc.expectedReplicas and
deployment.Spec.Replicas).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/deployment_test.go`:
- Around line 132-140: The test currently only calls t.Setenv for proxy env vars
when tc.httpProxy/tc.httpsProxy/tc.noProxy are non-empty, letting parent process
env leak into the "no proxy" case; always call t.Setenv for each of
"HTTP_PROXY", "HTTPS_PROXY", and "NO_PROXY" with the corresponding tc.httpProxy,
tc.httpsProxy, tc.noProxy values (allowing empty string to clear the variable)
so the tests are deterministic; update the block around the t.Setenv calls in
deployment_test.go (where tc and t are used) to unconditionally set those three
env vars for every test case.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/konnectivity/params_test.go`:
- Around line 40-50: The second test case for KonnectivityServiceParams (the
table entry named "When HCP has empty metadata it should still create params")
currently only asserts params and params.OwnerRef are non-nil; update its
validate function to also assert the params.Reference exists and that
params.Reference.Name is empty (or matches the expected empty-name behavior) to
mirror the first test's Reference validation and ensure empty
HostedControlPlane.ObjectMeta is handled correctly.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment_test.go`:
- Around line 174-177: The test currently only asserts container.Env is
non-empty; change it to assert for specific proxy env vars on the container
(inspect deployment.Spec.Template.Spec.Containers[0] -> container and its
container.Env slice) by checking that entries with Name "HTTP_PROXY",
"HTTPS_PROXY" and "NO_PROXY" (and their lowercase variants "http_proxy",
"https_proxy", "no_proxy" if you expect both) exist and have the expected
values; implement this by iterating container.Env or using the test helpers to
assert the presence of Env entries with those Name fields (and assert their
Value equals the expected proxy string if available) instead of the
len(container.Env) > 0 check.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/deployment_test.go`:
- Around line 140-143: The test is fragile because it asserts volumes by index
on deployment.Spec.Template.Spec.Volumes; replace index-based checks with
name-based assertions that locate volumes by their volume.Name (or by matching
Secret.SecretName/ConfigMap.Name) and then assert their contents. Update the
assertions in deployment_test.go to either use a helper like
findVolumeByName(deployment.Spec.Template.Spec.Volumes, "volume-name") and then
check the returned volume's Secret.SecretName/ConfigMap.Name, or use Gomega's
ContainElement with a matcher that checks volume.Name and the nested
Secret/ConfigMap fields for "other-secret", "updated-creds", and
"some-configmap" so ordering no longer matters.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go`:
- Around line 160-163: The test currently only asserts NodePort when
tc.expectedPort > 0; update the assertion logic around svc.Spec.Ports to
explicitly check the no-port case by asserting svc.Spec.Ports has length 1 and
that svc.Spec.Ports[0].NodePort == 0 when tc.expectedPort == 0, while keeping
the existing equality assertion when tc.expectedPort > 0 (referencing
tc.expectedPort and svc.Spec.Ports[0].NodePort).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.go`:
- Around line 63-64: Tests currently index into
deploymentObj.Spec.Template.Spec.Containers by position (Containers[0]) which is
order-dependent; change each assertion to locate the container by its Name field
(e.g., find the container where c.Name == "<expected-container-name>") and fail
the test if not found, then assert on c.Image (and other fields) for that named
container. Update all occurrences in deployment_test.go that reference
Containers[0] (including the other similar assertions) to use this name-based
lookup (or a small helper function like findContainerByName(deploymentObj,
name)) so the tests validate the intended container regardless of ordering.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/config_test.go`:
- Around line 88-90: The explicit per-iteration shadowing (tc := tc) in the
table-driven test loop is unnecessary with Go 1.22+ and inconsistent with
project convention; remove the redundant "tc := tc" from the test body around
t.Run so the loop uses the loop variable directly (the surrounding code to
change is the table loop over tests and the t.Run/t.Parallel block).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment_test.go`:
- Around line 462-487: The subtests call t.Parallel() but mutate the shared
tc.existingObjects in-place when appending the "feature-gate" ConfigMap, causing
a data race; make a local copy of tc.existingObjects at the start of the subtest
(e.g., localExisting := append([]runtime.Object(nil), tc.existingObjects...))
and operate on that copy when checking/adding the ConfigMap so the test uses
localExisting instead of mutating tc.existingObjects; update any subsequent
references in the subtest to use the local copy (references: tc.existingObjects,
t.Parallel(), feature-gate ConfigMap creation).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go`:
- Around line 108-119: The test currently checks NO_PROXY by substring
containment; instead parse the found noProxyEnv.Value into comma-separated
tokens, trim whitespace and drop empty entries, then assert the token set
exactly matches tc.expectedNoProxyHosts (use a set-aware matcher such as
Gomega's ConsistOf or compare sorted slices) so extra or malformed entries are
detected; locate the logic around olmOperatorContainer.Env, noProxyEnv and
tc.expectedNoProxyHosts in deployment_test.go and replace the per-host
ContainSubstring loop with the exact-token comparison described.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go`:
- Around line 144-155: Replace the fragile substring assertions for the NO_PROXY
env var in deployment_test.go with exact token validation: locate the NO_PROXY
env var in packageServerContainer.Env, read noProxyEnv.Value, split by commas,
trim whitespace from each token, and compare the resulting slice/set to
tc.expectedNoProxyHosts using an equality or set-equality assertion (e.g.,
comparing sorted slices or using Gomega's ConsistOf) so the test verifies exact
comma-separated hosts regardless of order or extra spaces.
🪄 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: Pro
Run ID: 1fac2b93-da00-4fa3-80a6-8104c1932bb5
⛔ Files ignored due to path filters (1)
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/testdata/zz_fixture_TestAdaptConfig.yamlis excluded by!**/testdata/**
📒 Files selected for processing (50)
control-plane-operator/controllers/hostedcontrolplane/konnectivity/params_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/secret_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/role_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_credential_operator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_credential_operator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/dnsoperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/dnsoperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/fg/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/fg/job_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/podmonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/secret_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/config_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/kubeconfig_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/konnectivity_agent/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/konnectivity_agent/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/cronjob_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/deployment_test.go
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
c6339f1 to
c832a62
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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go (1)
123-127:⚠️ Potential issue | 🟡 MinorMissing assertion for the “no replica override” path.
At Line 157-Line 161, tests only assert the override path. The
expectedReplicas == nilbranch still doesn't verify the deployment replicas remained unchanged.Suggested fix
deployment, err := assets.LoadDeploymentManifest(ComponentName) g.Expect(err).ToNot(HaveOccurred()) + var originalReplicas *int32 + if deployment.Spec.Replicas != nil { + v := *deployment.Spec.Replicas + originalReplicas = &v + } @@ if tc.expectedReplicas != nil { g.Expect(deployment.Spec.Replicas).ToNot(BeNil()) g.Expect(*deployment.Spec.Replicas).To(Equal(*tc.expectedReplicas)) + } else { + g.Expect(deployment.Spec.Replicas).To(Equal(originalReplicas)) }Also applies to: 157-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go` around lines 123 - 127, The test currently only asserts the replica-override branch; add an assertion for the "no replica override" path so that when expectedReplicas == nil the deployment's replica count is unchanged after calling adaptDeployment. Locate the test that calls assets.LoadDeploymentManifest(ComponentName) and adaptDeployment(cpContext, deployment) and add a check comparing deployment.Spec.Replicas (or the pointer value) to the original value saved before adaptDeployment when expectedReplicas is nil; ensure you reference the deployment variable, adaptDeployment function, and expectedReplicas in the assertion.
🧹 Nitpick comments (7)
control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component_test.go (1)
32-42: Extract the repeated KarpenterAutoNodefixture for maintainability.The same enabled-Karpenter object literal is repeated in multiple cases. A small helper would reduce duplication and make future updates safer.
♻️ Proposed refactor
+func enabledKarpenterAutoNode() *hyperv1.AutoNode { + return &hyperv1.AutoNode{ + Provisioner: hyperv1.ProvisionerConfig{ + Name: hyperv1.ProvisionerKarpenter, + Karpenter: &hyperv1.KarpenterConfig{ + Platform: hyperv1.AWSPlatform, + AWS: &hyperv1.KarpenterAWSConfig{ + RoleARN: "arn:aws:iam::123456789012:role/karpenter", + }, + }, + }, + } +} ... - autoNode: &hyperv1.AutoNode{ ... }, + autoNode: enabledKarpenterAutoNode(),Also applies to: 72-82, 89-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component_test.go` around lines 32 - 42, Extract the repeated Karpenter AutoNode literal into a small test helper (e.g., makeKarpenterAutoNode or newKarpenterAutoNode) that returns *hyperv1.AutoNode populated with Provisioner: hyperv1.ProvisionerConfig{Name: hyperv1.ProvisionerKarpenter, Karpenter: &hyperv1.KarpenterConfig{Platform: hyperv1.AWSPlatform, AWS: &hyperv1.KarpenterAWSConfig{RoleARN: "arn:aws:iam::123456789012:role/karpenter"}}}; replace the inline literals in the component_test.go cases that construct AutoNode (the instances using AutoNode, Provisioner, KarpenterConfig, KarpenterAWSConfig) with calls to this helper to remove duplication and reuse across other test cases.control-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.go (2)
137-162: Tighten the fake provider to validate component-key usage.Because
GetImageignorescomponentName, the test can’t catch wrong component lookups. Make the fake key-aware so incorrect keys fail assertions.🧪 Suggested stricter fake
type fakeReleaseImageProvider struct { version string - image string + images map[string]string } @@ func (f *fakeReleaseImageProvider) GetImage(componentName string) string { - return f.image + return f.images[componentName] } func (f *fakeReleaseImageProvider) ImageExist(name string) (string, bool) { - return f.image, true + image, ok := f.images[name] + return image, ok } @@ func (f *fakeReleaseImageProvider) ComponentImages() map[string]string { - return map[string]string{ - ComponentName: f.image, - } + return f.images }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.go` around lines 137 - 162, The fakeReleaseImageProvider currently ignores the componentName argument so tests can't detect wrong lookups; update fakeReleaseImageProvider to be key-aware by storing a map[string]string (or the expected key) and have GetImage(componentName string), ImageExist(name string) and ComponentImages() validate the provided componentName against that map (returning the matching image when present and an empty string/false when absent, or fail the test/assert/panic on unexpected keys) so incorrect component keys cause test failures; ensure references to ComponentName in ComponentImages still return the correct image from the new map.
107-116: Make the “updates existing env vars” case deterministic.This test currently depends on manifest internals. Seed the env vars explicitly before calling
adaptDeployment, and guardContainersbefore indexing so failures are clearer.♻️ Suggested test hardening
deployment, err := assets.LoadDeploymentManifest(ComponentName) g.Expect(err).ToNot(HaveOccurred()) +g.Expect(deployment.Spec.Template.Spec.Containers).ToNot(BeEmpty()) +container := &deployment.Spec.Template.Spec.Containers[0] +container.Env = []corev1.EnvVar{ + {Name: "RELEASE_VERSION", Value: "old-version"}, + {Name: "CLUSTER_NODE_TUNED_IMAGE", Value: "old-image"}, + {Name: "UNCHANGED_ENV", Value: "keep"}, +} // The deployment manifest already has these env vars with empty values, // so adaptDeployment should update them err = adaptDeployment(cpContext, deployment) g.Expect(err).ToNot(HaveOccurred()) -container := deployment.Spec.Template.Spec.Containers[0] +updatedContainer := deployment.Spec.Template.Spec.Containers[0] @@ -for _, env := range container.Env { +for _, env := range updatedContainer.Env {Also applies to: 110-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.go` around lines 107 - 116, The test relies on manifest internals; make it deterministic by explicitly setting the container and its env vars on the loaded deployment before calling adaptDeployment and checking results: after loading deployment via assets.LoadDeploymentManifest(ComponentName) set deployment.Spec.Template.Spec.Containers to contain at least one core container (e.g., ensure a container exists and set container.Env entries for the keys the test expects), then call adaptDeployment(cpContext, deployment); also add a guard that deployment.Spec.Template.Spec.Containers has length > 0 before indexing into Containers[0] so failures are clearer (reference: deployment, adaptDeployment, Containers, Env).control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/secret_test.go (1)
20-24: Remove redundantexpectedRoleARNfrom test cases.
expectedRoleARNmirrorsroleARNin every case, so it adds noise without extra validation value.♻️ Suggested simplification
testCases := []struct { name string roleARN string - expectedRoleARN string validateCredentials func(t *testing.T, g Gomega, credentials string) }{ { name: "When AWS role ARN is provided, it should generate correct credentials format", roleARN: "arn:aws:iam::123456789012:role/karpenter-role", - expectedRoleARN: "arn:aws:iam::123456789012:role/karpenter-role", validateCredentials: func(t *testing.T, g Gomega, credentials string) { @@ { name: "When different role ARN format is provided, it should be included correctly", roleARN: "arn:aws:iam::999999999999:role/my-custom-karpenter-role", - expectedRoleARN: "arn:aws:iam::999999999999:role/my-custom-karpenter-role", validateCredentials: func(t *testing.T, g Gomega, credentials string) { @@ { name: "When role ARN has path component, it should be preserved", roleARN: "arn:aws:iam::111111111111:role/path/to/role/karpenter", - expectedRoleARN: "arn:aws:iam::111111111111:role/path/to/role/karpenter", validateCredentials: func(t *testing.T, g Gomega, credentials string) { @@ - g.Expect(credentials).To(ContainSubstring(tc.expectedRoleARN)) + g.Expect(credentials).To(ContainSubstring(tc.roleARN))Also applies to: 27-30, 39-42, 48-51, 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/secret_test.go` around lines 20 - 24, Remove the redundant expectedRoleARN field from the test case structs and all references to it in the tests: simplify the testCases declaration by keeping only name and roleARN (and validateCredentials), update any table-driven test iterations to assert against roleARN directly (or remove assertions that compared expectedRoleARN to roleARN), and delete the now-unused expectedRoleARN variable occurrences; specifically update the testCases variable and any uses in the test helper logic that referenced expectedRoleARN so tests still validate credentials using roleARN only.control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go (1)
3-14: StrengthenNO_PROXYassertion to avoid false positives.At Line 117-Line 119,
ContainSubstringonly proves inclusion, not correctness of the full host set. This can miss regressions where unexpected hosts are added.Suggested test hardening
import ( + "strings" "testing" @@ g.Expect(noProxyEnv).ToNot(BeNil()) - for _, host := range tc.expectedNoProxyHosts { - g.Expect(noProxyEnv.Value).To(ContainSubstring(host)) - } + actualNoProxyHosts := strings.Split(noProxyEnv.Value, ",") + g.Expect(actualNoProxyHosts).To(ConsistOf(tc.expectedNoProxyHosts)) }) } }Also applies to: 109-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go` around lines 3 - 14, The test currently uses ContainSubstring against the NO_PROXY env var which can yield false positives; update the assertion that inspects the env var named "NO_PROXY" (the test that retrieves env := findEnvVar(envs, "NO_PROXY") or similar) to assert the full exact value instead of a substring (e.g., Expect(env.Value).To(Equal(expectedNoProxy)) or use a strict regex) and construct expectedNoProxy from the exact host list used by the deployment (join the known hosts into the exact string you expect) so the test fails when extra/unexpected hosts are present.control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go (1)
3-14: Use exact host-set matching forNO_PROXYexpectations.At Line 153-Line 155, substring matching is permissive and can hide extra/unexpected entries in
NO_PROXY.Suggested test hardening
import ( + "strings" "testing" @@ g.Expect(noProxyEnv).ToNot(BeNil()) - for _, host := range tc.expectedNoProxyHosts { - g.Expect(noProxyEnv.Value).To(ContainSubstring(host)) - } + actualNoProxyHosts := strings.Split(noProxyEnv.Value, ",") + g.Expect(actualNoProxyHosts).To(ConsistOf(tc.expectedNoProxyHosts))Also applies to: 145-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go` around lines 3 - 14, The test currently uses permissive substring matching to assert the container env var "NO_PROXY"; change the assertion in deployment_test.go to parse the NO_PROXY value by splitting on commas, trimming whitespace, and compare the resulting host set exactly against the expected host set (order-insensitive) instead of using substring/Contains checks so extra/unexpected entries are detected; update the assertion logic where the test inspects the container env var named "NO_PROXY" to perform this exact set comparison.control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go (1)
25-28: Drop the unusederrparameter fromvalidate.Every subtest passes
nilhere, so the leadingExpect(err).ToNot(HaveOccurred())checks in each validator never verify anything. Removing that parameter will make the table setup a lot clearer.Also applies to: 494-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go` around lines 25 - 28, The table-driven tests define validate as func(*testing.T, *GomegaWithT, *hyperv1.HostedControlPlane, error) but always pass nil for the error; remove the unused err parameter by changing the validate signature to func(*testing.T, *GomegaWithT, *hyperv1.HostedControlPlane) for the testCases in deployment_test.go (and the duplicate occurrence around the 494-500 region), then update all validate call sites in the subtest loop to stop passing nil and remove any Expect(err).ToNot(HaveOccurred()) checks inside the individual validator functions so they only accept (*testing.T, *GomegaWithT, *hyperv1.HostedControlPlane); ensure references to validate, testCases, GomegaWithT, HostedControlPlane, and Expect(err) are updated accordingly to keep compilation passing.
🤖 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/karpenteroperator/secret_test.go`:
- Around line 157-178: The test currently claims to verify the "exact format"
but uses normalizeWhitespace; remove that normalization and assert the raw
credentials string equals the expected string instead: use
g.Expect(credentials).To(Equal(expected)) and keep expectedTemplate/expected
as-is (ensure expected includes the exact final newline and spacing you expect),
so the comparison uses the actual credentials value and will catch
whitespace/formatting regressions; update or remove the normalizeWhitespace
function and adjust the test comment to reflect it now enforces exact match.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go`:
- Around line 362-377: The test must assert the precondition that the base
deployment contains the "audit-logs" container before calling adaptDeployment;
locate the container with findContainer("audit-logs",
deployment.Spec.Template.Spec.Containers) immediately after loading the manifest
and use g.Expect(container).ToNot(BeNil()) to ensure it exists, then call
adaptDeployment(cpContext, deployment) and finally assert the container was
removed with g.Expect(findContainer("audit-logs",
deployment.Spec.Template.Spec.Containers)).To(BeNil()); this uses the existing
symbols deployment, findContainer, adaptDeployment, cpContext and the Gomega
g.Expect assertions.
---
Duplicate comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go`:
- Around line 123-127: The test currently only asserts the replica-override
branch; add an assertion for the "no replica override" path so that when
expectedReplicas == nil the deployment's replica count is unchanged after
calling adaptDeployment. Locate the test that calls
assets.LoadDeploymentManifest(ComponentName) and adaptDeployment(cpContext,
deployment) and add a check comparing deployment.Spec.Replicas (or the pointer
value) to the original value saved before adaptDeployment when expectedReplicas
is nil; ensure you reference the deployment variable, adaptDeployment function,
and expectedReplicas in the assertion.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component_test.go`:
- Around line 32-42: Extract the repeated Karpenter AutoNode literal into a
small test helper (e.g., makeKarpenterAutoNode or newKarpenterAutoNode) that
returns *hyperv1.AutoNode populated with Provisioner:
hyperv1.ProvisionerConfig{Name: hyperv1.ProvisionerKarpenter, Karpenter:
&hyperv1.KarpenterConfig{Platform: hyperv1.AWSPlatform, AWS:
&hyperv1.KarpenterAWSConfig{RoleARN:
"arn:aws:iam::123456789012:role/karpenter"}}}; replace the inline literals in
the component_test.go cases that construct AutoNode (the instances using
AutoNode, Provisioner, KarpenterConfig, KarpenterAWSConfig) with calls to this
helper to remove duplication and reuse across other test cases.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/secret_test.go`:
- Around line 20-24: Remove the redundant expectedRoleARN field from the test
case structs and all references to it in the tests: simplify the testCases
declaration by keeping only name and roleARN (and validateCredentials), update
any table-driven test iterations to assert against roleARN directly (or remove
assertions that compared expectedRoleARN to roleARN), and delete the now-unused
expectedRoleARN variable occurrences; specifically update the testCases variable
and any uses in the test helper logic that referenced expectedRoleARN so tests
still validate credentials using roleARN only.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.go`:
- Around line 137-162: The fakeReleaseImageProvider currently ignores the
componentName argument so tests can't detect wrong lookups; update
fakeReleaseImageProvider to be key-aware by storing a map[string]string (or the
expected key) and have GetImage(componentName string), ImageExist(name string)
and ComponentImages() validate the provided componentName against that map
(returning the matching image when present and an empty string/false when
absent, or fail the test/assert/panic on unexpected keys) so incorrect component
keys cause test failures; ensure references to ComponentName in ComponentImages
still return the correct image from the new map.
- Around line 107-116: The test relies on manifest internals; make it
deterministic by explicitly setting the container and its env vars on the loaded
deployment before calling adaptDeployment and checking results: after loading
deployment via assets.LoadDeploymentManifest(ComponentName) set
deployment.Spec.Template.Spec.Containers to contain at least one core container
(e.g., ensure a container exists and set container.Env entries for the keys the
test expects), then call adaptDeployment(cpContext, deployment); also add a
guard that deployment.Spec.Template.Spec.Containers has length > 0 before
indexing into Containers[0] so failures are clearer (reference: deployment,
adaptDeployment, Containers, Env).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go`:
- Around line 25-28: The table-driven tests define validate as func(*testing.T,
*GomegaWithT, *hyperv1.HostedControlPlane, error) but always pass nil for the
error; remove the unused err parameter by changing the validate signature to
func(*testing.T, *GomegaWithT, *hyperv1.HostedControlPlane) for the testCases in
deployment_test.go (and the duplicate occurrence around the 494-500 region),
then update all validate call sites in the subtest loop to stop passing nil and
remove any Expect(err).ToNot(HaveOccurred()) checks inside the individual
validator functions so they only accept (*testing.T, *GomegaWithT,
*hyperv1.HostedControlPlane); ensure references to validate, testCases,
GomegaWithT, HostedControlPlane, and Expect(err) are updated accordingly to keep
compilation passing.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.go`:
- Around line 3-14: The test currently uses ContainSubstring against the
NO_PROXY env var which can yield false positives; update the assertion that
inspects the env var named "NO_PROXY" (the test that retrieves env :=
findEnvVar(envs, "NO_PROXY") or similar) to assert the full exact value instead
of a substring (e.g., Expect(env.Value).To(Equal(expectedNoProxy)) or use a
strict regex) and construct expectedNoProxy from the exact host list used by the
deployment (join the known hosts into the exact string you expect) so the test
fails when extra/unexpected hosts are present.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go`:
- Around line 3-14: The test currently uses permissive substring matching to
assert the container env var "NO_PROXY"; change the assertion in
deployment_test.go to parse the NO_PROXY value by splitting on commas, trimming
whitespace, and compare the resulting host set exactly against the expected host
set (order-insensitive) instead of using substring/Contains checks so
extra/unexpected entries are detected; update the assertion logic where the test
inspects the container env var named "NO_PROXY" to perform this exact set
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: Pro
Run ID: 8606e8ef-9f91-4570-916b-53af43f3642b
📒 Files selected for processing (22)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/fg/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/fg/job_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/podmonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/secret_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/nto/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/cronjob_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/deployment_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/servicemonitor_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/deployment_test.go
✅ Files skipped from review due to trivial changes (10)
- control-plane-operator/controllers/hostedcontrolplane/v2/fg/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/nto/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/cronjob_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/servicemonitor_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/podmonitor_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/fg/job_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/karpenteroperator/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/olm_operator/servicemonitor_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/collect_profiles/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/component_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/nto/servicemonitor_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalog_operator/deployment_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8215 +/- ##
==========================================
+ Coverage 34.65% 35.61% +0.96%
==========================================
Files 767 767
Lines 93263 93306 +43
==========================================
+ Hits 32318 33231 +913
+ Misses 58266 57388 -878
- Partials 2679 2687 +8
🚀 New features to boost your workflow:
|
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
c832a62 to
ccae78e
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. |
openshift-ci
bot
added
area/api
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
ccae78e to
a422450
Compare
openshift-ci
bot
added
the
area/testing
|
@jparrill: 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. |
Member
Author
|
/verified by ut |
|
@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. |
Add behavior-driven unit tests for KCM, konnectivity, machine approver, and PKI operator v2 controller packages covering config adaptation, deployment adaptation, predicates, and kubeconfig generation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add behavior-driven unit tests for the DNS operator and ignition server proxy v2 controller packages covering deployment adaptation, service configuration, and component predicates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add behavior-driven unit tests for the NTO, OAuth API server, and Karpenter operator v2 controller packages covering deployment adaptation, service monitor configuration, predicates, and secret generation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add behavior-driven unit tests for all OLM v2 controller sub-packages including catalog operator, catalogs, collect profiles, OLM operator, and packageserver covering deployment adaptation, service monitors, predicates, and cron schedule generation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add behavior-driven unit tests for the assets and feature gate v2 controller packages covering manifest loading, deserialization, iteration, and feature gate job adaptation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
improve-ut-coverage
branch
from
3f05aa5 to
5b7f608
Compare
openshift-ci-robot
removed
the
verified
Contributor
|
/lgtm |
Contributor
|
/verified by UnitTests |
openshift-ci-robot
added
the
verified
|
@jparrill: 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. |
Contributor
|
Scheduling tests matching the |
Member
Author
|
/verified by ut |
|
@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. |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
I now have all the evidence needed. Let me compile the complete analysis. The pattern is clear:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 7 Azure-platform hosted cluster tests (TestAutoscaling, TestCreateCluster, TestCreateClusterCustomConfig, TestNodePool/HostedCluster0, TestNodePool/HostedCluster2, TestAzureScheduler, TestUpgradeControlPlane) failed identically during Root CauseThe root cause is an Azure External DNS infrastructure failure in the Failure chain:
Key evidence this is infrastructure, not a code regression:
Recommendations
Evidence
|
Member
Author
|
/test e2e-aks |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Member
Author
|
/test e2e-aws |
Member
Author
|
Previous aws error seems like a flake |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Member
Author
|
/retest This was a completely different error on aws than the last one |
Contributor
|
@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. |
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
5 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.
What this PR does / why we need it:
Adds behavior-driven unit tests for 22 previously untested v2 control-plane-operator controller packages, covering ~9,200 lines of new test code across 51 test files. All packages were at 0% coverage before this PR.
Packages covered
v2/capi_manager,v2/capi_providerv2/cloud_controller_manager/powervs,v2/cloud_credential_operatorv2/kcm,konnectivity,v2/konnectivity_agent,v2/machine_approver,v2/pkioperatorv2/dnsoperator,v2/ignitionserver_proxyv2/nto,v2/oauth_apiserver,v2/karpenteroperatorv2/olm,v2/olm/catalog_operator,v2/olm/catalogs,v2/olm/collect_profiles,v2/olm/olm_operator,v2/olm/packageserverv2/assets,v2/fgTesting approach
Tests follow the behavior-driven-testing skill conventions:
"When <condition>, it should <behavior>"NewWithT(t)per subtestt.Parallel()at both test function and subtest levels (except wheret.Setenvis used)validatefunc fieldsNewComponent) is intentionally excludedWhich issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-3174
Special notes for your reviewer:
All 22 packages were previously at 0% unit test coverage. Tests cover predicates, deployment/config adapters, secret generation, service monitor configuration, and component options — the functions with real conditional logic. Pure boilerplate (
NewComponentwiring) is intentionally excluded.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit