Remove duplicated code in e2e tests #155
Conversation
|
Hi @k-keiichi-rh. Thanks for your PR. I'm waiting for a medik8s 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-sigs/prow repository. |
test/e2e/far_e2e_test.go
Outdated
| @@ -163,8 +126,18 @@ var _ = Describe("FAR E2e", func() { | |||
| remediationTimes = append(remediationTimes, time.Since(startTime)) | |||
| }) | |||
| }) | |||
| } | |||
|
|
|||
| Context("stress cluster with OutOfServiceTaint remediation strategy", func() { | |||
There was a problem hiding this comment.
The context doesn't match the RemediationStrategy.
| Context("stress cluster with OutOfServiceTaint remediation strategy", func() { | |
| Context("stress cluster with ResourceDeletion remediation strategy", func() { |
There was a problem hiding this comment.
Sorry. Fixed it.
|
/ok-to-test |
| }) | ||
| }) | ||
| Context("stress cluster with OutOfServiceTaint remediation strategy", func() { | ||
| runFARTests := func(remediationStrategy v1alpha1.RemediationStrategyType, skipCondition func() bool) { |
There was a problem hiding this comment.
Maybe I'm missing something, but can we just use a normal function here?
func runFARTest(...) bool {
...
}
There was a problem hiding this comment.
I've seen and used this pattern especially in tests more often. Advantages IMHO:
- the function is "physically" located nearer to the code where it's used, which improves readability IMHO
- the function has direct access to vars of the context, so less args are needed
- often these functions are very special for the tests of a specific context. Making them a regular
funcwould make them available for all tests though, which isn't needed
There was a problem hiding this comment.
Fair enough, could we make it stand up a bit more in the context then? Even some space around might be enough
There was a problem hiding this comment.
@slintes Thank you for the explanation. This is everything I thought.
In the current code, the test routine I wrote is duplicated and it will be troubled for maintenance in the future.
To remove the duplicated code, it needs access to vars of the context. This is the reason why I used this pattern.
@clobrano
Thank you for the comment. I added the following comment. Is that fine for you?
+ // runFARTests is a utility function to run FAR tests.
+ // It accepts a remediation strategy and a condition to determine if the tests should be skipped.
+ runFARTests := func(remediationStrategy v1alpha1.RemediationStrategyType, skipCondition func() bool) {
k-keiichi-rh
force-pushed
the
remove-dupe2etest
branch
from
da11bb1
to
9688952
Compare
|
/retest |
|
If the PR changes suit your needs and it is ready to be approved and reviewed, then please remove |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: k-keiichi-rh, razo7 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:
Approvers can indicate their approval by writing |
k-keiichi-rh
changed the title
|
As far as I checked, I have no concerns for this patch. |
|
Looking at the logs it works as designed. |
Why we need this PR
Refactoring for the duplicated code in e2e tests
Changes made
Extract the common test routine and remove the duplication