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

args_test: add helper functions #1426

Merged
merged 3 commits into from Nov 16, 2021
Merged

args_test: add helper functions #1426

merged 3 commits into from Nov 16, 2021

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Jun 29, 2021

This PR adds expectSuccess, getCommand and other helper functions to args_test, in order to reduce the verbosity of the source.

I recommend reviewing the commits one by one.

@umarcor umarcor mentioned this pull request Jun 29, 2021
@umarcor umarcor changed the title add helper functions to check errors in args_test args_test: add helper functions Jun 29, 2021
@umarcor umarcor force-pushed the test-helpers branch 2 times, most recently from 7205d45 to 3df8ae4 Compare Jul 1, 2021
@github-actions
Copy link

github-actions bot commented Oct 26, 2021

This PR is being marked as stale due to a long period of inactivity

@umarcor
Copy link
Contributor Author

umarcor commented Oct 26, 2021

Not stale, but ready to be merged.

Copy link
Collaborator

@jpmcb jpmcb left a 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 {
Copy link
Collaborator

@jpmcb jpmcb Nov 2, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@umarcor umarcor Nov 2, 2021

Choose a reason for hiding this comment

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

There are two main reasons: verbosity and human abstraction.

  • get_command is 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.

Copy link
Contributor Author

@umarcor umarcor Nov 2, 2021

Choose a reason for hiding this comment

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

See also these two dialogues with @jharshman where he did some suggestions about renaming tests and using sub-tests, which I applied:

Copy link
Collaborator

@jpmcb jpmcb Nov 16, 2021

Choose a reason for hiding this comment

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

Fair enough! I say let's get it in and if we need to refactor, we can go from there

umarcor added a commit to umarcor/cobra that referenced this pull request Nov 4, 2021
umarcor added a commit to umarcor/cobra that referenced this pull request Nov 4, 2021
umarcor added a commit to umarcor/cobra that referenced this pull request Nov 5, 2021
* noArgsWithArgs
* validWithInvalidArgs
* minimumNArgsWithLessArgs
* maximumNArgsWithMoreArgs
* exactArgsWithInvalidCount
* rangeArgsWithInvalidCount
umarcor added a commit to umarcor/cobra that referenced this pull request Nov 15, 2021
* noArgsWithArgs
* validWithInvalidArgs
* minimumNArgsWithLessArgs
* maximumNArgsWithMoreArgs
* exactArgsWithInvalidCount
* rangeArgsWithInvalidCount
umarcor added 3 commits Nov 15, 2021
* noArgsWithArgs
* validWithInvalidArgs
* minimumNArgsWithLessArgs
* maximumNArgsWithMoreArgs
* exactArgsWithInvalidCount
* rangeArgsWithInvalidCount
jpmcb
jpmcb approved these changes Nov 16, 2021
@jpmcb jpmcb merged commit 9e1d6f1 into spf13:master Nov 16, 2021
11 checks passed
@umarcor umarcor deleted the test-helpers branch Nov 16, 2021
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.

None yet

2 participants