Create test to cover path of azureMachinePool.Spec.UserAssignedIdenti… #4278
Conversation
k8s-ci-robot
added
release-note-none
| @@ -194,3 +194,7 @@ func (r *AzureJSONMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl | |||
|
|
|||
| return ctrl.Result{}, nil | |||
| } | |||
|
|
|||
| var getClient = func(auth azure.Authorizer) (identities.Client, error) { | |||
There was a problem hiding this comment.
This "patch" allows for this to be overridden during testing but not in production.
| @@ -18,17 +18,24 @@ package controllers | |||
|
|
|||
| import ( | |||
| "context" | |||
| "os" | |||
There was a problem hiding this comment.
Removed to use t.Setenv
troy0820
force-pushed
the
troy0820/add-unit-tests-to-azurejsonMachinePoolReconciler
branch
from
ce82395 to
20614cc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4278 +/- ##
==========================================
+ Coverage 58.15% 59.74% +1.58%
==========================================
Files 187 192 +5
Lines 19351 19303 -48
==========================================
+ Hits 11254 11533 +279
+ Misses 7459 7139 -320
+ Partials 638 631 -7 ☔ View full report in Codecov by Sentry. |
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
/cc @willie-yao |
| }, | ||
| } | ||
|
|
||
| azureCluster := &infrav1.AzureCluster{ |
There was a problem hiding this comment.
I think you can use this getFakeAzureCluster function that exists here: https://github.com/willie-yao/cluster-api-provider-azure/blob/aks-fleet/controllers/azuremachine_controller_test.go#L529
However, I think there could be a case to be made to create a new file that contains all of these functions for tests, since it's a bit confusing they exist in specific controller tests but can be used across the package. I think this could be done in a follow-up PR since it'll be a bit out of scope for this issue.
There was a problem hiding this comment.
I would definitely do this but to migrate from this to that may be additional effort that may pollute the addition of this test.
Also, setting up fixtures to settle all cases where that may be useful may also pollute the repo where that may need to be tested to ensure code coverage (I know that metric isn't bound to quality, but I digress).
I can create an issue and follow up to call that out as something we would like to have.
There was a problem hiding this comment.
Sounds good! I think it would be good to leave as-is for the current PR
…ties Signed-off-by: Troy Connor <[email protected]>
troy0820
force-pushed
the
troy0820/add-unit-tests-to-azurejsonMachinePoolReconciler
branch
from
20614cc to
5d388e7
Compare
|
/retest |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: fd3b45d604ba984db4527e0f77d32e9a6a0768b8
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adds testing to the
controllerspackageWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes ##4277
Partially fixes: #3649
Special notes for your reviewer:
I had to change the return value to an interface (which the return type implements) to allow the patch to ensure mocking can take place. The mocking will use the generated mocks and the EXPECT() to allow to hit that code path while testing.
TODOs:
Release note: