-
Notifications
You must be signed in to change notification settings - Fork 380
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
Add unit tests for match function in analytics controller #97
Conversation
@alierkilic thanks for you contribution, you can combine your unit test into one like :https://github.com/gocrane/crane/blob/main/pkg/controller/ehpa/hpa_test.go#L6 |
Should I mock the function calls inside Reconcile, SetupWithManager and createRecommendation? I don't see gomock used in the project. |
@alierkilic mock testing are wanted! you don't see those tests because we are still focusing on core feature development. It's perfect if you can start from such tasks. |
can you squash commits of this PR? |
@mfanjie I will start working on it |
3070a13
to
0fc3bd2
Compare
@alierkilic you can still refine this pr and merge the commit, more UT can be done in separated PR. |
@mfanjie can you accept this PR? If the merge commit is a problem, I can force push an older head, merge and then add my changes. It was like that before. I think I accidentally closed it. |
plz squash commits |
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.
suggest to simplify the description
Signed-off-by: alierkilic <erkilic.alican@gmail.com> Refactor unit test for match function Signed-off-by: alierkilic <erkilic.alican@gmail.com> Add unit test for match function in analytics controller
@mfanjie done |
/lgtm |
This will address issue: Unit tests for crane #91
If you have any problems with the test names please let me know