Standardise usage of #[should_panic] vs panic::catch_unwind in te…

…sts (#715)

The `#[should_panic]` annotation allows marking tests that are
expected to panic. It accepts an `expected` field which allows for
substring matching against the panic message.

`panic::catch_unwind` allows doing something similar, but requires
a lot more boilerplate. However, it can be used in cases where the
panic message contains dynamic values and substring matching isn't
adequate for the purpose of what the tests intends to test.

In general I believe we should prefer `#[should_panic]` and only use
`panic::catch_unwind` when it benefits the test.

I've added this recommendation to the top of the integration tests file,
and updated several of the tests to follow it.

Notably:
- For the tests `address_for_port_when_container_crashed` and
  `app_dir_invalid_path`, the tests now uses `panic::catch_unwind`
  allowing the assertions to match against the whole dynamic message,
  improving coverage.
- For `expected_pack_failure_still_panics_for_non_pack_failure`, the
  test has been switched back to using `#[should_panic]` (as it was before
  #666), since the purpose of the test is not to check the full error message
  of that specific failure mode, but instead to confirm that a non-pack failure
  isn't marked as a success when using `.expected_pack_result()`.

See:
https://doc.rust-lang.org/book/ch11-01-writing-tests.html#checking-for-panics-with-should_panic
https://doc.rust-lang.org/std/panic/fn.catch_unwind.html
https://doc.rust-lang.org/std/panic/struct.AssertUnwindSafe.html

libcnb-test/tests/integration_test.rs

@@ -2,20 +2,24 @@
 //!
 //! All integration tests are skipped by default (using the `ignore` attribute),
 //! since performing builds is slow. To run the tests use: `cargo test -- --ignored`
+//!
+//! When testing panics, prefer using `#[should_panic(expected = "...")]`, unless you need
+//! to test dynamic values, in which case the only option is to use `panic::catch_unwind`
+//! since `should_panic` doesn't support globs/regular expressions/compile time macros.
 
 // Enable Clippy lints that are disabled by default.
 // https://rust-lang.github.io/rust-clippy/stable/index.html
 #![warn(clippy::pedantic)]
 
-use indoc::indoc;
+use indoc::{formatdoc, indoc};
 use libcnb_data::buildpack_id;
 use libcnb_test::{
     assert_contains, assert_empty, assert_not_contains, BuildConfig, BuildpackReference,
     ContainerConfig, PackResult, TestRunner,
 };
 use std::path::PathBuf;
 use std::time::Duration;
-use std::{env, fs, thread};
+use std::{env, fs, panic, thread};
 
 const PROCFILE_URL: &str = "heroku/procfile";
 const TEST_PORT: u16 = 12345;
@@ -61,16 +65,16 @@ fn rebuild() {
 #[test]
 #[ignore = "integration test"]
 fn buildpack_packaging_failure() {
-    let err = std::panic::catch_unwind(|| {
+    let err = panic::catch_unwind(|| {
         TestRunner::default().build(BuildConfig::new("invalid!", "tests/fixtures/empty"), |_| {
             unreachable!("The test should panic prior to the TestContext being invoked.");
         });
     })
     .unwrap_err();
 
     assert_eq!(
-        err.downcast_ref::<String>().unwrap().to_string(),
-        format!(
+        err.downcast_ref::<String>().unwrap(),
+        &format!(
             "Could not package directory as buildpack! No `buildpack.toml` file exists at {}",
             env::var("CARGO_MANIFEST_DIR").unwrap()
         )
@@ -136,24 +140,16 @@ fn expected_pack_failure() {
 
 #[test]
 #[ignore = "integration test"]
+#[should_panic(
+    expected = "Could not package directory as buildpack! No `buildpack.toml` file exists at"
+)]
 fn expected_pack_failure_still_panics_for_non_pack_failure() {
-    let err = std::panic::catch_unwind(|| {
-        TestRunner::default().build(
-            BuildConfig::new("invalid!", "tests/fixtures/empty")
-                .expected_pack_result(PackResult::Failure),
-            |_| {
-                unreachable!("The test should panic prior to the TestContext being invoked.");
-            },
-        );
-    })
-    .unwrap_err();
-
-    assert_eq!(
-        err.downcast_ref::<String>().unwrap().to_string(),
-        format!(
-            "Could not package directory as buildpack! No `buildpack.toml` file exists at {}",
-            env::var("CARGO_MANIFEST_DIR").unwrap()
-        )
+    TestRunner::default().build(
+        BuildConfig::new("invalid!", "tests/fixtures/empty")
+            .expected_pack_result(PackResult::Failure),
+        |_| {
+            unreachable!("The test should panic prior to the TestContext being invoked.");
+        },
     );
 }
 
@@ -224,24 +220,29 @@ fn app_dir_absolute_path() {
 
 #[test]
 #[ignore = "integration test"]
-// The full panic message looks like this:
-// `"App dir is not a valid directory: /.../libcnb-test/tests/fixtures/non-existent-fixture"`
-// It's intentionally an absolute path to make debugging failures easier when a relative path has been
-// passed (the common case). However, since the absolute path is system/environment dependent, we would
-// need to either construct the expected string dynamically in `should_panic` (but cannot due to
-// https://github.com/rust-lang/rust/issues/88430), or else use regex (which isn't supported either).
-// As such we test the most important part, the fact that the error message lists the non-existent
-// fixture directory path. We intentionally include the `libcnb-test/` crate directory prefix, since
-// that only appears in the absolute path, not the relative path passed to `BuildConfig::new`.
-#[should_panic(expected = "libcnb-test/tests/fixtures/non-existent-fixture")]
 fn app_dir_invalid_path() {
-    TestRunner::default().build(
-        BuildConfig::new("heroku/builder:22", "tests/fixtures/non-existent-fixture")
-            .buildpacks(Vec::new())
-            .app_dir_preprocessor(|_| {
-                unreachable!("The app dir should be validated before the preprocessor is run.");
-            }),
-        |_| {},
+    let err = panic::catch_unwind(|| {
+        TestRunner::default().build(
+            BuildConfig::new("invalid!", "tests/fixtures/non-existent-fixture")
+                .buildpacks(Vec::new())
+                .app_dir_preprocessor(|_| {
+                    unreachable!("The app dir should be validated before the preprocessor is run.");
+                }),
+            |_| {},
+        );
+    })
+    .unwrap_err();
+
+    assert_eq!(
+        err.downcast_ref::<String>().unwrap(),
+        &format!(
+            "App dir is not a valid directory: {}",
+            env::var("CARGO_MANIFEST_DIR")
+                .map(PathBuf::from)
+                .unwrap()
+                .join("tests/fixtures/non-existent-fixture")
+                .display()
+        )
     );
 }
 
@@ -480,34 +481,49 @@ fn address_for_port_when_port_not_exposed() {
 
 #[test]
 #[ignore = "integration test"]
-#[should_panic(expected = "
-This normally means that the container crashed. Container logs:
-
-## stderr:
-
-some stderr
+fn address_for_port_when_container_crashed() {
+    let mut container_name = String::new();
 
-## stdout:
+    // AssertUnwindSafe is required so that `container_name`` can be mutated across the unwind boundary.
+    let err = panic::catch_unwind(panic::AssertUnwindSafe(|| {
+        TestRunner::default().build(
+            BuildConfig::new("heroku/builder:22", "tests/fixtures/procfile")
+                .buildpacks([BuildpackReference::Other(String::from(PROCFILE_URL))]),
+            |context| {
+                context.start_container(
+                    ContainerConfig::new()
+                        .entrypoint("launcher")
+                        .command(["echo 'some stdout'; echo 'some stderr' >&2; exit 1"])
+                        .expose_port(TEST_PORT),
+                    |container| {
+                        container_name = container.container_name.clone();
+                        // Wait for the container to actually exit, otherwise `address_for_port()` will succeed.
+                        thread::sleep(Duration::from_secs(1));
+                        let _ = container.address_for_port(TEST_PORT);
+                    },
+                );
+            },
+        );
+    }))
+    .unwrap_err();
 
-some stdout
-")]
-fn address_for_port_when_container_crashed() {
-    TestRunner::default().build(
-        BuildConfig::new("heroku/builder:22", "tests/fixtures/procfile")
-            .buildpacks([BuildpackReference::Other(String::from(PROCFILE_URL))]),
-        |context| {
-            context.start_container(
-                ContainerConfig::new()
-                    .entrypoint("launcher")
-                    .command(["echo 'some stdout'; echo 'some stderr' >&2; exit 1"])
-                    .expose_port(TEST_PORT),
-                |container| {
-                    // Wait for the container to actually exit, otherwise `address_for_port()` will succeed.
-                    thread::sleep(Duration::from_secs(1));
-                    let _ = container.address_for_port(TEST_PORT);
-                },
-            );
-        },
+    assert_eq!(
+        err.downcast_ref::<String>().unwrap(),
+        &formatdoc! {"
+            Error obtaining container port mapping:
+            Error: No public port '12345' published for {container_name}
+
+            This normally means that the container crashed. Container logs:
+            
+            ## stderr:
+            
+            some stderr
+            
+            ## stdout:
+            
+            some stdout
+            
+        "}
     );
 }