Skip to content
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

Refactor controller unit test with proper mock #220

Closed
leakingtapan opened this issue Feb 20, 2019 · 6 comments
Closed

Refactor controller unit test with proper mock #220

leakingtapan opened this issue Feb 20, 2019 · 6 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone

Comments

@leakingtapan
Copy link
Contributor

leakingtapan commented Feb 20, 2019

Proper mocking should be used for controller unit testing instead of using the current fake cloud provider that has fake logic that are implemented for sanity testing purpose.

Goal

  • move fake cloud provider inside tests/sanity package and make sure it is only used there
  • auto generate cloud provider using gomock. See ./hack/update-gomock as an example
  • update controller_test.go and node_test.go to use the generated cloud provider mock.

Reference PR

Could you create a mock_cloud using gomock? With that, we could truly unit test controller service without depending on the fake cloud implementation.

Here is something similar I did for another driver: https://github.com/aws/aws-fsx-csi-driver/blob/master/pkg/driver/controller_test.go#L224

Originally posted by @leakingtapan in #219

@bertinatto
Copy link
Member

+1

The fake cloud provider object was introduced way before I started using gomock, so we should definitely get rid of it in unit tests.

@bertinatto bertinatto added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Mar 1, 2019
@zacharya
Copy link
Contributor

zacharya commented Apr 2, 2019

If no one has taken this up, I'd like to contribute

@leakingtapan
Copy link
Contributor Author

@zacharya I updated the issue description. let me know if you have any questions, will be happy to clarify more.

@zacharya
Copy link
Contributor

zacharya commented Apr 5, 2019

I submitted #269 for this. Let me know if you have any issues with the implementation or test refactors.

@leakingtapan
Copy link
Contributor Author

/close

@k8s-ci-robot
Copy link
Contributor

@leakingtapan: Closing this issue.

In response to this:

/close

Instructions 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.

dobsonj pushed a commit to dobsonj/aws-ebs-csi-driver that referenced this issue May 18, 2023
…ency-openshift-4.14-ose-aws-ebs-csi-driver

Updating ose-aws-ebs-csi-driver images to be consistent with ART
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

No branches or pull requests

4 participants