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.

[Kartik494]

Hi @msau42 Thanks for the response, i will be changing this as suggested and raising a PR Accordingly

[mauriciopoppe]

@Kartik494 I'd frame the problem like this, we have a list of test cases t = [t1, t2, t3, ...] which have to be tested (as tc) by

external-provisioner/pkg/controller/controller_test.go

Line 2193 in a9b470d

func runProvisionTest(t *testing.T, tc provisioningTestcase, requestedBytes int64, provisionDriverName, supportsMigrationFromInTreePluginName string, testProvision bool) {

It looks like runProvisionTest doesn't have all the information of a test case when it's called and it augments info to it in these statements

external-provisioner/pkg/controller/controller_test.go

Lines 2209 to 2236 in a9b470d

	if tc.notNilSelector {
	tc.volOpts.PVC.Spec.Selector = &metav1.LabelSelector{}
	} else if tc.makeVolumeNameErr {
	tc.volOpts.PVC.ObjectMeta.UID = ""
	} else if tc.getSecretRefErr {
	tc.volOpts.StorageClass.Parameters[provisionerSecretNameKey] = ""
	} else if tc.getCredentialsErr {
	tc.volOpts.StorageClass.Parameters[provisionerSecretNameKey] = "secretx"
	tc.volOpts.StorageClass.Parameters[provisionerSecretNamespaceKey] = "default"
	} else if !testProvision {
	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, tc.createVolumeError).Times(0)
	} else if tc.volWithLessCap {
	out.Volume.CapacityBytes = int64(80)
	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, tc.createVolumeError).Times(1)
	controllerServer.EXPECT().DeleteVolume(gomock.Any(), gomock.Any()).Return(&csi.DeleteVolumeResponse{}, tc.createVolumeError).Times(1)
	} else if tc.volWithZeroCap {
	out.Volume.CapacityBytes = int64(0)
	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
	} else if tc.expectCreateVolDo != nil {
	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, req *csi.CreateVolumeRequest) {
	tc.expectCreateVolDo(t, ctx, req)
	}).Return(out, tc.createVolumeError).Times(1)
	} else {
	// Setup regular mock call expectations.
	if !tc.expectErr && !tc.skipCreateVolume {
	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, tc.createVolumeError).Times(1)
	}
	}

Ideally when runProvisionTest is called tc should already be defined and not modified, an idea that @msau42 suggests is to move these if statements and add it to the testcases themselves e.g. to t1, t2, ..., adding them back would take a lot of effort too because there are a lot of testcases to modify, something on top of that is that there's a lot of boilerplate code (common code) that all of these test cases share, this is a good place where we can abstract the definition of a test case to simplify it.

Imagine that the creation of a single test case is abstracted by a function:

// TestCaseBuilder returns testCase Object
func TestCaseBuilder() provisioningTestcase {
   tc := provisioningTestcase{ ... }
   return tc
}

// usage
tc := TestCaseBuilder()
// tc is not very well defined at this point, we might need to add some additional properties
if tc.foo { 
	tc.bar = "dummy"
}

if tc.hello { 
	tc.world = "dummy"
}

Instead of adding if statements outside like in the example we accept functions that will encapsulate that behavior for us:

type ProvisioningTestCaseOption func(*provisioningTestcase) *provisioningTestcase

// TestCaseBuilder returns testCase Object
func TestCaseBuilder(...options ProvisioningTestCaseOption) provisioningTestcase {
   // build object as before
   tc := &provisioningTestcase{ ... }
   for option := range options {
      tc = option(tc)
   }
   return tc
}

func WithBar() ProvisioningTestCaseOption {
   return func(tc *provisioningTestCase) *provisioningTestCase {
       if tc.foo {
          tc.bar = "dummy"
       }
       return tc
   }
}

func WithWorld() ProvisioningTestCaseOption {
   return func(tc *provisioningTestCase) *provisioningTestCase {
       if tc.hello {
          tc.world = "dummy"
       }
       return tc
   }
}

// usage
tc := TestCaseBuilder(WithBar(), WithWorld())
// tc is very well defined at this point

That's one of doing it, I learned this from https://github.com/grpc/grpc-go/blob/62adda2ece5ec803c824c5009b83cea86de5030d/examples/helloworld/greeter_client/main.go#L39

you can use the builder pattern too

[Kartik494]

@mauriciopoppe Thanks for the response, if i understood it correctly here you are asking for making a test case Builder for provisiongTestCase struct which would implement struct object as a functions ?

Thanks

[Kartik494]

For example here you mentioned a function above so here the function withBar() should be a object created from struct objects

func WithBar() ProvisioningTestCaseOption {
return func(tc *provisioningTestCase) *provisioningTestCase {
if tc.foo {
tc.bar = "dummy"
}
return tc
}
}

[mauriciopoppe]

yes we could have a builder for the provisioningTestcase struct, the objective is for runProvisionTests() not to modify its arguments anymore therefore provisionTestcases() should build the objects with all the information that runProvisionTests() needs

[hshitomi]

@msau42 @mauriciopoppe I am interested in working on this issue and would like to take it over. If there is no problem, could you please assign me to this issue? Thank you in advance.

[mauriciopoppe]

@hshitomi you can create a minimal PR where you show what the proposed change is going to look like in the description, you can follow #376 (comment)

[hshitomi]

@mauriciopoppe Thank you for your quick reply. Also, thank you for sharing the directions. That was very helpful. Yes, I plan to create a draft PR to start our conversation and confirm that I am on the right track, hopefully, tonight.

I am currently focusing on the provisioning test cases. I removed the if statements from runProvisionTest() using the method chaining approach instead of the functional option pattern because it seems a bit simpler to me.

Is it okay for me to open a draft PR on this issue while I am not assigned to this issue?

[hshitomi]

/assign