Refactor testutil MakePVC to implement builder pattern

[AndrewSirenko]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change /kind bug

/kind cleanup

/kind design /kind documentation /kind failing-test /kind feature /kind flake

What this PR does / why we need it: This PR refactors test files for external-resizer as @carlory requested in PR 351.

It makes MakePVC simpler by letting developers use use MakePVC().WithXXX().WithYYY() to implement an expected PVC.

Finally, this PR refactors helper functions previously in testutil to use this pattern and moves them to their respective packages as requested.

Which issue(s) this PR fixes:

Partially addresses #367

Special notes for your reviewer:

• Will squash commits before merging.
• ~Should I go refactor ALL unit tests to use this pattern? Or is this sufficient for marking this part of #367 off?~ Yes must refactor all tests.

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @AndrewSirenko. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrewSirenko Once this PR has been reviewed and has the lgtm label, please assign gnufied for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-csi/external-resizer/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[carlory]

/ok-to-test

[carlory]

@AndrewSirenko Please refactor all the test code with the new library if possible. It makes the test code more readable and maintainable.

[AndrewSirenko]

Hey @carlory I refactored most of the unit tests and also added in PVWrapper to testutil for consistency. I will do a final deep comb through for any places I've missed (like a few lines in util_test.go), but would you be able to skim through some of my changes to confirm I'm on the right track?

Also, I've been keeping the convenience functions like makeTestPVC in each package intact. Is this fine or in an ideal world we'd want to get rid of them such that most WithXXX()s happen within the testcase definitions?

[carlory]

Firstly, thanks for working on this task. It's not a blocker for the beta target.

The builder pattern wraps an object inside and returns the inner object after setting the properties via a series of chained methods. The following benefits are:

1. It can return a valid or invalid object. And 🌟🌟🌟 the object may be uncompleted, lacking some required properties but we don't take care of it in our test cases.
2. It is easy to generate the object. so we don't define lots of constructors 🌟🌟 for the object
3. It provides a helper function to return a wrapper from an existing object. So we can generate a new object with a few properties changed from an existing object.

We can see lots of examples in k8s.io/kubernetes. i.e.

• https://github.com/kubernetes/kubernetes/blob/5bf1e95541d90e37f6c6637b5b45d8783e7907aa/pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go#L103-L104
• https://github.com/kubernetes/kubernetes/blob/5bf1e95541d90e37f6c6637b5b45d8783e7907aa/pkg/scheduler/framework/plugins/nodename/node_name_test.go#L41

So I added a suggestion to the previous PR and hope to reduce the code duplication in the pkg/modifycontroller and pkg/resizer directories if possible.

Finally, after I deeply read the code and tried to add some changes, it may increase the complexity and the benefits are not obvious. So we can ignore this suggestion. What do you think? @AndrewSirenko

cc @msau42

[AndrewSirenko]

Closing PR, discussed that refactoring to builder pattern may be desirable once all sidecars are in one.