Open infokiller opened 3 years ago
matejcik
commented
3 years ago 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
commented
3 years ago 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
commented
3 years ago 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
commented
3 years ago 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
commented
3 years ago 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
After this PR, I also want to send a PR that adds CI