Increase unit test coverage for AWSCluster controller

[Ankitasw]

What type of PR is this?
/area testing

What this PR does / why we need it:
This PR increases the unit tests coverage for AWSCluster controller. All the scenarios covered are listed here.
Coverage of controllers pkg increased from 42.5% to 60%.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #2910

Special notes for your reviewer:
Plan to submit a separate PR per each controller.

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

Release note:

Add unit tests for AWSCluster controller

[Ankitasw]

/area testing
/triage accepted
/priority important-soon

[Ankitasw]

/test ?

[k8s-ci-robot]

@Ankitasw: The following commands are available to trigger required jobs:

• /test pull-cluster-api-provider-aws-build
• /test pull-cluster-api-provider-aws-test
• /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

• /test pull-cluster-api-provider-aws-e2e
• /test pull-cluster-api-provider-aws-e2e-blocking
• /test pull-cluster-api-provider-aws-e2e-conformance
• /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
• /test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

• pull-cluster-api-provider-aws-build
• pull-cluster-api-provider-aws-test
• pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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/test-infra repository.

[Ankitasw]

/retest

[pydctw]

The reason I have used fakeClient is to reduce the number of mocking calls as you have seen in case of integration tests, and as we have discussed here, wherever we can use testenv we are using it. Since we want to test only a function and not subfunctions, we are using fakeClient in some places so that we don't end up adding expected mock calls for each AWS API.

I am confused by this statement. I've used both fakeClient and envTest before and know that there are pros and cons of using one vs. another. See https://cluster-api.sigs.k8s.io/developer/testing.html?highlight=envtest#envtest.

However, I don't think the choice affects the number of mocking calls. Could you help me understand what mocking calls were avoided by using a fakeClient in TestAWSClusterReconcileOperations?

[sedefsavas]

For integration tests, we should define couple of realistic scenarios. And checking conditions to see if they are as expected is a good signal.

The scenarios I am thinking are:
For happy path:
1- Testing successful AWSCluster creation
2- Testing successful AWSCluster deletion

For unhappy path:
1- Testing failure during AWSCluster creation (this could be one of the tests already included in this PR.)
2- Testing failure during AWSCluster deletion

@pydctw please add further suggestion on the unhappy path testing.

[Ankitasw]

@sedefsavas For happy path, we already have delete scenario in PR, and the unmanaged VPC test case covers happy path just that it wont reconcile VPC, rest all flows it goes through. Do you suggest to add separate case to PR to test happy path fully?

[Ankitasw]

However, I don't think the choice affects the number of mocking calls. Could you help me understand what mocking calls were avoided by using a fakeClient in TestAWSClusterReconcileOperations?

@pydctw If you check mocking in TestAWSClusterReconcileOperations tests, you would see I have mocked whole subfunctions like ReconcileBastion, ReconcileLoadbalancers. Wherever we want the flow to go through, we wont mock that service, and mock other services for which the flow is not to be tested, this saves lots of AWS API mocking calls as we have seen in integ tests. Since this is unit test, we don't want to cover each and every flow for subfunctions, hence using fakeClient.

[pydctw]

@Ankitasw, first of all, thanks for all the hard work and great discussion.

I had an impression that envTest is a preferred way and CAPI is moving away from fakeClient but after more discussion with some folks (@fabriziopandini, @yastij, @sedefsavas), the recommendation is to pick a right approach depending on test cases. You probably know but to summarize the pros and cons of two approaches here -

fakeClient

• Fast and easier to write
• Cannot test resourceVersion, webhooks and etc

envTest

• Closer to the real env with a local kube-apiserver and etcd
• Webhooks and controllers can be registered, hence more realistic uses cases can be tested.
• Cache sync delays need to be accounted for, meaning created/deleted resources can take time to be updated in cache. We've observed tests being flaky in the past and it is why many tests based on envTest uses g.Eventually for assertion and help functions like CreateAndWait or CleanupAndWait
• Slower

What I was trying to ask here was the whys of choosing fakeClient vs. envTest for different unit tests (not across unit vs. integration tests).

As long as we understand tradeoffs of using fakeClient vs. envTest and picks an appropriate one, I am ok with the choices. And if we change our mind, we can always iterate and refactor later 😄

[pydctw]

As discussed, let's split the integration test out of this PR so that we can merge unit tests first.

[Ankitasw]

What I was trying to ask #3073 (comment) was the whys of choosing fakeClient vs. envTest for different unit tests (not across unit vs. integration tests).

@pydctw I hope I answered your question correctly here 😄

As long as we understand tradeoffs of using fakeClient vs. envTest and picks an appropriate one, I am ok with the choices. And if we change our mind, we can always iterate and refactor later 😄

Thanks @pydctw I hope the current tests are ok for now and I agree we can always refactor it later based on what is good for our tests and how good is the maintainability of mocks

As discussed, let's split the integration test out of this PR so that we can merge unit tests first.

I will do that

[Ankitasw]

The scope of the PR is now changed, added separate PR for integration tests

[Madhur97]

/lgtm

[k8s-ci-robot]

@Madhur97: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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/test-infra repository.

[shivi28]

/lgtm

[sedefsavas]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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:

• OWNERS [sedefsavas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment