[OCPPLAN-9509] skip storage tests if cap disabled #27470
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qJkee The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @wking |
|
/retest |
pkg/test/ginkgo/cmd_runsuite.go
Outdated
| } | ||
|
|
||
| if !storageEnabled { | ||
| fmt.Fprintln(opt.Out, "Skipping [sig-storage] tests because Storage capability disabled") |
There was a problem hiding this comment.
100% of sig-storage tests can't be run when this cap is disabled? including things like emptydir storage?
There was a problem hiding this comment.
nit, can we move this line down to an else block of the newly-conditional tests = append(tests, storageTestsCopy...)?
if storageEnabled {
// I thought about randomizing the order of the kube, storage, and openshift tests, but storage dominates our e2e runs, so it doesn't help much.
storageTestsCopy := copyTests(storageTests)
q.Execute(testCtx, storageTestsCopy, max(1, parallelism/2), status.Run) // storage tests only run at half the parallelism, so we can avoid cloud provider quota problems.
tests = append(tests, storageTestsCopy...)
} else {
fmt.Fprintln(opt.Out, "Skipping [sig-storage] tests because Storage capability disabled")
}Keeping that collected together reduces the chances that someone touches one of the branches without noticing that they may also need to adjust the other.
|
|
||
| // no need to count storage tests if storage disabled | ||
| if !storageEnabled { | ||
| storageTests = nil |
There was a problem hiding this comment.
Ah, clearing this here keeps expectedTestCount from being inflated with subsequently-not-added storage tests. Maybe this is where we should log the Skipping ....
pkg/test/ginkgo/cmd_runsuite.go
Outdated
| q.Execute(testCtx, storageTestsCopy, max(1, parallelism/2), status.Run) // storage tests only run at half the parallelism, so we can avoid cloud provider quota problems. | ||
| tests = append(tests, storageTestsCopy...) | ||
| // run storage tests only if storage capability enabled | ||
| if storageEnabled { |
There was a problem hiding this comment.
With this nil-ing above, I don't understand why we need a conditional here. Can we leave this block of code unchanged?
Do not run storage related tests if storage capability disabled.
qJkee
force-pushed
the
OCPPLAN-9509-check-caps
branch
from
d1eb466 to
9e66b24
Compare
|
/retest |
|
@qJkee: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
closing because of #27481 |
3 participants
Do not run storage related tests if storage capability disabled.