Refactor controller_tests to use builder pattern

[msau42]

Provision and snapshot tests have a lot of logic built into the test case itself to construct input objects to the functions. For examples:

external-provisioner/pkg/controller/controller_test.go

Line 1487 in 16a2da3

if tc.notNilSelector {

external-provisioner/pkg/controller/controller_test.go

Line 2020 in 16a2da3

if tc.misBoundSnapshotContentUID {

I think it would be cleaner to refactor these tests to use a builder pattern and construct the input object when defining the test cases.

[msau42]

/help
/kind cleanup

[k8s-ci-robot]

@msau42:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/kind cleanup

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.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[msau42]

/lifecycle frozen

[Kartik494]

/assign

[Kartik494]

Hi @msau42 could you let me know, here what we mean when we are talking on refactor provisioner and snapshot test case to use builder pattern ?

[msau42]

Hi @Kartik494, in some of the initial examples I referenced, the test case has a lot of conditional statements about different scenarios, and are adjusting the test inputs based off of that.

I think it makes the test hard to read and understand because you need to run through all these conditionals to figure out what the input object looks like. Instead, I think it would be better if the test took the actual object as an input. Here is an example:

external-provisioner/pkg/controller/topology_test.go

Line 40 in 16a2da3

func TestGenerateVolumeNodeAffinity(t *testing.T) {

You can see that the test case itself is very short and and simple compared to the original examples, and it's very clear what objects are being tested.

I suggested the builder pattern as a way to reduce duplicate lines for the input object, but it doesn't have to be done this way. I think what I had in mind was a method that produces a basic input object, and then chain that with methods that customize different fields for the test case. Here's a nice post I found explaining a couple of options: https://www.calhoun.io/using-functional-options-instead-of-method-chaining-in-go/

I couldn't find an example of a unit test that does this, but you can see some libraries that provide "WithX()" methods that modify the input object: https://github.com/kubernetes-csi/csi-lib-utils/blob/master/leaderelection/leader_election.go#L124

Hope this is helpful, let me know if you have more questions!

[Kartik494]

Hi @msau42 So for Every Conditional statement here

external-provisioner/pkg/controller/controller_test.go

Line 1487 in 16a2da3

if tc.notNilSelector {

Should i provide Methods Like this

external-provisioner/pkg/controller/topology_test.go

Line 40 in 16a2da3

func TestGenerateVolumeNodeAffinity(t *testing.T) {

and when i study the article (https://www.calhoun.io/using-functional-options-instead-of-method-chaining-in-go/) i get to know that Like we have Pattern builder as part of creational design pattern in java, we should implement functional options here. Is this the Ideal approach. Please Suggest

[msau42]

Hi @Kartik494, so I think the final end goal should be to remove the conditionals in the testcase that adjust the input object. Using a build/chaining api to construct the input is optional. Maybe for now to keep it simple, start with passing the entire object as input, and we can see if adding some constructors would be useful or not.