Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 98 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions hugr-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ anyhow.workspace = true
thiserror.workspace = true
tracing = "0.1.41"
tracing-subscriber = { version = "0.3.19", features = ["fmt"] }
human-panic = "2"

[lints]
workspace = true
Expand All @@ -37,6 +38,9 @@ workspace = true
enum_missing = "warn"
struct_missing = "warn"

[features]
panic-test = []

[dev-dependencies]
assert_cmd = { workspace = true }
assert_fs = { workspace = true }
Expand Down
9 changes: 9 additions & 0 deletions hugr-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

human_panic::setup_panic!();
}

// Test-only panic trigger (harmless in production; only fires if env is set).
if std::env::var_os("PANIC_FOR_TESTS").is_some() {
panic!("triggered by test");
}
let cli_args = CliArgs::parse();

let level = match cli_args.verbose.filter() {
Expand Down
64 changes: 64 additions & 0 deletions hugr-cli/tests/human_panic_integration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
//! Integration test for `human-panic`.
//!
//! Builds the release binary with the `panic-test` feature, runs it with a
//! test-only panic trigger and an isolated temp dir, asserts the banner,
//! and verifies a TOML report is written. Temp dir is removed at the end.

//! Black-box integration test for `human-panic`.

use predicates::str::contains; // for cargo_bin()
use std::process::Command;
use tempfile::TempDir;

#[test]
fn human_panic_writes_report() {
// Isolated temp dir for the crash report.
let tmp = TempDir::new().expect("create tempdir");
let tmp_path = tmp.path();

// 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");
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not used assert_cmd before but looking at the docs I see

  1. The cargo_bin method appears to be provided for exactly this kind of case, i.e. running cargo
  2. 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 ?

cmd.args([
"run",
"--release",
"-p",
"hugr-cli",
"--bin",
"hugr",
"--", // end cargo args; program args would follow
]);

// Isolate temp location & trigger the test panic in the child process.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

if cfg!(windows) {
cmd.env("TEMP", tmp_path).env("TMP", tmp_path);
} else {
cmd.env("TMPDIR", tmp_path);
}
cmd.env("PANIC_FOR_TESTS", "1");

// Expect non-zero exit and the banner on stderr (release only).
assert_cmd::Command::from_std(cmd)
.assert()
.failure()
.stderr(contains("Well, this is embarrassing."));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It'd be good to look for the string in the panic too


// Confirm a .toml report exists in our temp dir.
let made_report = std::fs::read_dir(tmp_path)
.unwrap()
.filter_map(Result::ok)
.any(|e| {
e.path()
.extension()
.and_then(|ext| ext.to_str())
.is_some_and(|ext| ext.eq_ignore_ascii_case("toml"))
});
assert!(
made_report,
"expected a human-panic report in {:?}",
tmp_path
);

// Explicit cleanup; surface any removal errors.
tmp.close().expect("failed to remove temp dir");
}
Loading