feat!: Set up human_panic in hugr-cli
#2511
Conversation
|
|
||
| // Run the release CLI binary from the workspace root. | ||
| // No features needed: main() installs human-panic in release builds. | ||
| let mut cmd = Command::new("cargo"); |
There was a problem hiding this comment.
I've not used assert_cmd before but looking at the docs I see
- The cargo_bin method appears to be provided for exactly this kind of case, i.e. running cargo
- That recommends use of the
cargo_bin!macro
Did you try these, is there some reason why they don't work here?
If we can't use those, then (if I understand correctly) you can just create a assert_cmd::Command here and call .args, .env etc. on it exactly the same way, which looks to be the preferred way of using assert_cmd ?
| @@ -9,6 +9,15 @@ use hugr_cli::{CliArgs, CliCommand}; | |||
| use tracing::{error, metadata::LevelFilter}; | |||
|
|
|||
| fn main() { | |||
| // Enable human-panic for release builds, or when tests force it via env. | |||
| if cfg!(not(debug_assertions)) || std::env::var_os("FORCE_HUMAN_PANIC").is_some() { | |||
There was a problem hiding this comment.
The human-panic docs state that "It only displays a human-friendly panic message in release mode:" so while you have written some mildly complex logic here I wonder do we really need this?
| "--", // end cargo args; program args would follow | ||
| ]); | ||
|
|
||
| // Isolate temp location & trigger the test panic in the child process. |
There was a problem hiding this comment.
I'm of two minds about these environment variables. I don't see them documented anywhere on the human-panic crate! I mean, certainly it seems reasonable for human-panic to use std::env::temp_dir and the docs there mention these variables, along with a warning that this may change in the future...
On the plus side, it seems good for the test to clean up neatly after itself 😃.
But an alternative would be - human-panic outputs the exact temporary-file path in it's stderr (in quotes after We have generated a report file at ) - which is mentioned in the docs - one could extract the path from that. However that would probably need a regex, which does not look that easy (e.g. probably not by using the predicates library) 😢 ...
So yeah, could go either way here, did you consider other approaches?
| assert_cmd::Command::from_std(cmd) | ||
| .assert() | ||
| .failure() | ||
| .stderr(contains("Well, this is embarrassing.")); |
There was a problem hiding this comment.
nit: It'd be good to look for the string in the panic too
Adding the
human_panicfeature and testing it inhugr-cli. Re. #2337 .Thanks!BREAKING CHANGE: Added dependencies for
human_panic, included aPANIC_FOR_TESTSenvironment in the driver, and added a corresponding test.