Rework webhook test method to actually send requests #640
Conversation
Currently, the webhook's objects are mocked through the decoder interface, but the webhooks don't actually get a request. This replaces the decoder with a generic encoder based on the scheme and instead passes a "proper" request to the webhooks under test.
markusthoemmes
force-pushed
the
rework-webhook-tests
branch
from
3a83774 to
0ebac4a
Compare
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "ke2", | ||
| }, | ||
| dec, err := admission.NewDecoder(scheme.Scheme) |
There was a problem hiding this comment.
It seems that the call will never produce an error:
// NewDecoder creates a Decoder given the runtime.Scheme
func NewDecoder(scheme *runtime.Scheme) (types.Decoder, error) {
return decoder{codecs: serializer.NewCodecFactory(scheme)}, nil
}
In addition I think we could just have a pointer and check that value in tests and fail instead of panicking.
There was a problem hiding this comment.
Heh, great catch. Pushed a version that ditches the error handling. I moved it outside of tests since we never expect it to fail so I didn't want to pollute tests. Now it's even better.
| Version: version, | ||
| // RequestFor generates an admission request for the given object. | ||
| func RequestFor(obj runtime.Object) (types.Request, error) { | ||
| b, err := json.Marshal(obj) |
There was a problem hiding this comment.
bytes is a stdlib package name, so I'd rather not use it.
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "ks2", | ||
| }, | ||
| dec, err := admission.NewDecoder(scheme.Scheme) |
| validator := Validator{} | ||
| validator.InjectDecoder(&mockDecoder{ke1}) | ||
| result := validator.Handle(context.TODO(), types.Request{}) | ||
| validator.InjectDecoder(decoder) |
There was a problem hiding this comment.
This also returns an error but is always nil, weird...
There was a problem hiding this comment.
Yeah, we usually just don't care in tests 🤷
| *p = *mock.ke | ||
| result := validator.Handle(context.Background(), req) | ||
| if result.Response.Allowed { | ||
| t.Errorf("Too many KnativeEventings: %v", result.Response) |
There was a problem hiding this comment.
We could aggregate response reasons and check them. Validation checks the namespace and the uniqueness of the namespace. Here since no required env var is set it will return true for the invalid namespace check and will return Allowed=false with reason text: "Only one KnativeEventing allowed per namespace".
It seems to me that if something changes in the implementation this test may pass for the wrong reason eg. fail the first check and pass the latter.
There was a problem hiding this comment.
I agree. Would you be fine if we did that in a separate PR? This refactor is on the blocking path for the version bumping, so I'd rather not conflate the concerns.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes, skonto 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 |
Currently, the webhook's objects are mocked through the decoder interface, but the webhooks don't actually get a request. This replaces the decoder with a generic encoder based on the scheme and instead passes a "proper" request to the webhooks under test.
Note: Newer versions of controller-runtime no longer have a decoder interface, so this is prep work to have a much smaller diff once bumping the version.