CNTRLPLANE-3307: add unit tests for Azure Private Link Service controllers#8285
Conversation
|
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-3307 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 "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughThis pull request expands and hardens unit tests for the Azure Private Link Service controller and observer. Mock Azure SDK clients were updated to return configurable Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/azureprivatelinkservice/observer_test.go (1)
435-460: Consider addingt.Parallel()for consistency.Other tests in this file (e.g.,
TestControllerName,TestBaseDomainFromServices) uset.Parallel()at the top of the test function. This test could also benefit from parallel execution.Suggested fix
func TestReconcile_WhenServiceNotFound_ItShouldReturnNoError(t *testing.T) { + t.Parallel() g := NewGomegaWithT(t)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/azureprivatelinkservice/observer_test.go` around lines 435 - 460, Add t.Parallel() to the top of the TestReconcile_WhenServiceNotFound_ItShouldReturnNoError test to enable parallel execution consistent with other tests; edit the TestReconcile_WhenServiceNotFound_ItShouldReturnNoError function (which constructs AzurePrivateLinkServiceObserver and calls observer.Reconcile) and insert a call to t.Parallel() immediately after the function starts to allow this test to run concurrently with others.
🤖 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/azureprivatelinkservice/observer_test.go`:
- Around line 435-460: Add t.Parallel() to the top of the
TestReconcile_WhenServiceNotFound_ItShouldReturnNoError test to enable parallel
execution consistent with other tests; edit the
TestReconcile_WhenServiceNotFound_ItShouldReturnNoError function (which
constructs AzurePrivateLinkServiceObserver and calls observer.Reconcile) and
insert a call to t.Parallel() immediately after the function starts to allow
this test to run concurrently with others.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 176674bb-97d4-45f7-bb26-d903e712d5f8
📒 Files selected for processing (2)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.gocontrol-plane-operator/controllers/azureprivatelinkservice/observer_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8285 +/- ##
==========================================
+ Coverage 36.46% 36.71% +0.24%
==========================================
Files 765 765
Lines 93256 93256
==========================================
+ Hits 34010 34238 +228
+ Misses 56532 56338 -194
+ Partials 2714 2680 -34 see 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
bryan-cox
force-pushed
the
CNTRLPLANE-3307
branch
from
f70903b to
5e9a322
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.go (2)
409-415: Consider extracting a shared reconciler/test fixture builder.This setup pattern repeats heavily across the file, which makes future updates noisy. A small fixture helper (scheme/client/mocks/reconciler) would reduce duplication and improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 409 - 415, Extract a reusable test fixture builder to reduce duplication: create a helper function (e.g., newTestReconciler or buildReconcilerFixture) that constructs and returns an *AzurePrivateLinkServiceReconciler pre-populated with the common pieces (scheme, fakeClient, and the mock implementations currently used such as mockPrivateEndpoints, mockPrivateDNSZones, mockVirtualNetworkLinks, mockRecordSets). Replace repeated inline construction sites with calls to this helper, allowing callers to override or pass optional mock overrides (like getErr on mockPrivateEndpoints) so individual tests can customize behavior without duplicating the entire setup.
1848-1852: Assert mapped request identities, not only count.
TestMapHCPToAzurePLScurrently validates only length, so incorrect names/namespaces can still pass. Assert exacttypes.NamespacedNamevalues for stronger coverage.✅ Suggested assertion hardening
- if tt.expectedLen == 0 { - g.Expect(requests).To(BeEmpty()) - } else { - g.Expect(requests).To(HaveLen(tt.expectedLen)) - } + if tt.expectedLen == 0 { + g.Expect(requests).To(BeEmpty()) + } else { + g.Expect(requests).To(HaveLen(tt.expectedLen)) + got := map[types.NamespacedName]struct{}{} + for _, req := range requests { + got[req.NamespacedName] = struct{}{} + } + g.Expect(got).To(HaveKey(types.NamespacedName{Name: "pls-1", Namespace: "test-ns"})) + g.Expect(got).To(HaveKey(types.NamespacedName{Name: "pls-2", Namespace: "test-ns"})) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 1848 - 1852, TestMapHCPToAzurePLS currently only checks request count; change the test to assert the exact mapped identities (types.NamespacedName) instead of just length. For each table entry in TestMapHCPToAzurePLS, add an expected slice of namespaced names (e.g., expectedRequests) and replace the g.Expect(requests).To(HaveLen(...))/BeEmpty() checks with an equality or unordered membership assertion that compares requests to the expected slice (use the existing requests variable and compare to []types.NamespacedName built from your test case data), ensuring incorrect names/namespaces will fail the test. Ensure you reference the same types.NamespacedName format used by the MapHCPToAzurePLS mapping logic when constructing expected values.
🤖 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/azureprivatelinkservice/controller_test.go`:
- Around line 2271-2273: Update the inaccurate inline comment above the
AzurePrivateLinkServiceReconciler test: it currently states the first delete is
for hypershift.local "api" but the test actually uses "oauth-openshift" and is
checking base-domain deletion behavior. Edit the comment near the
AzurePrivateLinkServiceReconciler instantiation to mention "oauth-openshift" as
the first delete target and that the test validates base-domain deletion
semantics rather than an "api" hypershift.local record.
- Around line 585-588: Update the misleading comment about reconcileDelete in
controller_test.go: change or remove the phrase “reconcileDelete is best-effort,
always returns nil” and instead state that reconcileDelete normally returns nil
for successful/expected delete flows but tests may assert non-nil errors for
simulated non-404 delete failures; reference the reconcileDelete call in the
test (r.reconcileDelete(t.Context(), azPLS, testr.New(t))) and adjust wording to
reflect that some tests deliberately assert non-nil errors for failure
scenarios.
---
Nitpick comments:
In
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 409-415: Extract a reusable test fixture builder to reduce
duplication: create a helper function (e.g., newTestReconciler or
buildReconcilerFixture) that constructs and returns an
*AzurePrivateLinkServiceReconciler pre-populated with the common pieces (scheme,
fakeClient, and the mock implementations currently used such as
mockPrivateEndpoints, mockPrivateDNSZones, mockVirtualNetworkLinks,
mockRecordSets). Replace repeated inline construction sites with calls to this
helper, allowing callers to override or pass optional mock overrides (like
getErr on mockPrivateEndpoints) so individual tests can customize behavior
without duplicating the entire setup.
- Around line 1848-1852: TestMapHCPToAzurePLS currently only checks request
count; change the test to assert the exact mapped identities
(types.NamespacedName) instead of just length. For each table entry in
TestMapHCPToAzurePLS, add an expected slice of namespaced names (e.g.,
expectedRequests) and replace the g.Expect(requests).To(HaveLen(...))/BeEmpty()
checks with an equality or unordered membership assertion that compares requests
to the expected slice (use the existing requests variable and compare to
[]types.NamespacedName built from your test case data), ensuring incorrect
names/namespaces will fail the test. Ensure you reference the same
types.NamespacedName format used by the MapHCPToAzurePLS mapping logic when
constructing expected values.
🪄 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 Plus
Run ID: 567e9139-ccb0-4cf5-a965-55693d2d7536
📒 Files selected for processing (2)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.gocontrol-plane-operator/controllers/azureprivatelinkservice/observer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/azureprivatelinkservice/observer_test.go
bryan-cox
force-pushed
the
CNTRLPLANE-3307
branch
2 times, most recently
from
37706f0 to
db09aef
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/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 security |
openshift-ci
Bot
added
the
needs-rebase
bryan-cox
force-pushed
the
CNTRLPLANE-3307
branch
from
db09aef to
13f8a76
Compare
openshift-ci-robot
removed
the
verified
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/hold Revision 0790fbb was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
bryan-cox
force-pushed
the
CNTRLPLANE-3307
branch
from
0790fbb to
31d9fef
Compare
openshift-ci-robot
removed
the
verified
Add comprehensive behavior-driven unit tests for the observer and controller in the azureprivatelinkservice package, increasing coverage from 68% to 92.7%. Signed-off-by: Bryan Cox <[email protected]> Commit-Message-Assisted-by: Claude (via Claude Code)
bryan-cox
force-pushed
the
CNTRLPLANE-3307
branch
from
31d9fef to
67f3e17
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
|
/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 |
|
I now have the complete root cause analysis. Here is the report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe Three workflow files were modified on
The PR branch was created on 2026-04-20 from base commit Recommendations
Evidence
|
|
/override "ci/prow/verify-workflows" This is just UT in this PR |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/verify-workflows 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 kubernetes-sigs/prow repository. |
|
@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 comprehensive behavior-driven unit tests for the Azure Private Link Service observer and controller in
control-plane-operator/controllers/azureprivatelinkservice/, increasing code coverage from 68% to 92.7%.Tests cover:
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3307
Special notes for your reviewer:
SetupWithManagerfunctions (0% coverage) require a real controller manager and are excluded from unit testingWhen... it should...) and use gomega assertionsPollingHandlerimplementationsChecklist:
Summary by CodeRabbit