args_test: add helper functions #1426
Conversation
umarcor
changed the title
umarcor
force-pushed
the
test-helpers
branch
2 times, most recently
from
7205d45
to
3df8ae4
Compare
|
This PR is being marked as stale due to a long period of inactivity |
|
Not stale, but ready to be merged. |
There was a problem hiding this comment.
Thanks so much for the work that went into this!
I'm not sure it makes sense to abstract so much away from the tests themselves into helper functions. Is there a real need to do this? For me personally, it only makes the tests harder to read since now I have to jump to a different function to understand what it does
| @@ -5,6 +5,18 @@ import ( | |||
| "testing" | |||
| ) | |||
|
|
|||
| func getCommand(args PositionalArgs, withValid bool) *Command { | |||
There was a problem hiding this comment.
Hmmm not sure if I'm a huge fan of using helper methods to abstract away creating a command struct. What benefit do we get adding this complexity when using literal structs inline gets us the same thing?
There was a problem hiding this comment.
There are two main reasons: verbosity and human abstraction.
get_commandis used 16 times, and it contains 9 lines. That's 16*7~=112 lines of duplicated code. It's 40% of the file.- Using this abstraction makes it easier to understand that two orthogonal features are tested: different types of argument constraints and usage of ValidArgs. The orthogonality of those features is fixed in args_test refactorization after generalizing ValidArgs #841. In fact, this PR is a subset of that one.
As you can see in https://github.com/spf13/cobra/pull/841/files#diff-a48034865676903aed3da7be846f56efab79615c941b4b0089d0eb17584d391a, the complexity of this file would explode in #841 if we didn't have this (and other) helper function.
There was a problem hiding this comment.
See also these two dialogues with @jharshman where he did some suggestions about renaming tests and using sub-tests, which I applied:
There was a problem hiding this comment.
Fair enough! I say let's get it in and if we need to refactor, we can go from there
* noArgsWithArgs * validWithInvalidArgs * minimumNArgsWithLessArgs * maximumNArgsWithMoreArgs * exactArgsWithInvalidCount * rangeArgsWithInvalidCount
* noArgsWithArgs * validWithInvalidArgs * minimumNArgsWithLessArgs * maximumNArgsWithMoreArgs * exactArgsWithInvalidCount * rangeArgsWithInvalidCount
* noArgsWithArgs * validWithInvalidArgs * minimumNArgsWithLessArgs * maximumNArgsWithMoreArgs * exactArgsWithInvalidCount * rangeArgsWithInvalidCount
umarcor
force-pushed
the
test-helpers
branch
from
3ad5a92
to
4120f6e
Compare
This PR adds
expectSuccess,getCommandand other helper functions toargs_test, in order to reduce the verbosity of the source.I recommend reviewing the commits one by one.