Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create unit tests for azuremanagedcontrolplane_reconciler.go #4290

Closed
1 task
troy0820 opened this issue Nov 17, 2023 · 4 comments · Fixed by #4303
Closed
1 task

Create unit tests for azuremanagedcontrolplane_reconciler.go #4290

troy0820 opened this issue Nov 17, 2023 · 4 comments · Fixed by #4303
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@troy0820
Copy link
Member

/kind cleanup

  • Add unit tests to azuremanagedcontrolplane_reconciler.go

This issue is part of a larger issue #3649 to help with coverage and ensure confidence

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 17, 2023
@troy0820
Copy link
Member Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Nov 17, 2023
@troy0820
Copy link
Member Author

/assign @troy0820

@troy0820
Copy link
Member Author

troy0820 commented Nov 17, 2023

This is going to look interesting for the following reasons.

  1. reconcileNormal goes through the code path where it creates a lot of "scopes".
    a. These scopes create a lot of "clients" which are not necessarily mocked from my understanding.
    b. These clients need an "authorizer" which is also not mocked from my understanding.
  2. Edited: Because we also use PatchHelper (which is not exported in azure/scope/managedcontrolplane.go) exporting this to set this on the ManagedControlPlaneScope type allows us to inject the fake client into this type

Plan:

  1. Return interface to allow the value from New to return an interface and not the concrete type (this will allow us to inject the patch that's coming)
  2. Define interface with the "client" to do what the concrete type is doing so it will satisfy the interface that is now being returned instead of the concrete type.
  3. Mock e.g armnetwork.SubnetsClient and return the "AzureClient" so that the mock can be injected when creating the AzureClient in any call.
  4. Don't export the "struct" but the interface to allow the concrete type that is doing the "work" to only do what the interface allows. (hides implementation)
  5. Mock the newly created interface to allow this to be able to be tested.
  6. Patch the "New" function to allow the injection of the mock interface when the "caller" needs to do this (see PR Create test to cover path of azureMachinePool.Spec.UserAssignedIdenti… #4278 to see this being done on that unit test)
  7. Export New<client> as a exported variable equaling the function necessary to be able to be overridden in testing
  8. In test call New<nameofclient>Client (which will equal privatendpoints.New(auth azure.Authorizer) in prod but patched for testing to return the mocked interface we now have)
  9. Patch call below to allow that to return the necessary services I need and I don't have to go that far down in the caller stack to get the values I need.

So overall this PR may be larger than just the test file and the adjustments to allow this to be testable.

type SubnetsClient interface {
      Get(ctx context.Context, resourceGroupName string, virtualNetworkName string, subnetName string, options *armnetwork.SubnetsClientGetOptions) (armnetwork.SubnetsClientGetResponse, error)
      BeginCreateOrUpdate(ctx context.Context, resourceGroupName string, virtualNetworkName string, subnetName string, subnetParameters armnetwork.Subnet, options *armnetwork.SubnetsClientBeginCreateOrUpdateOptions) (*runtime.Poller[arm      network.SubnetsClientCreateOrUpdateResponse], error)
      BeginDelete(ctx context.Context, resourceGroupName string, virtualNetworkName string, subnetName string, options *armnetwork.SubnetsClientBeginDeleteOptions) (*runtime.Poller[armnetwork.SubnetsClientDeleteResponse], error)
    }

type AzureClient struct {
      subnets SubnetsClient 
   }
// edited 
// still do the same New function to get an AzureClient

Will update comment with workaround.

Possible workaround

//patch this method
 newAzureManagedControlPlaneReconciler(scope *scope.ManagedControlPlaneScope) (*azureManagedControlPlaneService, error) 
/* Patching this to return the svc that I need: 
1. PrivateEndpoints (which the underlying type will be mocked)
2. ResourceHealth (which the underlying type will be mocked) 
3. ....
*/
// to allow the return value in testing to coincide with the mock values

Edited: Because the mocks exists, we can perform the testing without having to do this, however the next comment expresses why this is still a good idea.

@troy0820
Copy link
Member Author

Mocks exists for azureManagedControlPlane.services and can use those to inject the necessary bits to test reconcileNormal
We do not need to create mocks for the clients.

(However this is not a bad idea as to get more granular with testing and push the abstraction to the clients who need to do the calls and not the mocks from the interfaces wrapped in values only known to the caller at runtime)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants