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

ci: refactor node controller unit tests #2380

Conversation

m-ildefons
Copy link
Contributor

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

@m-ildefons m-ildefons self-assigned this Dec 18, 2023
@m-ildefons m-ildefons requested a review from a team as a code owner December 18, 2023 13:39
@m-ildefons m-ildefons force-pushed the wip/7332-refactor-node-controller-unit-tests branch from 4818623 to 39776e2 Compare December 18, 2023 13:50
PhanLe1010
PhanLe1010 previously approved these changes Dec 18, 2023
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

I very like the new test design pattern!
In general LGTM expect for some small nits

controller/node_controller_test.go Outdated Show resolved Hide resolved
controller/node_controller_test.go Outdated Show resolved Hide resolved
controller/node_controller_test.go Outdated Show resolved Hide resolved
@m-ildefons m-ildefons force-pushed the wip/7332-refactor-node-controller-unit-tests branch from 39776e2 to 3a5d6b2 Compare December 19, 2023 13:18
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@PhanLe1010 PhanLe1010 enabled auto-merge (rebase) December 21, 2023 19:43
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>
- Rename "TestCase" to "NodeControllerFixture"
- Rename "Expectation" to "NodeControllerExpectation"
- Avoid passing unnecessary parameters to test helper functions
- Use "checkDiskConditions" test helper function in all places where
  disk conditions need to be tested

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@PhanLe1010 PhanLe1010 force-pushed the wip/7332-refactor-node-controller-unit-tests branch from 3a5d6b2 to f9344c7 Compare December 21, 2023 19:44
@PhanLe1010 PhanLe1010 disabled auto-merge December 21, 2023 19:44
@PhanLe1010 PhanLe1010 merged commit cc7f120 into longhorn:master Dec 21, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants