openshift integration #403
Conversation
There was a problem hiding this comment.
Is the intermittent failure due to the Status Condition not being present, or having the wrong value? As of this change, the Eventually will loop until a status condition is returned, regardless of if it's 'True' or not, and then still fail if it's 'False'. Perhaps the checks on each returned condition should be part of the Eventually loop?
There was a problem hiding this comment.
I would add a descriptive message for the Should assertion, so that Gomega reports something more useful than "Expected 1, received 0" 🙂
https://onsi.github.io/gomega/#annotating-assertions
|
@coro Actually in this case the condition seems not to be present, as the error result is: <[]v1beta1.Condition | len:0, cap:0>: nil, but after putting a timeout seems then to always be present. |
There was a problem hiding this comment.
Looks good. One small thing, the 20s timeout is used only for waiting for status condition to be updated and the time out is there to accommodate smalled openshift env. Perhaps create a variable to represent the time out similar to how cluster operator specific timeout const used across the system tests suite? https://github.com/rabbitmq/cluster-operator/blob/6d5295b4858c3b70b6ad3cf63dee766f9e3a00b1/system_tests/utils.go#L61-L63
Then you could add a comment to the timeout explaining its mostly required for openshift.
DanielePalaia
force-pushed
the
systemtests_eventually_on_status_conditions
branch
from
c341d85 to
617e801
Compare
|
@ChunyiLyu @Zerpet Thanks for the suggestions. Implemented! |
system_tests/tls_system_test.go
Outdated
| Expect(k8sClient.Get(ctx, types.NamespacedName{Name: policy.Name, Namespace: policy.Namespace}, &fetchedPolicy)).To(Succeed()) | ||
| return fetchedPolicy.Status.Conditions | ||
| }, 10, 2).Should(HaveLen(1)) | ||
| }, 20, 2).Should(HaveLen(1), "policy status condition should be present") |
There was a problem hiding this comment.
I think you would like to use waitUpdatedStatusConditionhere as well
There was a problem hiding this comment.
Yes indeed :) fixed, thanks!
system_tests/utils.go
Outdated
| "k8s.io/utils/pointer" | ||
| ) | ||
|
|
||
| // Used for openshift smaller cluster |
There was a problem hiding this comment.
nitpick: I would say something like "useful for small Openshift environment while updating status takes a long time"
There was a problem hiding this comment.
Fair enough! Fixed!
Wrap expect into eventually on updating status condition in system tests to let tests be more stable in the smaller openshift environment
DanielePalaia
force-pushed
the
systemtests_eventually_on_status_conditions
branch
from
617e801 to
f525396
Compare
Wrap expect into eventually on updating status condition in system tests
to let tests be more stable in the smaller openshift environment
This closes #
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes
Summary Of Changes
Additional Context