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

Add unit test for Node controller #1648

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

cadenmarchese
Copy link
Collaborator

Which issue this PR addresses:

https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/10227470/

What this PR does / why we need it:

Adds some coverage to the Node controller to ensure we're adding and reading our annotations properly. Also adds a const.go file for the node package since it's standard practice among other operator controllers.

Test plan for issue:

make test-go
go test -v

Is there any documentation that needs to be updated for this PR?

No

@cadenmarchese cadenmarchese added priority-low Low priority issue or pull request ready-for-review labels Jul 29, 2021
Copy link
Contributor

@troy0820 troy0820 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits. You have coverage over all these functions but because you are doing a table test you could create a test that tests every condition in the isDraining function. So the annotations to see if they are blank, etc. That way the confidence of this function will increase as you tested that each condition can create the return value necessary.

pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need the following tests for this controller:

  1. Test individual functions such as isDraining with all possible scenarios as Troy suggested.
  2. Make isDraining mockable and test the controller via Reconcile. This way we don't have to meet all the conditions inside isDraining in test data and don't have to care about implementation of isDraining: we just make it returns either true or false and cover these code paths.

I'm happy for these two things to be submitted in a separate PRs, if you prefer. But we definitely need the second point - without it there is a lot of code (contriller's main logic) without coverage.

@cadenmarchese
Copy link
Collaborator Author

@m1kola @troy0820 I agree we should test via Reconcile, and I had previously attempted to do it unsuccessfully, but I can work on it more. I have addressed the other comments and squashed commits, so we can either merge with followup issue to change to Reconcile, or else I will continue to work on it via this PR - whatever is preferred.

@troy0820 troy0820 added the size-small Size small label Aug 2, 2021
Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cadenmarchese let's finish covering individual functions in this PR and cover Reconcile in a separate PR.

pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controllers/node/node_controller_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only one small suggestion for TestSetAnnotation.

Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We still need to cover main logic of this controller (Reconcile method). Let's do it in a separate PR.

@m1kola m1kola dismissed troy0820’s stale review August 17, 2021 12:28

Troy's feedback was addressed, I believe. Dismissing the review.

@m1kola m1kola merged commit 0b39eb5 into Azure:master Aug 17, 2021
@cadenmarchese cadenmarchese deleted the node-controller-tests branch August 24, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-low Low priority issue or pull request ready-for-review size-small Size small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants