-
Notifications
You must be signed in to change notification settings - Fork 366
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
🌱 apibindingdeletion: use mockable methods #2173
🌱 apibindingdeletion: use mockable methods #2173
Conversation
newPartialObject("v1", "Pod", "pod1", "ns1", nil), | ||
newPartialObject("apps/v1", "Deployment", "deploy1", "ns1", nil), | ||
}, | ||
metadataClientActionSet: []metaAction{ |
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.
I removed any check for the client calls here since it seems sufficient to validate the method we're testing - just inputs and outputs - without assuming implementation details in there.
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.
I kinda think we should be validating what it's issuing - e.g. at least that it's trying to delete the right stuff
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.
Happy to have someone do that in a follow-up - notably, the original test did not do that.
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.
File an issue or add a TODO?
pkg/reconciler/apis/apibindingdeletion/apibinding_deletion_controller.go
Show resolved
Hide resolved
pkg/reconciler/apis/apibindingdeletion/apibinding_deletion_controller.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/apis/apibindingdeletion/apibinding_deletion_controller.go
Outdated
Show resolved
Hide resolved
newPartialObject("v1", "Pod", "pod1", "ns1", nil), | ||
newPartialObject("apps/v1", "Deployment", "deploy1", "ns1", nil), | ||
}, | ||
metadataClientActionSet: []metaAction{ |
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.
I kinda think we should be validating what it's issuing - e.g. at least that it's trying to delete the right stuff
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
72b7128
to
ae76451
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc 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 |
apibindingdeletion: use committer library for patches
Signed-off-by: Steve Kuznetsov skuznets@redhat.com
apibindingdeletion: don't use fake clients in tests
Signed-off-by: Steve Kuznetsov skuznets@redhat.com
/cc @qiujian16
/assign @ncdc
Part of #2167