From 9531fbc1dc9b9c6666d3023e31aff9a1f02f9d91 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 7 Mar 2023 12:03:44 -0500 Subject: [PATCH 1/4] chore: avoid fixed sleep in integration tests. Prior to this commit the `tests/tests.rs` integration tests had an unconditional 1 second sleep waiting for a server to become available. This is slightly problematic: * Waiting a full second if the server becomes available sooner is unnecessarily slow. * Conversely, we can't be sure the server is available after the 1s sleep. This commit replaces the sleeps with a loop of up to 10 iterations that tries to connect to the server port. If any iteration succeeds we return immediately to avoid unnecessary waiting. If an iteration fails, we wait longer, increasing the duration each time we wait (up to a maximum of 10 iteration). --- tests/tests.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/tests.rs b/tests/tests.rs index f08b7cb..3d23783 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1,4 +1,5 @@ use std::env; +use std::net::TcpStream; use std::path::PathBuf; use std::process::Command; use std::thread; @@ -21,6 +22,16 @@ fn client_command() -> Command { Command::new(examples_dir().join("client")) } +fn wait_for_server(addr: &str) { + for i in 0..10 { + if let Ok(_) = TcpStream::connect(addr) { + return; + } + thread::sleep(time::Duration::from_millis(i * 100)); + } + panic!("failed to connect to {:?} after 10 tries", addr); +} + #[test] fn client() { let rc = client_command() @@ -38,13 +49,14 @@ fn server() { .spawn() .expect("cannot run server example"); - thread::sleep(time::Duration::from_secs(1)); + let addr = "localhost:1337"; + wait_for_server(addr); let output = Command::new("curl") .arg("--insecure") .arg("--http1.0") .arg("--silent") - .arg("https://localhost:1337") + .arg(format!("https://{}", addr)) .output() .expect("cannot run curl"); @@ -61,10 +73,11 @@ fn custom_ca_store() { .spawn() .expect("cannot run server example"); - thread::sleep(time::Duration::from_secs(1)); + let addr = "localhost:1338"; + wait_for_server(addr); let rc = client_command() - .arg("https://localhost:1338") + .arg(format!("https://{}", addr)) .arg("examples/sample.pem") .output() .expect("cannot run client example"); From 2b7da581f58e4724e31630b58fea739cc9e5fc4e Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 7 Mar 2023 12:12:24 -0500 Subject: [PATCH 2/4] fix: kill server before asserting on output. Prior to this commit the `server()` test would call `assert_eq!` to assert on client output matching expected _before_ calling `srv.kill()` on the `Child` spawned by `server_command()`. The upstream rustdocs for `std.process.Child` mention: > There is no implementation of Drop for child processes, so if you do > not ensure the Child has exited then it will continue to run, even > after the Child handle to the child process has gone out of scope. The net result is that if the assertion in `server()` fails, we leave a running server instance behind. This manifests as a port conflict when re-running the test without first killing the orphan. This commit moves the `srv.kill()` call to before the `assert_eq`. This avoids leaking the server process if the assertion fails. --- tests/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tests.rs b/tests/tests.rs index 3d23783..dbee5bc 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -60,10 +60,10 @@ fn server() { .output() .expect("cannot run curl"); + srv.kill().unwrap(); + println!("client output: {:?}", output.stdout); assert_eq!(output.stdout, b"Try POST /echo\n"); - - srv.kill().unwrap(); } #[test] From fd8a11152e82ea55aa61a92fd06e2cbae54a3f50 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 7 Mar 2023 12:59:28 -0500 Subject: [PATCH 3/4] chore: improve server integration test output. This commit adds a few quality of life improvements to the server integration test: 1. The `--silent` curl arg is removed so that we can get diagnostic info from curl on stderr in the event of a failure. Our test only asserts on stdout content, which remains unaffected by this change. 2. If the `curl` command invocation status isn't success, we print the stderr content enabled by not using `--silent`. This will provide more diagnostic context to investigate the failure. 3. The `assert_eq` is changed to assert on a string value created from stdout instead of raw bytes. This is easier for humans to diff in the test output compared to a slice of byte values. Combined these changes should make it easier to diagnose future test failures without needing to iterate in CI. --- tests/tests.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/tests.rs b/tests/tests.rs index dbee5bc..4cc2ce2 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -55,15 +55,17 @@ fn server() { let output = Command::new("curl") .arg("--insecure") .arg("--http1.0") - .arg("--silent") .arg(format!("https://{}", addr)) .output() .expect("cannot run curl"); srv.kill().unwrap(); - println!("client output: {:?}", output.stdout); - assert_eq!(output.stdout, b"Try POST /echo\n"); + if !output.status.success() { + println!("curl stderr:\n{}", String::from_utf8_lossy(&output.stderr)); + } + + assert_eq!(String::from_utf8_lossy(&*output.stdout), "Try POST /echo\n"); } #[test] From ffa7677285ff1758f31e6dbc04118b0bde9d43f7 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 7 Mar 2023 12:28:52 -0500 Subject: [PATCH 4/4] chore: output curl version in server test failure. In the event the `server()` integration test fails it's helpful to know the version of `curl` that was used. This commit includes this information when the `curl` client invocation returns a non-zero status code. --- tests/tests.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/tests.rs b/tests/tests.rs index 4cc2ce2..aa39880 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -62,6 +62,12 @@ fn server() { srv.kill().unwrap(); if !output.status.success() { + let version_stdout = Command::new("curl") + .arg("--version") + .output() + .expect("cannot run curl to collect --version") + .stdout; + println!("curl version: {}", String::from_utf8_lossy(&version_stdout)); println!("curl stderr:\n{}", String::from_utf8_lossy(&output.stderr)); }