e2e/storage: distinct test names for provisioning #70987
Conversation
k8s-ci-robot
added
release-note-none
|
@pohly: GitHub didn't allow me to assign the following users: mkimuram. Note that only kubernetes members and repo collaborators can be assigned. 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/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
|
@pohly: GitHub didn't allow me to request PR reviews from the following users: mkimuram. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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/test-infra repository. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/milestone v1.13 |
|
/priority backlog |
k8s-ci-robot
added
priority/backlog
| @@ -173,7 +173,7 @@ type provisioningTestInput struct { | |||
| } | |||
|
|
|||
| func testProvisioning(input *provisioningTestInput) { | |||
| It("should provision storage", func() { | |||
| It("should provision storage without mount options", func() { | |||
There was a problem hiding this comment.
Maybe a better name would be "with default storageclass options". For example there are provisioning test cases for reclaim policy and other fields
There was a problem hiding this comment.
Should I then also rename the "should provision storage with mount options" test?
Right now it is consistent: https://github.com/kubernetes/kubernetes/blob/9df867dac94c35f1c9b74739a585ec38be911f06/test/e2e/storage/testsuites/provisioning.go#L175-L187
If I rename just this name here, the difference between these two test cases would become less obvious.
There was a problem hiding this comment.
Storage class options is not the same as mount options. Maybe the use of options for storage class is not the best. Storage class settings? Storage class fields?
There was a problem hiding this comment.
How about "should provision storage with defaults" and "should provision storage with mount options"?
In the end I don't care too much. @msau42 please pick something for both tests and I'll change the PR accordingly.
There was a problem hiding this comment.
With defaults sounds fine to me. Thanks!
A test name should not be the subset of another, because then it is impossible to focus on it. In this case, -ginkgo.focus=should.provision.storage ran both "should provision storage" and "should provision storage with mount options" without the ability to select just the former.
pohly
force-pushed
the
storage-volume-testsuites-names
branch
from
9df867d
to
7efad66
Compare
k8s-ci-robot
removed
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, saad-ali 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 |
k8s-ci-robot
added
the
approved
|
/test pull-kubernetes-e2e-kops-aws |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
A test name should not be the subset of another, because then it is
impossible to focus on it.
In this case, -ginkgo.focus=should.provision.storage ran both "should
provision storage" and "should provision storage with mount options"
without the ability to select just the former.
Does this PR introduce a user-facing change?:
/sig storage
/assign @mkimuram