From 488a12a57c4ea8163d5b7a00cf78e25906d3f51f Mon Sep 17 00:00:00 2001 From: Will Date: Sat, 14 Sep 2024 19:55:11 -0700 Subject: [PATCH] Remove sleeps from tests. (#213) * Remove sleeps. * Increase timeouts. --- .config/nextest.toml | 2 +- README.md | 4 +--- src/client/client_impl.rs | 15 +++++--------- src/client/common.rs | 9 --------- src/tests/client.rs | 41 ++++++++++++++++++++------------------- src/tests/mod.rs | 10 ++++++++++ src/tests/processing.rs | 8 +++++++- src/tests/time.rs | 6 +++--- 8 files changed, 48 insertions(+), 47 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index f641fab8..5d2bdd6d 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -1,5 +1,5 @@ [profile.default] test-threads = 1 fail-fast = false -slow-timeout = { period = "2s", terminate-after = 2 } +slow-timeout = { period = "10s", terminate-after = 1 } retries = { backoff = "fixed", count = 3, delay = "1s" } diff --git a/README.md b/README.md index 776acd75..9d54d106 100644 --- a/README.md +++ b/README.md @@ -48,9 +48,7 @@ run in single threaded mode. If the tests are failing, a possible gotcha may be timing issues. 1. If using `cargo test`, try `cargo nextest`. The `cargo nextest` - configuration is set up to run single threaded and to retry flaky tests up - to 3 times. -1. Increase the value used by `sleep_on_test` in `client/common.rs`. + configuration is set up to run single threaded and to retry flaky tests. Another case is that libjack may be broken on your setup. Try using libjack2 or pipewire-jack. diff --git a/src/client/client_impl.rs b/src/client/client_impl.rs index 4490ea3a..1ac73401 100644 --- a/src/client/client_impl.rs +++ b/src/client/client_impl.rs @@ -3,7 +3,7 @@ use std::fmt::Debug; use std::sync::Arc; use std::{ffi, fmt, ptr}; -use crate::client::common::{sleep_on_test, CREATE_OR_DESTROY_CLIENT_MUTEX}; +use crate::client::common::CREATE_OR_DESTROY_CLIENT_MUTEX; use crate::jack_enums::CodeOrMessage; use crate::jack_utils::collect_strs; use crate::properties::PropertyChangeHandler; @@ -61,18 +61,15 @@ impl Client { } crate::logging::maybe_init_logging(); - sleep_on_test(); let mut status_bits = 0; let client = unsafe { let client_name = ffi::CString::new(client_name).unwrap(); j::jack_client_open(client_name.as_ptr(), options.bits(), &mut status_bits) }; - sleep_on_test(); let status = ClientStatus::from_bits(status_bits).unwrap_or_else(ClientStatus::empty); if client.is_null() { Err(Error::ClientError(status)) } else { - sleep_on_test(); Ok((Client(client, Arc::default(), None), status)) } } @@ -673,12 +670,10 @@ impl Client { impl Drop for Client { fn drop(&mut self) { let _m = CREATE_OR_DESTROY_CLIENT_MUTEX.lock().ok(); - debug_assert!(!self.raw().is_null()); // Rep invariant - // Close the client - sleep_on_test(); - let _res = unsafe { j::jack_client_close(self.raw()) }; // best effort: close the client - sleep_on_test(); - //assert_eq!(res, 0); //do not assert here. connection could be broken + // Rep invariant. + debug_assert!(!self.raw().is_null()); + // Best effort close client. + let _res = unsafe { j::jack_client_close(self.raw()) }; self.0 = ptr::null_mut(); } } diff --git a/src/client/common.rs b/src/client/common.rs index aab1b9e8..5ed6c579 100644 --- a/src/client/common.rs +++ b/src/client/common.rs @@ -18,12 +18,3 @@ lazy_static! { lazy_static! { pub static ref CREATE_OR_DESTROY_CLIENT_MUTEX: Mutex<()> = Mutex::new(()); } - -#[inline(always)] -pub fn sleep_on_test() { - #[cfg(test)] - { - use std::{thread, time}; - thread::sleep(time::Duration::from_millis(100)); - } -} diff --git a/src/tests/client.rs b/src/tests/client.rs index 356fa1d9..788d2e19 100644 --- a/src/tests/client.rs +++ b/src/tests/client.rs @@ -1,3 +1,5 @@ +use crate::tests::DEFAULT_TEST_CLIENT; + #[test] fn client_can_open() { let (client, status) = @@ -7,34 +9,34 @@ fn client_can_open() { assert_ne!(client.sample_rate(), 0); assert_ne!(client.buffer_size(), 0); assert_ne!(client.uuid_string(), ""); + assert_ne!(client.uuid(), 0); let cpu_load = client.cpu_load(); assert!(cpu_load > 0.0, "client.cpu_load() = {}", cpu_load); } #[test] fn time_is_montonically_increasing() { - let (client, _) = crate::Client::new("", crate::ClientOptions::empty()).unwrap(); - - let t0 = client.time(); - let frames0 = client.frames_since_cycle_start(); - let frame_time0 = client.frame_time(); + let t0 = DEFAULT_TEST_CLIENT.time(); + let frames0 = DEFAULT_TEST_CLIENT.frames_since_cycle_start(); + let frame_time0 = DEFAULT_TEST_CLIENT.frame_time(); std::thread::sleep(std::time::Duration::from_millis(50)); - assert_ne!(client.time(), t0); - assert_ne!(client.frames_since_cycle_start(), frames0); - assert_ne!(client.frame_time(), frame_time0); + assert_ne!(DEFAULT_TEST_CLIENT.time(), t0); + assert_ne!(DEFAULT_TEST_CLIENT.frames_since_cycle_start(), frames0); + assert_ne!(DEFAULT_TEST_CLIENT.frame_time(), frame_time0); } #[test] fn maybe_client_can_set_buffer_size() { - let (client, _) = crate::Client::new("", crate::ClientOptions::empty()).unwrap(); - let initial_buffer_size = client.buffer_size(); - if let Err(err) = client.set_buffer_size(initial_buffer_size * 2) { + let initial_buffer_size = DEFAULT_TEST_CLIENT.buffer_size(); + if let Err(err) = DEFAULT_TEST_CLIENT.set_buffer_size(initial_buffer_size * 2) { eprintln!("client does not support setting buffer size: {err}"); return; } - assert_eq!(client.buffer_size(), 2 * initial_buffer_size); - client.set_buffer_size(initial_buffer_size).unwrap(); + assert_eq!(DEFAULT_TEST_CLIENT.buffer_size(), 2 * initial_buffer_size); + DEFAULT_TEST_CLIENT + .set_buffer_size(initial_buffer_size) + .unwrap(); } #[test] @@ -76,12 +78,11 @@ fn uuid_can_map_to_client_name() { #[test] fn nonexistant_uuid_to_client_name_returns_none() { - let (client1, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap(); - let (client2, _) = + let (client, _) = crate::Client::new("dropped-client", crate::ClientOptions::default()).unwrap(); - let uuid_string = client2.uuid_string(); - let uuid = client2.uuid(); - drop(client2); - assert_eq!(client1.name_by_uuid_str(&uuid_string), None); - assert_eq!(client1.name_by_uuid(uuid), None); + let uuid_string = client.uuid_string(); + let uuid = client.uuid(); + drop(client); + assert_eq!(DEFAULT_TEST_CLIENT.name_by_uuid_str(&uuid_string), None); + assert_eq!(DEFAULT_TEST_CLIENT.name_by_uuid(uuid), None); } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 194292fe..00432073 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,3 +1,7 @@ +use std::sync::LazyLock; + +use crate::{Client, ClientOptions}; + mod client; mod log; mod processing; @@ -5,6 +9,12 @@ mod ringbuffer; mod time; mod transport; +pub static DEFAULT_TEST_CLIENT: LazyLock = LazyLock::new(|| { + Client::new("default-test-client", ClientOptions::default()) + .unwrap() + .0 +}); + #[ctor::ctor] fn log_to_stdio() { crate::set_logger(crate::LoggerType::Stdio); diff --git a/src/tests/processing.rs b/src/tests/processing.rs index a964e2e4..69b016e4 100644 --- a/src/tests/processing.rs +++ b/src/tests/processing.rs @@ -34,16 +34,20 @@ fn panic_in_buffer_size_handler_propagates_as_error_in_deactivate() { #[test] fn quitting_stops_calling_process() { + eprintln!("Creating client."); let (client, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap(); let mut calls = 0; let (send, recv) = std::sync::mpsc::sync_channel(2); + eprintln!("Creating callback."); let process_handler = crate::contrib::ClosureProcessHandler::new(move |_, _| { send.try_send(true).unwrap(); calls += 1; assert_eq!(calls, 1); crate::Control::Quit }); + eprintln!("Activating client."); let ac = client.activate_async((), process_handler).unwrap(); + eprintln!("Waiting for async response."); assert!(recv .recv_timeout(std::time::Duration::from_secs(1)) .unwrap()); @@ -85,12 +89,14 @@ fn buffer_size_is_called_before_process() { |state, _, _| { assert_eq!(*state, "initializing"); *state = "processing"; + // Give the processing thread some time to run, in case it wants to. + std::thread::sleep(std::time::Duration::from_secs(3)); crate::Control::Continue }, ); let ac = client.activate_async((), process_handler).unwrap(); assert!(recv - .recv_timeout(std::time::Duration::from_secs(1)) + .recv_timeout(std::time::Duration::from_secs(5)) .unwrap()); assert_eq!(ac.deactivate().unwrap().2.state, "processing"); } diff --git a/src/tests/time.rs b/src/tests/time.rs index 4987a3bf..6632d23a 100644 --- a/src/tests/time.rs +++ b/src/tests/time.rs @@ -2,7 +2,7 @@ use approx::assert_abs_diff_eq; #[test] fn frame_and_time_are_convertable() { - let (client, _) = crate::Client::new("", crate::ClientOptions::empty()).unwrap(); + let (client, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap(); assert_eq!(client.time_to_frames(client.frames_to_time(0)), 0); } @@ -13,7 +13,7 @@ fn one_frame_duration_is_inverse_of_sample_rate() { assert_abs_diff_eq!( (client.frames_to_time(sample_rate as _) - client.frames_to_time(0)) as f64, 1_000_000.0, - epsilon = 1_000_000.0 * 1e-4, + epsilon = 1_000_000.0 * 1e-3, ); } @@ -25,6 +25,6 @@ fn one_second_is_sample_rate_frames() { assert_abs_diff_eq!( (t1 - t0) as f64, client.sample_rate() as f64, - epsilon = client.sample_rate() as f64 * 1e-4 + epsilon = client.sample_rate() as f64 * 1e-3 ); }