Refactor (controller_tests): build the test case objects with all the information #1125
Conversation
Remove the conditional statements that customize the input test case object based on the different test scenarios in the function of running a test. The function to build the test cases should build the test case objects with all the information that the function of running a test needs so that it makes the test code easier to read and understand. Signed-off-by: Hidehisa Shitomi <[email protected]>
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @hshitomi! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @hshitomi. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hshitomi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@mauriciopoppe Let me start our conversation before I completely refactor the code. My current focus is on the provisioning test cases. I have removed the if statements from Since I rewrote the test cases using method chaining (in For now, I am using the method chaining rather than the functional option pattern because it seems a bit simpler to me. I know that the method to build the test case objects with all the information does not matter much. But if you prefer to use the functional option pattern, please let me know. Thank you in advance for your time and guidance. Looking forward to your feedback. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/controller/controller_test.go
Outdated
| @@ -1090,8 +1198,8 @@ func provisionTestcases() (int64, map[string]provisioningTestcase) { | |||
| } | |||
| vacName := "test-vac" | |||
| return requestedBytes, map[string]provisioningTestcase{ | |||
| "normal provision": { | |||
| volOpts: controller.ProvisionOptions{ | |||
| "normal provision": *provisionTestcaseBuilder().volOpts( | |||
There was a problem hiding this comment.
I think the builder pattern should be applied to things we want to build in an object to then test it externally. With this implementation both the things to build and the expectations are part of the same object.
Maybe provisioningTestCase shouldn't have setters for expectations.
There was a problem hiding this comment.
Thank you for your feedback. Let me confirm your suggestion.
We should create another function to set the expectations. The function is to set the following elements of the provisioningTestcase struct, which is the expectation of the test result.
expectedPVSpec *pvSpec
expectedMockReturn func(t *testing.T, out *csi.CreateVolumeResponse, controllerServer *driver.MockControllerServer, createVolumeError error)
expectErr bool
expectState controller.ProvisioningState
expectSelectedNode string // a specific selected-node of the PVC in the apiserver after the test, same as before if empty
expectNoProvision bool // if true, then ShouldProvision should return false
At the same time, the provisionTestcase() function should be used to build the test case, which means to set the following elements of the provisioningTestcase struct.
capacity int64 // if zero, default capacity, otherwise available bytes
volOpts controller.ProvisionOptions
clientSetObjects []runtime.Object
createVolumeError error
withExtraMetadata bool
deploymentNode string // fake distributed provisioning with this node as host
immediateBinding bool // enable immediate binding support for distributed provisioning
controllerPublishReadOnly bool
featureGates map[featuregate.Feature]bool
pluginCapabilities func() (rpc.PluginCapabilitySet, rpc.ControllerCapabilitySet)
Did I understand your suggestion properly?
There was a problem hiding this comment.
I split the first three test cases into builder (buildProvisionTestcase()) and expectation setter (setProvisionTestcaseExpectation()). Is this what you intended?
Or is the intention to further divide the provisioningTestcase struct into two, as shown below, and rewrite related parts based on that?
type provisioningTestcase struct {
capacity int64 // if zero, default capacity, otherwise available bytes
volOpts controller.ProvisionOptions
clientSetObjects []runtime.Object
createVolumeError error
withExtraMetadata bool
deploymentNode string // fake distributed provisioning with this node as host
immediateBinding bool // enable immediate binding support for distributed provisioning
controllerPublishReadOnly bool
featureGates map[featuregate.Feature]bool
pluginCapabilities func() (rpc.PluginCapabilitySet, rpc.ControllerCapabilitySet)
}
type provisioningTestcaseExpectation struct {
expectedPVSpec *pvSpec
expectedMockReturn func(t *testing.T, out *csi.CreateVolumeResponse, controllerServer *driver.MockControllerServer, createVolumeError error)
expectErr bool
expectState controller.ProvisioningState
expectSelectedNode string // a specific selected-node of the PVC in the apiserver after the test, same as before if empty
expectNoProvision bool // if true, then ShouldProvision should return false
}
pkg/controller/controller_test.go
Outdated
| @@ -1090,8 +1198,8 @@ func provisionTestcases() (int64, map[string]provisioningTestcase) { | |||
| } | |||
| vacName := "test-vac" | |||
| return requestedBytes, map[string]provisioningTestcase{ | |||
| "normal provision": { | |||
| volOpts: controller.ProvisionOptions{ | |||
| "normal provision": *provisionTestcaseBuilder().volOpts( | |||
There was a problem hiding this comment.
I think it became a little bit harder to read with chaining, compare:
makeProvisionTestcaseBuilder().volOpts(
...
).foo(
...
).bar(
...
).build()
vs
makeProvisionTestcaseBuilder(
withFoo(),
withBar(),
withVolOpts()
).build()
There was a problem hiding this comment.
OK. Let me implement it using the functional option pattern.
There was a problem hiding this comment.
I wrote the first three test cases with the functional option pattern.
As you said, the test case setup part in buildProvisionTestcase() has become a bit easier to read (we do not need the last build() method in case of the functional option pattern). On the other hand, the functions for the functional option pattern seem to be a bit harder to read than the methods for method chaining.
func (tco *provisioningTestcaseOption) volOpts(op controller.ProvisionOptions) *provisioningTestcaseOption {
tco.tc.volOpts = op
return tco
}
vs.
func withVolOpts(op controller.ProvisionOptions) provisionTestcaseOption {
return func(tc *provisioningTestcase) *provisioningTestcase {
tc.volOpts = op
return tc
}
}
That is why I chose the method chaining. However, I guess the simplicity of the test case setup part might be more important. So, I should use the functional option pattern for the rest of the test cases.
What do you think?
- Change to use the functional option patter for the first 3 test cases - Split the first 3 test cases into builder and expectation setter Signed-off-by: Hidehisa Shitomi <[email protected]>
|
@mauriciopoppe @msau42 I'm sure you're busy, but I'm looking forward to your feedback. If possible, could you please let me know when you can get to the review? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/stale
k8s-ci-robot
added
the
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Remove the conditional statements that customize the input test case object based on the different test scenarios in the function of running a test.
The function to build the test cases should build the test case objects with all the information that the function of running a test needs so that it makes the test code easier to read and understand.
Which issue(s) this PR fixes:
Fixes #376
Special notes for your reviewer:
Does this PR introduce a user-facing change?: