Remove sleeps from tests. (#213)

* Remove sleeps.
* Increase timeouts.

Author: wmedrano

.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" }

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.

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();
     }
 }

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));
-    }
-}

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);
 }

src/tests/mod.rs

@@ -1,10 +1,20 @@
+use std::sync::LazyLock;
+
+use crate::{Client, ClientOptions};
+
 mod client;
 mod log;
 mod processing;
 mod ringbuffer;
 mod time;
 mod transport;
 
+pub static DEFAULT_TEST_CLIENT: LazyLock<Client> = LazyLock::new(|| {
+    Client::new("default-test-client", ClientOptions::default())
+        .unwrap()
+        .0
+});
+
 #[ctor::ctor]
 fn log_to_stdio() {
     crate::set_logger(crate::LoggerType::Stdio);

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");
 }

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
     );
 }