-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
tests: trycmd for integration testing #261
Conversation
This sounds almost exactly like what @cafkafk was talking about writing... I wonder if this is the answer to testing #197 Seems like a good proof of concept for both. The interface is literally what I would have to heavily modify VHS to do the same thing EDIT: Approved for concept anyway, obviously i don't assume that is our new pipeline XD |
Yeah I read your discussion and thought, wait, we can't be the first that are trying to test cli apps like that ... and I had to search a bit and first found Although I did plan to spend the evening on a different project I just had to try this, this looks like exactly what we were looking for. And it's made by a clap guy and they know what they are doing. Once @cafkafk gives her ok, we can start building a ton of tests, merge this, and then use it to verify the clap PR. |
This will be super useful indeed for the parser, I also thought about python based tests but staying in rust is also good. |
In general, this looks like just what we've been searching for! Very cool!
This is also great, it would suck to adopt something too obscure.
I'm gonna try this out a bit and get a feel for it. Probably an okay from me, but I just wanna see if there are any obvious problems. Will get back to you on that shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things I think we need to keep in mind:
- We'll likely still need sandboxing
- We might need some platform specific tests, e.g. for Windows (see: https://github.com/eza-community/eza/actions/runs/6150741633/job/16692367062?pr=26)
- Sandboxing on windows will likely be harder, and might simply be out of scope for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some logic to run these tests in a platform dependent way so windows CI isn't broken, but else, I'm very much approving this! It's looks really nice.
Ideally, that's all the changes we do in this PR, I'd prefer this merging as soon as it's not breaking anything so we can get to work on using this. E.g. I'd like to cleanup some of the "itest" stuff I did.
Yep, I already thought about that yesterday and I think this should be fairly doable within the integration as it's in Rust and we should be able to leverage
Alright, makes sense I'll cleanup a bit, make the reference folder for testing a little more elaborate with more than just plain and empty files, and try to get the windows stuff either running or skipped. |
Perhaps most of this could be done in a follow up PR as well? if we just get the windows stuff skipped or working I can start working on sandboxing today when that is merged. |
Alright I'll try to make it quick ... waiting for the CI is probably what takes most of the time anyway |
Alright, I revised This already ran through the CI also for windows as I also revised it's expected stdout for the long option where it failed before. Just rebased this again on main. Once the CI is done again this can be merged. |
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already ran through the CI also for windows as I also revised it's expected stdout for the long option where it failed before. Just rebased this again on main. Once the CI is done again this can be merged.
Alright! LGTM 👍
Changed commit summaries from feature -> feat so this conformed to convco.
Ahhh, I really gotta remember that one haha |
I'll look into adding a CI check, I also sometimes make mistakes with this, specially doc instead of docs >_< |
Just one thing I have to say about trycmd in the end: "This is the way!" 😆 |
I read through the discussion regarding VHS for testing and googled around a little and found this: https://crates.io/crates/trycmd
One can define the binary, arguments and environment variables in
![Screenshot from 2023-09-11 20-49-32](https://private-user-images.githubusercontent.com/49617392/267119212-89db4a62-434e-4147-a82d-744eedc8b0f9.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ1MDkwNjYsIm5iZiI6MTcyNDUwODc2NiwicGF0aCI6Ii80OTYxNzM5Mi8yNjcxMTkyMTItODlkYjRhNjItNDM0ZS00MTQ3LWE4MmQtNzQ0ZWVkYzhiMGY5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI0VDE0MTI0NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTM3MmQxZTc5NmIxOGU3YzQxMDk0OTljMDgwNTEzM2RmMjA2MTIxYmFmZTJiMDVkMTBkYjBhYTg3NGFlYTFhYmMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.2ccio-iuucX08xwUc0H_aIwa2YlBbhJAAVP92Viy8hw)
.toml
files and stdin in separate files. A simple.rs
file does the setup and integration withcargo test
. After defining those one can record the stdout and stderr withTRYCMD=dump cargo test --test cli_tests
which dumps them todump/
. Once moved to the chosen test folder, heretests/cmd
, one can actually perform tests with normalcargo test
, getting something like:And if a test fails you get quite extensive output:
![Screenshot from 2023-09-11 20-50-12](https://private-user-images.githubusercontent.com/49617392/267119366-90c9b291-d9a8-4dac-9229-941439d36c7d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ1MDkwNjYsIm5iZiI6MTcyNDUwODc2NiwicGF0aCI6Ii80OTYxNzM5Mi8yNjcxMTkzNjYtOTBjOWIyOTEtZDlhOC00ZGFjLTkyMjktOTQxNDM5ZDM2YzdkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI0VDE0MTI0NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNlNDFkODcxMDg1YzFkMjg3ZmEyNjk2NjNhMTQ5ZDc2ZWJjZDRlMzQxZjhkYjM0M2NiOWFhMTJmOGY2ZGJmNmImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.-tZP0QT80XsEbom3HzcJukYKHrGhpQqCqea52Uqri6o)
You can also easily overwrite outdated outputs with
TRYCMD=overwrite
.This might have some caveats like that there are no colors, and maybe things like terminal size can't be limited like with VHS, but I feel this is a very strong and solid framework with good integration, automation and very detailed information on test failure. Figures, since it's made by the guy that maintains
clap
which apparently also at least partly uses this for tests.This assert-rs community seems to have a couple of interesting crates for testing like https://crates.io/crates/assert_cmd for very fine grained cli tests. They describe it in the docs like
assert_cmd
is for treating tests like pets andtrycmd
for treating them like cattle, so easily managing hundreds of tests at once.Any thoughts, @cafkafk and @PThorpe92