Merge pull request #97 from michel/test/event-synced-e2e

test: wait on events instead of timers in the flaky tests

soulseek-rs/src/port_mapping/mod.rs

@@ -418,7 +418,18 @@ mod tests {
         thread::spawn(move || {
             let mut buf = [0u8; 64];
             // Answer many requests (initial map + cleanup unmap + any renews).
-            while let Ok((n, from)) = socket.recv_from(&mut buf) {
+            loop {
+                let (n, from) = match socket.recv_from(&mut buf) {
+                    Ok(received) => received,
+                    // Windows reports a reply that met an already-closed
+                    // client socket on the next receive; the gateway is fine.
+                    Err(e)
+                        if e.kind() == std::io::ErrorKind::ConnectionReset =>
+                    {
+                        continue;
+                    }
+                    Err(_) => break,
+                };
                 let request = buf[..n].to_vec();
                 let internal = u16::from_be_bytes([request[4], request[5]]);
                 let lifetime = u32::from_be_bytes([
@@ -465,20 +476,21 @@ mod tests {
         );
 
         // Dropping the mapper must remove the mapping (a lifetime-0 request).
+        // A map the gateway was slow to answer is sent again, and that resend
+        // can arrive first.
         drop(mapper);
-        let unmap_req = requests
-            .recv_timeout(StdDuration::from_secs(3))
-            .expect("dropping the mapper should send an unmap request");
-        assert_eq!(
+        let unmapped = std::iter::from_fn(|| {
+            requests.recv_timeout(StdDuration::from_secs(3)).ok()
+        })
+        .any(|request| {
             u32::from_be_bytes([
-                unmap_req[8],
-                unmap_req[9],
-                unmap_req[10],
-                unmap_req[11]
-            ]),
-            0,
-            "cleanup should request lifetime 0"
-        );
+                request[8],
+                request[9],
+                request[10],
+                request[11],
+            ]) == 0
+        });
+        assert!(unmapped, "cleanup should request lifetime 0");
     }
 
     /// A minimal in-process UPnP IGD: an HTTP server that answers every SOAP

soulseek-rs/tests/cli_e2e.rs

@@ -884,21 +884,28 @@ fn split_session_flags(args: &[&str]) -> (Vec<String>, Vec<String>) {
 ///
 /// `--no-config` is dropped for the obvious reason, and `--quiet` with it:
 /// these commands report the schedule the *server* chose on stderr, which is
-/// the only place a test can see it.
+/// the only place a test can see it. Session flags go to the daemon, as in
+/// [`cli`].
 fn cli_with_config(
     server: &TestServer,
     user: &str,
     config: &Path,
     args: &[&str],
 ) -> Output {
+    let (session_flags, command_args) = match mode() {
+        Mode::Local => {
+            (Vec::new(), args.iter().map(|a| (*a).to_string()).collect())
+        }
+        Mode::Daemon => split_session_flags(args),
+    };
     let mut all: Vec<String> = server
-        .args(user)
+        .args_with(user, &session_flags)
         .into_iter()
         .filter(|arg| arg != "--no-config" && arg != "--quiet")
         .collect();
     all.push("--config".to_string());
     all.push(config.to_str().expect("utf-8 path").to_string());
-    all.extend(args.iter().map(|a| (*a).to_string()));
+    all.extend(command_args);
     let refs: Vec<&str> = all.iter().map(String::as_str).collect();
     run(&refs)
 }
@@ -1888,8 +1895,19 @@ fn room_listen_streams_what_is_said_in_the_room() {
         .spawn()
         .expect("the binary should run");
 
-    // Give the listener time to join before saying anything.
-    std::thread::sleep(Duration::from_secs(3));
+    // The room announces the listener once the server has it in; a line
+    // said before that never reaches it.
+    let deadline = Instant::now() + Duration::from_secs(30);
+    while !speaker.take_room_events().iter().any(|event| {
+        matches!(
+            event,
+            soulseek_rs::types::RoomEvent::UserJoined { room, username }
+                if room == "cli_e2e_lobby_b" && username == "cli_e2e_room_reader"
+        )
+    }) {
+        assert!(Instant::now() < deadline, "the listener never joined");
+        std::thread::sleep(Duration::from_millis(50));
+    }
     speaker
         .say_in_room("cli_e2e_lobby_b", "streamed line")
         .expect("say");
@@ -1958,6 +1976,11 @@ fn a_private_message_reaches_its_recipient() {
 fn message_read_streams_incoming_private_messages() {
     let server = server_or_skip!();
     let sender = server.client("cli_e2e_pm_sender", Vec::new());
+    // soulfind drops a message to a name it has never seen but keeps one for
+    // a known user until they log in, so however long the reader takes to
+    // get online, the line below reaches it. Registered before the reader's
+    // own session exists: a later login as them would take its place.
+    drop(server.client("cli_e2e_pm_reader", Vec::new()));
     settle();
 
     let mut args = server.args("cli_e2e_pm_reader");

soulseek-rs/tests/common/mod.rs

@@ -60,12 +60,16 @@ pub struct Soulfind {
 }
 
 impl Soulfind {
+    /// soulfind exits if its port is taken between `free_port()` handing it
+    /// out and soulfind binding it, so a start that fails gets a fresh port.
     pub fn start() -> Option<Self> {
-        let port = free_port()?;
-        let db = std::env::temp_dir().join(format!("soulfind-{port}.db"));
-        let _ = std::fs::remove_file(&db);
-        let child = Self::spawn(port, &db)?;
-        Some(Self { child, db, port })
+        (0..3).find_map(|_| {
+            let port = free_port()?;
+            let db = std::env::temp_dir().join(format!("soulfind-{port}.db"));
+            let _ = std::fs::remove_file(&db);
+            let child = Self::spawn(port, &db)?;
+            Some(Self { child, db, port })
+        })
     }
 
     fn spawn(port: u16, db: &Path) -> Option<Child> {
@@ -78,8 +82,8 @@ impl Soulfind {
 
         let deadline = Instant::now() + Duration::from_secs(10);
         while Instant::now() < deadline {
-            // Whoever answers on the port has to be this child: one that
-            // lost the port and died would otherwise pass as up.
+            // A child that has exited is never taken as up: soulfind exits
+            // when its port is gone, and whoever took it may answer instead.
             if child.try_wait().ok().flatten().is_some() {
                 return None;
             }