-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix improper use of non-polling Eventually in tests #1
base: fallback-validation
Are you sure you want to change the base?
Conversation
…plica counts Signed-off-by: Vivian Dai <[email protected]>
Signed-off-by: Vivian Dai <[email protected]>
Signed-off-by: Vivian Dai <[email protected]>
Signed-off-by: Vivian Dai <[email protected]>
Signed-off-by: Vivian Dai <[email protected]>
}).ShouldNot(HaveOccurred()) | ||
}) | ||
|
||
var _ = It("shouldn't create so when stabilizationWindowSeconds exceeds 3600", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation for stabilizationWindowSeconds
was added in kedacore#4983 via a validation patch directly on the CRD. It looks like the patch doesn't get picked up by the admission webhook, so the webhook actually thinks this is a valid object. This test is a false positive, and I don't think it belongs in this file with the rest of the admission webhook unit tests.
# error message when rejected by admission webhook
Error from server (Forbidden): error when creating "scaledobject.yaml": admission webhook "vscaledobject.kb.io" denied the request: type is cpu , but fallback it is not supported by the CPU & memory scalers
# error message when rejected by CRD validation
The ScaledObject "keda-test" is invalid: spec.advanced.horizontalPodAutoscalerConfig.behavior.scaleDown.stabilizationWindowSeconds: Invalid value: 36000: spec.advanced.horizontalPodAutoscalerConfig.behavior.scaleDown.stabilizationWindowSeconds in body should be less than or equal to 3600
I'm not sure how to test a direct CRD patch though so I haven't added a replacement test - need to look into it more, or open to suggestions here.
ce73bea
to
32ff796
Compare
Description
As discovered in kedacore#5962 (comment), the
Eventually
function of Gomega is meant toblock when called and attempts an assertion periodically until it passes or a timeout occurs
(docs).This means we should be using it with polling functions such as
Get
to wait for an event to happen (example), but not functions such asCreate
unless we're intending to repeatedly try the creation until something different happens.Our incorrect usage causes the existing tests to report false results, because in cases such as:
An error does occur here, but that's caused by the error
resourceVersion should not be set on objects to be created
sinceso
gets modified in-place after the first create and we're trying to apply it again.On the other hand, if we expect no error to occur with the create:
If the first
Create
passes, then the Eventually block also passes. In this case, there's no point having theEventually
since we're not waiting for anything.Changes
As proof that this
Eventually
is not necessary and will only lead to false positives, I've tried removing the wrapper from all places where it's called with a mutatingCreate
orUpdate
call. With this change, the tests still pass.Checklist
Fixes #
Relates to #