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] Node Controller Unit Tests #7332

Closed
m-ildefons opened this issue Dec 14, 2023 · 3 comments
Closed

[REFACTOR] Node Controller Unit Tests #7332

m-ildefons opened this issue Dec 14, 2023 · 3 comments
Assignees
Labels
component/longhorn-manager Longhorn manager (control plane) kind/refactoring Request for refactoring (code) kind/test Request for adding test
Milestone

Comments

@m-ildefons
Copy link

Is your improvement request related to a feature? Please describe

The current unit tests for the node controller are utilizing the testing framework improperly. There is only one "test", which repeatedly sets up data structures and mock clients for the K8s API and then iterates over a list of test cases (i.e. inputs and outcomes) and executes the same checks for each case. This makes it hard to modify and add new test cases, as existing test cases have to be modified to pass additional checks as well.
In addition to that, this makes the output hard to understand when a test case fails. It also makes it very hard to tell what the test expects as output on each test case.

Describe the solution you'd like

  • Break up the test cases into individual test functions
  • Reduce the amount of involved resources in teach test case to a minimum
  • Extend the test suite to additionally ensure the node controller emits K8s events only in desired situations

Describe alternatives you've considered

  1. Keep the current architecture and add test data for PR#2356 to each test case as well as an additional test case for PR#2356

  2. Break up the test into individual test functions and add a new test function for PR#2356

@m-ildefons m-ildefons added the kind/refactoring Request for refactoring (code) label Dec 14, 2023
@m-ildefons m-ildefons self-assigned this Dec 14, 2023
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this issue Dec 18, 2023
Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this issue Dec 18, 2023
Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@innobead
Copy link
Member

@m-ildefons remember to add the issue to the current milestone when you are working on it.

@innobead innobead added this to the v1.6.0 milestone Dec 18, 2023
@innobead innobead added component/longhorn-manager Longhorn manager (control plane) kind/test Request for adding test labels Dec 18, 2023
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this issue Dec 18, 2023
Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@m-ildefons
Copy link
Author

👍 Sorry, I though it had, but it turned out I had just added it to the sprint but not the milestone as well.

m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this issue Dec 19, 2023
Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
PhanLe1010 pushed a commit to m-ildefons/longhorn-manager that referenced this issue Dec 21, 2023
Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
PhanLe1010 pushed a commit to longhorn/longhorn-manager that referenced this issue Dec 21, 2023
* ci: refactor node controller unit tests

Refactor node controller unit tests:
- Break up single test function into multiple test functions
- Create a clear separation between test case input data and expected
  result data
- Implement existing tests with new structure as individual test
  functions
- Avoid checking things that aren't subject matter of the test case at
  hand

related-to: longhorn/longhorn#7332

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@m-ildefons
Copy link
Author

The changes are merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/longhorn-manager Longhorn manager (control plane) kind/refactoring Request for refactoring (code) kind/test Request for adding test
Projects
Status: Done
Development

No branches or pull requests

2 participants