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

Add tests to CLI #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

infokiller
Copy link
Contributor

No description provided.

@infokiller
Copy link
Contributor Author

After this PR, I also want to send a PR that adds CI

@matejcik
Copy link
Contributor

matejcik commented Jan 4, 2021

These tests seem rather pointless, as they're checking that the command executes without an error code but don't really care what is the output.

If you want to introduce tests for the CLI, I'd recommend including them in the unit test suite: https://click.palletsprojects.com/en/7.x/testing/

@infokiller
Copy link
Contributor Author

I agree testing for correctness too is better, but I don't think they're totally pointless, because they do catch breakage in the CLI interface, which the regular unit tests don't.
I think my motivation was that I made a change in my fork and ran into an issue where the CLI was broken (but the regular tests passed), but it was a long time ago.
I think adding click tests wouldn't be as good as a shell script as an integration test, since it doesn't simulate how a real user would run it.

Anyway, I can add tests for correctness if you think it would be valuable, but if you think even that is pointless I'll just close the PR.

@matejcik
Copy link
Contributor

matejcik commented Jan 8, 2021

Tests for correctness would definitely be worth it, but again, I'd suggest plugging them in the unit test infrastructure. I'm wary of including code in another language (shell test harnesses) that could itself be broken, and that can't easily be executed in a Python-standard way.

Note that the Click tests are more-or-less simulating what a user is doing. The only advantage of running through shell is that you're also testing the Click library itself -- but if that's broken, there isn't much we can do.

I would accept this as a smoke-test for CI, trying to run a couple basic commands to see that they run (as opposed to crash). In that case, however, your shell script can be reduced to ~6 lines that do the actual commands -- and should be included in the CI runner code itself.

Also, what sort of CI do you want to add? We used to use Travis, but we're moving away from them seeing as they are dropping free support for open-source projects.

@infokiller
Copy link
Contributor Author

Alright, I'll play with the click tests and see if I can break the CLI without breaking the click tests. If I can't, I'll just switch to click tests from the shell script, and if I can, that will show there is added value to the shell scripts, so I'll post the results and you can decide if you want the added complexity of another language.

Another advantage I see in a shell script test is that you no longer rely on the python CLI framework, so you could switch to argparse without changing the tests. That's a bit tangent, but In general I personally try to avoid click because I don't like extra dependencies unless I really need them, and I find argparse pretty good as is. Out of curiosity, why did you use click and not the built in argparse?

As for CI, my plan was to use either Github Actions or Gitlab CI, both of which worked well for me in the past and are easy to understand. Any preferences?

@matejcik
Copy link
Contributor

why did you use click and not the built in argparse?

I'd say Click to argparse is like pytest to unittest: less verbose, nicer to use, fewer lines to MVP. Also this project is supposed to be a reference implementation so not much thought was put into the dependencies.

As for CI, my plan was to use either Github Actions or Gitlab CI, both of which worked well for me in the past and are easy to understand. Any preferences?

Github Actions is slightly preferred

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