libcnb-test: Improve error messages for address_for_port (#636)

Improves the debugging UX for several `ContainerContext::address_for_port` failure modes. In particular, if containers crash after startup then the container logs are now included in the panic error message, rather than only the potentially confusing "No public port X published" message from Docker. Fixes #482. GUS-W-13966399.
heroku · Aug 17, 2023 · e99e3aa · e99e3aa
1 parent 74708c6
commit e99e3aa
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,12 +9,14 @@ separate changelogs for each crate were used. If you need to refer to these old
 ### Added
 
 - `libcnb-package`: Add cross-compilation assistance for Linux `aarch64-unknown-linux-musl`. ([#577](https://github.com/heroku/libcnb.rs/pull/577))
-- `libcnb-test`: `LogOutput` now implements `std::fmt::Display`. ([#635](https://github.com/heroku/libcnb.rs/pull/635))
+- `libcnb-test`:
+  - `LogOutput` now implements `std::fmt::Display`. ([#635](https://github.com/heroku/libcnb.rs/pull/635))
+  - `ContainerConfig` now implements `Clone`. ([#636](https://github.com/heroku/libcnb.rs/pull/636))
 
 ### Changed
 
 - `libcnb-test`:
-  - `ContainerContext::address_for_port` now returns `SocketAddr` directly instead of `Option<SocketAddr>`. ([#605](https://github.com/heroku/libcnb.rs/pull/605))
+  - `ContainerContext::address_for_port` will now panic for all failure modes rather than just some, and so now returns `SocketAddr` directly instead of `Option<SocketAddr>`. This reduces test boilerplate due to the caller no longer needing to `.unwrap()` and improves debugging UX when containers crash after startup. ([#605](https://github.com/heroku/libcnb.rs/pull/605) and [#636](https://github.com/heroku/libcnb.rs/pull/636))
   - Docker commands are now run using the Docker CLI instead of Bollard and the Docker daemon API. ([#620](https://github.com/heroku/libcnb.rs/pull/620))
   - `ContainerConfig::entrypoint` now accepts a String rather than a vector of strings. Any arguments to the entrypoint should be moved to `ContainerConfig::command`. ([#620](https://github.com/heroku/libcnb.rs/pull/620))
   - `TestRunner::new` has been removed, since its only purpose was for advanced configuration that's no longer applicable. Use `TestRunner::default` instead. ([#620](https://github.com/heroku/libcnb.rs/pull/620))

diff --git a/libcnb-test/src/container_config.rs b/libcnb-test/src/container_config.rs
@@ -25,7 +25,7 @@ use std::collections::{HashMap, HashSet};
 ///     },
 /// );
 /// ```
-#[derive(Default)]
+#[derive(Clone, Default)]
 pub struct ContainerConfig {
     pub(crate) entrypoint: Option<String>,
     pub(crate) command: Option<Vec<String>>,

diff --git a/libcnb-test/src/container_context.rs b/libcnb-test/src/container_context.rs
@@ -2,13 +2,15 @@ use crate::docker::{
     DockerExecCommand, DockerLogsCommand, DockerPortCommand, DockerRemoveContainerCommand,
 };
 use crate::log::LogOutput;
-use crate::util;
+use crate::util::CommandError;
+use crate::{util, ContainerConfig};
 use std::net::SocketAddr;
 
 /// Context of a launched container.
 pub struct ContainerContext {
     /// The randomly generated name of this container.
     pub container_name: String,
+    pub(crate) config: ContainerConfig,
 }
 
 impl ContainerContext {
@@ -111,15 +113,30 @@ impl ContainerContext {
     /// was not exposed using [`ContainerConfig::expose_port`](crate::ContainerConfig::expose_port).
     #[must_use]
     pub fn address_for_port(&self, port: u16) -> SocketAddr {
+        assert!(
+            self.config.exposed_ports.contains(&port),
+            "Unknown port: Port {port} needs to be exposed first using `ContainerConfig::expose_port`"
+        );
+
         let docker_port_command = DockerPortCommand::new(&self.container_name, port);
-        util::run_command(docker_port_command)
-            .unwrap_or_else(|command_err| {
-                panic!("Error obtaining container port mapping:\n\n{command_err}")
-            })
-            .stdout
-            .trim()
-            .parse()
-            .expect("Couldn't parse `docker port` output")
+
+        match util::run_command(docker_port_command) {
+            Ok(output) => output
+                .stdout
+                .trim()
+                .parse()
+                .expect("Couldn't parse `docker port` output"),
+            Err(CommandError::NonZeroExitCode { log_output, .. }) => {
+                panic!(
+                    "Error obtaining container port mapping:\n{}\nThis normally means that the container crashed. Container logs:\n\n{}",
+                    log_output.stderr,
+                    self.logs_now()
+                );
+            }
+            Err(command_err) => {
+                panic!("Error obtaining container port mapping:\n\n{command_err}");
+            }
+        }
     }
 
     /// Executes a shell command inside an already running container.

diff --git a/libcnb-test/src/test_context.rs b/libcnb-test/src/test_context.rs
@@ -109,7 +109,10 @@ impl<'a> TestContext<'a> {
         util::run_command(docker_run_command)
             .unwrap_or_else(|command_err| panic!("Error starting container:\n\n{command_err}"));
 
-        f(ContainerContext { container_name });
+        f(ContainerContext {
+            container_name,
+            config: config.clone(),
+        });
     }
 
     /// Run the provided shell command.

diff --git a/libcnb-test/tests/integration_test.rs b/libcnb-test/tests/integration_test.rs
@@ -475,13 +475,9 @@ fn expose_port_invalid_port() {
 
 #[test]
 #[ignore = "integration test"]
-#[should_panic(expected = "Error obtaining container port mapping:
-
-docker command failed with exit code 1!
-
-## stderr:
-
-Error: No public port '12345")]
+#[should_panic(
+    expected = "Unknown port: Port 12345 needs to be exposed first using `ContainerConfig::expose_port`"
+)]
 fn address_for_port_when_port_not_exposed() {
     TestRunner::default().build(
         BuildConfig::new("heroku/builder:22", "test-fixtures/procfile")
@@ -496,14 +492,17 @@ fn address_for_port_when_port_not_exposed() {
 
 #[test]
 #[ignore = "integration test"]
-// TODO: Improve the UX here: https://github.com/heroku/libcnb.rs/issues/482
-#[should_panic(expected = "Error obtaining container port mapping:
-
-docker command failed with exit code 1!
+#[should_panic(expected = "
+This normally means that the container crashed. Container logs:
 
 ## stderr:
 
-Error: No public port '12345")]
+some stderr
+
+## stdout:
+
+some stdout
+")]
 fn address_for_port_when_container_crashed() {
     TestRunner::default().build(
         BuildConfig::new("heroku/builder:22", "test-fixtures/procfile")
@@ -512,11 +511,12 @@ fn address_for_port_when_container_crashed() {
             context.start_container(
                 ContainerConfig::new()
                     .entrypoint("launcher")
-                    .command(["exit 1"])
+                    .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(12345);
+                    let _ = container.address_for_port(TEST_PORT);
                 },
             );
         },