refactor: centralize logic for parseInput tests #1033
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merging this branch changes the coverage (1 decrease, 9 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
General idea
This centralizes the logic into a single func which can be re-used everywhere. Not only does this save A LOT of boilerplate code (as you can see, sorry for that big diff). It also makes sure that all checks are called properly. As you can see I had to adjust some tests, e.g. because the flag groups or required flags weren't checked at all in some test implementations.
The new central func
The new func is located in
internal/pkg/testutils/testutils.goExplanation of additional adjustments to make this whole thing possible
Some adjustments were needed to be able to do this refactoring. To be able to use the same testutil function for all
parseInputfunc tests, allparseInputfuncs need the same signature. This wasn't given.The signature of the
parseInputfunc of a cobra command which take some argument(s) (mostly commands likecreateorlist) looks like this:The signature of the
parseInputfunc of a cobra command which take no args (mostly commands likecreateorlist) looked like this:As you can see the
argsparameter wasn't needed here and was left out therefore.I added this
argsparameter to allparseInputfuncs now so they all share the same signature (even when theargsparam is not needed in most of them).Note: The same goes for the
errorin the return value as you can see in the diff above.Thinking ahead
For the future it may make sense to define some interface so it's required to implement a
parseInputfunc with a given signature to completely enforce this implementation style.Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)