From b8ff73ef9b994995dd3d2cc358ea3aa22fbac087 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Fri, 21 Jul 2023 10:31:31 +0100 Subject: [PATCH] Improve the ergonomics of `ContainerContext::address_for_port` Previously `ContainerContext::address_for_port` would panic for some failure modes, but return an `Option` for others. Given that: - the function is only used in tests - `libcnb-test` already leans heavily into the "directly panic rather than make the tests call unwrap/expect everywhere" approach - the current `Option` usage is for when there is a bug in the test (ie: the test author forgot to call `ContainerConfig::expose_port`) ...then it makes sense to panic instead of returning `None`. This avoids the `.unwrap()` boilerplate on the callers side, and also simplifies a future PR to switch from Bollard to using the Docker CLI (this change is being split out to reduce the size of that PR). The `expect()` message doesn't conform to the recommended "expect as precondition" style since the other `expect()`s in the function do not either. (We can always revisit this as part of the Bollard migration.) --- CHANGELOG.md | 1 + examples/ruby-sample/tests/integration_test.rs | 2 +- libcnb-test/README.md | 2 +- libcnb-test/src/container_config.rs | 2 +- libcnb-test/src/container_context.rs | 10 ++++++++-- libcnb-test/tests/integration_test.rs | 2 +- 6 files changed, 13 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5108b4d2..33003430 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ separate changelogs for each crate were used. If you need to refer to these old ### Changed +- `libcnb-test`: `ContainerContext::address_for_port` now returns `SocketAddr` directly instead of `Option`. ([#605](https://github.com/heroku/libcnb.rs/pull/605)) - `libcnb-package`: buildpack target directory now contains the target triple. Users that implicitly rely on the output directory need to adapt. The output of `cargo libcnb package` will refer to the new locations. ([#580](https://github.com/heroku/libcnb.rs/pull/580)) - `libherokubuildpack`: Switch the `flate2` decompression backend from `miniz_oxide` to `zlib`. ([#593](https://github.com/heroku/libcnb.rs/pull/593)) diff --git a/examples/ruby-sample/tests/integration_test.rs b/examples/ruby-sample/tests/integration_test.rs index 2aa46ce1..fe19dba4 100644 --- a/examples/ruby-sample/tests/integration_test.rs +++ b/examples/ruby-sample/tests/integration_test.rs @@ -35,7 +35,7 @@ fn basic() { assert_eq!( call_test_fixture_service( - container.address_for_port(TEST_PORT).unwrap(), + container.address_for_port(TEST_PORT), "Hello World!" ) .unwrap(), diff --git a/libcnb-test/README.md b/libcnb-test/README.md index 09b6c5cd..8f8d0e28 100644 --- a/libcnb-test/README.md +++ b/libcnb-test/README.md @@ -123,7 +123,7 @@ fn starting_web_server_container() { .env("PORT", TEST_PORT.to_string()) .expose_port(TEST_PORT), |container| { - let address_on_host = container.address_for_port(TEST_PORT).unwrap(); + let address_on_host = container.address_for_port(TEST_PORT); let url = format!("http://{}:{}", address_on_host.ip(), address_on_host.port()); // Give the server time to start. diff --git a/libcnb-test/src/container_config.rs b/libcnb-test/src/container_config.rs index 0a911369..26031fcc 100644 --- a/libcnb-test/src/container_config.rs +++ b/libcnb-test/src/container_config.rs @@ -128,7 +128,7 @@ impl ContainerConfig { /// context.start_container( /// ContainerConfig::new().env("PORT", "12345").expose_port(12345), /// |container| { - /// let address_on_host = container.address_for_port(12345).unwrap(); + /// let address_on_host = container.address_for_port(12345); /// // ... /// }, /// ); diff --git a/libcnb-test/src/container_context.rs b/libcnb-test/src/container_context.rs index 9bd4ba2e..0ecb0e8f 100644 --- a/libcnb-test/src/container_context.rs +++ b/libcnb-test/src/container_context.rs @@ -113,15 +113,20 @@ impl<'a> ContainerContext<'a> { /// context.start_container( /// ContainerConfig::new().env("PORT", "12345").expose_port(12345), /// |container| { - /// let address_on_host = container.address_for_port(12345).unwrap(); + /// let address_on_host = container.address_for_port(12345); /// // ... /// }, /// ); /// }, /// ); /// ``` + /// + /// # Panics + /// + /// Will panic if there was an error obtaining the container port mapping, or the specified port + /// was not exposed using [`ContainerConfig::expose_port`](crate::ContainerConfig::expose_port). #[must_use] - pub fn address_for_port(&self, port: u16) -> Option { + pub fn address_for_port(&self, port: u16) -> SocketAddr { self.test_context.runner.tokio_runtime.block_on(async { self.test_context .runner @@ -137,6 +142,7 @@ impl<'a> ContainerContext<'a> { .get(&port) .copied() }) + .expect("Could not find specified port in container port mapping") }) } diff --git a/libcnb-test/tests/integration_test.rs b/libcnb-test/tests/integration_test.rs index beffc9c7..daac0e20 100644 --- a/libcnb-test/tests/integration_test.rs +++ b/libcnb-test/tests/integration_test.rs @@ -68,7 +68,7 @@ fn starting_containers() { .env("PORT", TEST_PORT.to_string()) .expose_port(TEST_PORT), |container| { - let address_on_host = container.address_for_port(TEST_PORT).unwrap(); + let address_on_host = container.address_for_port(TEST_PORT); let url = format!("http://{}:{}", address_on_host.ip(), address_on_host.port()); // Retries needed since the server takes a moment to start up.