diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f8913f0..bcfc68c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,8 +111,7 @@ jobs: run: cargo test -p soulseek-rs --test cli_e2e -- --nocapture # The window over a real session: keys in, a screen out, a peer on the - # other end. The unit tests drive it against a stand-in session and the - # CLI suite never opens it, so this is the only place the two meet. + # other end. Nothing else opens the TUI against a server. - name: Run TUI e2e tests (required against soulfind) env: SOULSEEK_E2E_REQUIRED: "1" diff --git a/README.md b/README.md index fca9053..c8d031d 100644 --- a/README.md +++ b/README.md @@ -621,19 +621,19 @@ cargo fmt ### End-to-end tests -Three suites run against [soulfind](https://github.com/soulfind-dev/soulfind), +Four suites run against [soulfind](https://github.com/soulfind-dev/soulfind), a local Soulseek server: `soulseek-rs-lib/tests/e2e.rs` covers the protocol library, `soulseek-rs/tests/cli_e2e.rs` drives the binary the way a script -would, and `soulseek-rs/tests/tui_e2e.rs` drives the window with key presses -and reads the screen back. All are **server-optional**, running when a server -is available and skipping otherwise, so `cargo test` stays green everywhere. +would, `soulseek-rs/tests/daemon_protocol_e2e.rs` meets the daemon's control +protocol as a third party would, and `soulseek-rs/tests/tui_e2e.rs` drives the +window with key presses and reads the screen back. All are +**server-optional**, running when a server is available and skipping +otherwise, so `cargo test` stays green everywhere. -They locate a server in this order: - -1. `SOULSEEK_TEST_SERVER=host:port`: connect to an already-running server, or -2. `SOULFIND_BIN=/path/to/soulfind` (or a `soulfind/bin/soulfind` checkout in a - parent directory), which spawns soulfind on an ephemeral port with a - throwaway database. +Every suite spawns soulfind from `SOULFIND_BIN=/path/to/soulfind` (or a +`soulfind/bin/soulfind` checkout in a parent directory) on an ephemeral port +with a throwaway database. The library and CLI suites take +`SOULSEEK_TEST_SERVER=host:port` first, to run against a server already up. ```bash # Build soulfind once (see its BUILDING.md), then: @@ -659,8 +659,8 @@ soulfind instead of skipping. `cargo clippy --workspace --all-targets -- -D warnings`. - **Test**: `cargo test --verbose` on Linux, macOS, and Windows. - **End-to-end**: builds soulfind from source (LDC + `dub build :server`), - points `SOULFIND_BIN` at it, and runs both the library and CLI e2e suites - with `SOULSEEK_E2E_REQUIRED=1`, so a missing server fails instead of skipping. + points `SOULFIND_BIN` at it, and runs all four e2e suites with + `SOULSEEK_E2E_REQUIRED=1`, so a missing server fails instead of skipping. ## Contributing diff --git a/soulseek-rs/src/ui/main_tui/search.rs b/soulseek-rs/src/ui/main_tui/search.rs index 95fafe2..e2892cf 100644 --- a/soulseek-rs/src/ui/main_tui/search.rs +++ b/soulseek-rs/src/ui/main_tui/search.rs @@ -213,14 +213,9 @@ impl MainTui { self.show_search(self.state.searches.len() - 1); } - /// Run the search at `index` again, in its place. - /// - /// This is what a restored list is for: last session's queries come back - /// with nothing under them, and typing one out again to see what the - /// network has now is the work the list was meant to save. The entry - /// keeps its row rather than moving to the end — the user is looking at - /// it — and a run still collecting is stopped first, or two threads would - /// be pouring into one result set. + /// Run the search at `index` again, in its row: a restored query comes + /// back with nothing under it, and this saves typing it out. A run still + /// collecting is stopped first, or two threads would fill one set. pub(super) fn rerun_search_at_index(&mut self, index: usize) { let Some(previous) = self.state.searches.get(index) else { return; @@ -255,18 +250,12 @@ impl MainTui { let client = self.client.clone(); let timeout = self.search_timeout; + // Results are polled in update_search_results, not returned here. thread::spawn(move || { - match client.search_with_cancel( - &query, - timeout, - Some(cancel_flag.clone()), - ) { - Ok(_results) => { - // Results will be polled in update_search_results - } - Err(e) => { - soulseek_rs::warn!("Search failed: {e}"); - } + if let Err(e) = + client.search_with_cancel(&query, timeout, Some(cancel_flag)) + { + soulseek_rs::warn!("Search failed: {e}"); } }); diff --git a/soulseek-rs/tests/cli_e2e.rs b/soulseek-rs/tests/cli_e2e.rs index b6b0cb9..b045e31 100644 --- a/soulseek-rs/tests/cli_e2e.rs +++ b/soulseek-rs/tests/cli_e2e.rs @@ -11,8 +11,8 @@ mod common; -use common::{CLI_ENV_VARS, free_port, soulfind_binary}; -use soulseek_rs::{Client, ClientSettings, PeerAddress}; +use common::{CLI_ENV_VARS, Soulfind, free_port, login, settle}; +use soulseek_rs::Client; use std::io::Write; use std::net::{TcpStream, ToSocketAddrs}; use std::path::{Path, PathBuf}; @@ -508,8 +508,9 @@ static SERVER_GATE: std::sync::Mutex<()> = std::sync::Mutex::new(()); struct TestServer { host: String, port: u16, - child: Option, - db: Option, + /// The soulfind spawned for this run; `None` against a server named by + /// `SOULSEEK_TEST_SERVER`. + _server: Option, _gate: std::sync::MutexGuard<'static, ()>, /// Daemons started for this server, one per (user, session flags). Dropped /// with the server, which kills them. @@ -640,41 +641,17 @@ impl TestServer { return Some(Self { host: host.to_string(), port, - child: None, - db: None, + _server: None, _gate: gate, daemons: std::sync::Mutex::new(std::collections::HashMap::new()), }); } - let bin = soulfind_binary()?; - let port = free_port()?; - let db = std::env::temp_dir().join(format!("soulfind-cli-{port}.db")); - let _ = std::fs::remove_file(&db); - - let mut child = Command::new(&bin) - .arg("-p") - .arg(port.to_string()) - .arg("-d") - .arg(&db) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() - .ok()?; - - if wait_until_listening("127.0.0.1", port, Duration::from_secs(5)) - .is_none() - { - let _ = child.kill(); - let _ = child.wait(); - return None; - } - + let server = Soulfind::start()?; Some(Self { host: "127.0.0.1".to_string(), - port, - child: Some(child), - db: Some(db), + port: server.port(), + _server: Some(server), _gate: gate, daemons: std::sync::Mutex::new(std::collections::HashMap::new()), }) @@ -777,31 +754,7 @@ impl TestServer { /// An in-process client, used to stand up the other end of a test. fn client(&self, user: &str, shares: Vec) -> Client { - let port = free_port().expect("peer port"); - let mut client = Client::with_settings(ClientSettings { - username: user.to_string(), - password: "pw".to_string(), - server_address: PeerAddress::new(self.host.clone(), self.port), - enable_listen: true, - listen_port: port, - shared_directories: shares, - version: soulseek_rs::ClientVersion::default(), - }); - client.connect().expect("peer connect"); - assert!(client.login().expect("peer login"), "peer should log in"); - client - } -} - -impl Drop for TestServer { - fn drop(&mut self) { - if let Some(child) = self.child.as_mut() { - let _ = child.kill(); - let _ = child.wait(); - } - if let Some(db) = self.db.as_ref() { - let _ = std::fs::remove_file(db); - } + login(&self.host, self.port, user, shares) } } @@ -829,17 +782,7 @@ macro_rules! server_or_skip { match TestServer::resolve() { Some(server) => server, None => { - let required = std::env::var("SOULSEEK_E2E_REQUIRED") - .is_ok_and(|v| v != "0" && !v.is_empty()); - assert!( - !required, - "SOULSEEK_E2E_REQUIRED is set but no soulfind server could \ - be started (set SOULFIND_BIN or SOULSEEK_TEST_SERVER)" - ); - println!( - "cli e2e skipped: no soulfind server (set SOULFIND_BIN or \ - SOULSEEK_TEST_SERVER to run)" - ); + common::no_server("cli e2e"); return; } } @@ -936,11 +879,6 @@ fn split_session_flags(args: &[&str]) -> (Vec, Vec) { (session, command) } -/// Wait out the SetWaitPort registrations so peer lookups resolve. -fn settle() { - std::thread::sleep(Duration::from_secs(1)); -} - /// Like [`cli`], but keeping a config file so a command that reads or writes /// one (the wishlist) sees it. /// diff --git a/soulseek-rs/tests/common/mod.rs b/soulseek-rs/tests/common/mod.rs index 0ecb1f5..223131c 100644 --- a/soulseek-rs/tests/common/mod.rs +++ b/soulseek-rs/tests/common/mod.rs @@ -3,7 +3,11 @@ #![allow(dead_code)] +use soulseek_rs::{Client, ClientSettings, PeerAddress}; +use std::net::TcpStream; use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; /// Every environment variable the binary reads, cleared for each run so a /// developer's shell (or a stray `.env`) cannot change what a test observes — @@ -44,3 +48,116 @@ pub fn free_port() -> Option { .ok() .map(|addr| addr.port()) } + +/// A soulfind spawned for one test, on a port nobody else holds, with a +/// throwaway database that goes with it. +pub struct Soulfind { + child: Child, + db: PathBuf, + port: u16, +} + +impl Soulfind { + pub fn start() -> Option { + 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 { + let mut child = Command::new(soulfind_binary()?) + .args(["-d", db.to_str()?, "-p", &port.to_string()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .ok()?; + + 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. + if child.try_wait().ok().flatten().is_some() { + return None; + } + if TcpStream::connect(("127.0.0.1", port)).is_ok() { + return Some(child); + } + std::thread::sleep(Duration::from_millis(100)); + } + let _ = child.kill(); + let _ = child.wait(); + None + } + + pub const fn port(&self) -> u16 { + self.port + } + + pub fn address(&self) -> String { + format!("127.0.0.1:{}", self.port) + } + + pub fn stop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } + + /// Bring it back on the same port and database. + pub fn restart(&mut self) -> Option<()> { + self.stop(); + self.child = Self::spawn(self.port, &self.db)?; + Some(()) + } + + /// A client logged in to this server as `user`, sharing `shares`. + pub fn client(&self, user: &str, shares: Vec) -> Client { + login("127.0.0.1", self.port, user, shares) + } +} + +impl Drop for Soulfind { + fn drop(&mut self) { + self.stop(); + let _ = std::fs::remove_file(&self.db); + } +} + +/// An in-process client logged in to the server at `host:port` as `user`, +/// sharing `shares`: the other end of a test. +/// +/// The listener stays on: a peer delivers search results and transfers over +/// a connection it opens back to us, so a client without one hears nothing. +pub fn login(host: &str, port: u16, user: &str, shares: Vec) -> Client { + let mut client = Client::with_settings(ClientSettings { + username: user.to_string(), + password: "pw".to_string(), + server_address: PeerAddress::new(host.to_string(), port), + enable_listen: true, + listen_port: free_port().expect("peer port"), + shared_directories: shares, + version: soulseek_rs::ClientVersion::default(), + }); + client.connect().expect("peer connect"); + assert!(client.login().expect("peer login"), "peer should log in"); + client +} + +/// Wait out the SetWaitPort registrations so peer lookups resolve. +pub fn settle() { + std::thread::sleep(Duration::from_secs(1)); +} + +/// What a suite does without a server: fail when the run said one is +/// required, otherwise say it skipped. The caller returns afterwards. +pub fn no_server(suite: &str) { + let required = std::env::var("SOULSEEK_E2E_REQUIRED") + .is_ok_and(|v| v != "0" && !v.is_empty()); + assert!( + !required, + "SOULSEEK_E2E_REQUIRED is set but no soulfind server could be \ + started for the {suite} suite (set SOULFIND_BIN)" + ); + println!("{suite} skipped: no soulfind server (set SOULFIND_BIN to run)"); +} diff --git a/soulseek-rs/tests/daemon_protocol_e2e.rs b/soulseek-rs/tests/daemon_protocol_e2e.rs index 88e7bca..60fd48f 100644 --- a/soulseek-rs/tests/daemon_protocol_e2e.rs +++ b/soulseek-rs/tests/daemon_protocol_e2e.rs @@ -13,7 +13,7 @@ mod common; -use common::{CLI_ENV_VARS, free_port, soulfind_binary}; +use common::{CLI_ENV_VARS, Soulfind, free_port}; use std::io::{BufRead, BufReader, Write}; use std::net::TcpStream; use std::path::{Path, PathBuf}; @@ -215,17 +215,7 @@ macro_rules! daemon_or_skip { match Daemon::start() { Some(daemon) => daemon, None => { - let required = std::env::var("SOULSEEK_E2E_REQUIRED") - .is_ok_and(|v| v != "0" && !v.is_empty()); - assert!( - !required, - "SOULSEEK_E2E_REQUIRED is set but no daemon could be \ - started (set SOULFIND_BIN)" - ); - println!( - "daemon protocol e2e skipped: no soulfind server (set \ - SOULFIND_BIN to run)" - ); + common::no_server("daemon protocol e2e"); return; } } @@ -431,66 +421,6 @@ fn discover_serves_a_contract_whose_parameters_match_what_is_accepted() { // Fixtures // --------------------------------------------------------------------------- -struct Soulfind { - child: Child, - db: PathBuf, - port: u16, -} - -impl Soulfind { - fn start() -> Option { - let port = free_port()?; - let db = std::env::temp_dir().join(format!("soulfind-proto-{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 { - let bin = soulfind_binary()?; - - let mut child = Command::new(bin) - .args(["-d", db.to_str()?, "-p", &port.to_string()]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() - .ok()?; - - let deadline = Instant::now() + Duration::from_secs(10); - while Instant::now() < deadline { - if TcpStream::connect(("127.0.0.1", port)).is_ok() { - return Some(child); - } - std::thread::sleep(Duration::from_millis(100)); - } - let _ = child.kill(); - let _ = child.wait(); - None - } - - fn address(&self) -> String { - format!("127.0.0.1:{}", self.port) - } - - fn stop(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); - } - - fn restart(&mut self) -> Option<()> { - self.stop(); - self.child = Self::spawn(self.port, &self.db)?; - Some(()) - } -} - -impl Drop for Soulfind { - fn drop(&mut self) { - self.stop(); - let _ = std::fs::remove_file(&self.db); - } -} - fn wait_until(timeout: Duration, mut f: impl FnMut() -> bool) -> bool { let deadline = Instant::now() + timeout; while Instant::now() < deadline { diff --git a/soulseek-rs/tests/tui_e2e.rs b/soulseek-rs/tests/tui_e2e.rs index b192362..6436c57 100644 --- a/soulseek-rs/tests/tui_e2e.rs +++ b/soulseek-rs/tests/tui_e2e.rs @@ -14,17 +14,15 @@ mod common; -use common::{free_port, soulfind_binary}; +use common::{Soulfind, settle}; use ratatui::Terminal; use ratatui::backend::TestBackend; use ratatui::crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; -use soulseek_rs::{Client, ClientSettings, PeerAddress}; +use soulseek_rs::Client; use soulseek_rs_tui::MainTui; -use soulseek_rs_tui::models::{FocusedPane, SearchStatus}; +use soulseek_rs_tui::models::SearchStatus; use soulseek_rs_tui::persist::state::StateStore; -use std::net::TcpStream; use std::path::Path; -use std::process::{Child, Command, Stdio}; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -32,98 +30,18 @@ use std::time::{Duration, Instant}; /// it out; long enough for a peer on loopback to answer many times over. const SEARCH_WINDOW: Duration = Duration::from_secs(3); -/// A soulfind spawned for one test, on a port nobody else holds. -struct Soulfind { - child: Child, - db: std::path::PathBuf, - port: u16, -} - -impl Soulfind { - fn start() -> Option { - let bin = soulfind_binary()?; - let port = free_port()?; - let db = std::env::temp_dir().join(format!("soulfind-tui-{port}.db")); - let _ = std::fs::remove_file(&db); - - let mut child = Command::new(bin) - .args(["-d", db.to_str()?, "-p", &port.to_string()]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() - .ok()?; - - let deadline = Instant::now() + Duration::from_secs(10); - while Instant::now() < deadline { - if TcpStream::connect(("127.0.0.1", port)).is_ok() { - return Some(Self { child, db, port }); - } - std::thread::sleep(Duration::from_millis(100)); - } - let _ = child.kill(); - let _ = child.wait(); - None - } - - /// A client logged in to this server as `user`, sharing `shares`. - /// - /// The listener stays on: a peer delivers search results over a - /// connection it opens back to the searcher, so a searcher without one - /// hears nothing. - fn client(&self, user: &str, shares: Vec) -> Client { - let mut client = Client::with_settings(ClientSettings { - username: user.to_string(), - password: "pw".to_string(), - server_address: PeerAddress::new( - "127.0.0.1".to_string(), - self.port, - ), - enable_listen: true, - listen_port: free_port().expect("peer port"), - shared_directories: shares, - version: soulseek_rs::ClientVersion::default(), - }); - client.connect().expect("peer connect"); - assert!(client.login().expect("peer login"), "peer should log in"); - client - } -} - -impl Drop for Soulfind { - fn drop(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); - let _ = std::fs::remove_file(&self.db); - } -} - macro_rules! server_or_skip { () => { match Soulfind::start() { Some(server) => server, None => { - let required = std::env::var("SOULSEEK_E2E_REQUIRED") - .is_ok_and(|v| v != "0" && !v.is_empty()); - assert!( - !required, - "SOULSEEK_E2E_REQUIRED is set but no soulfind server could \ - be started (set SOULFIND_BIN)" - ); - println!( - "tui e2e skipped: no soulfind server (set SOULFIND_BIN to \ - run)" - ); + common::no_server("tui e2e"); return; } } }; } -/// Wait out the SetWaitPort registrations so peer lookups resolve. -fn settle() { - std::thread::sleep(Duration::from_secs(1)); -} - /// The window under test: the real `MainTui` over a real session, drawn onto /// a buffer instead of a terminal. struct Window { @@ -162,23 +80,22 @@ impl Window { self.terminal.backend().to_string() } - /// Keep the window running until `done` says so, or `timeout` runs out. - /// Returns the last screen either way, so a failing test shows what the - /// user would have been looking at. + /// Keep the window running until `done` says so, and hand back the + /// screen it stopped on. Past `timeout` the test fails showing the last + /// screen, what the user would have been looking at, under `what`. fn run_until( &mut self, timeout: Duration, + what: &str, mut done: impl FnMut(&MainTui, &str) -> bool, - ) -> (bool, String) { + ) -> String { let deadline = Instant::now() + timeout; loop { let screen = self.frame(); if done(&self.tui, &screen) { - return (true, screen); - } - if Instant::now() > deadline { - return (false, screen); + return screen; } + assert!(Instant::now() <= deadline, "{what}:\n{screen}"); std::thread::sleep(Duration::from_millis(100)); } } @@ -192,10 +109,6 @@ impl Window { .map(|file| file.filename.clone()) .collect() } - - fn search_status(&self, index: usize) -> SearchStatus { - self.tui.state().searches[index].status.clone() - } } /// Whether the file the sharer offers has reached the Results pane, on @@ -208,6 +121,11 @@ fn shows_the_shared_file(tui: &MainTui, screen: &str) -> bool { && screen.contains("tui_probe_rerun") } +/// Whether the one search has run its window out. +fn finished(tui: &MainTui, _: &str) -> bool { + tui.state().searches[0].status == SearchStatus::Completed +} + #[test] fn shift_s_runs_a_restored_query_again_and_the_answers_land_on_screen() { let server = server_or_skip!(); @@ -215,11 +133,8 @@ fn shift_s_runs_a_restored_query_again_and_the_answers_land_on_screen() { // The other end of the wire: a peer sharing one file with a name only // this test searches for. let share = tempfile::tempdir().expect("share dir"); - std::fs::write( - share.path().join("tui_probe_rerun_vexq.bin"), - (0..4096u32).map(|i| (i % 251) as u8).collect::>(), - ) - .expect("share file"); + std::fs::write(share.path().join("tui_probe_rerun_vexq.bin"), [7u8; 64]) + .expect("share file"); let _sharer = server .client("tui_e2e_sharer", vec![share.path().display().to_string()]); settle(); @@ -240,46 +155,21 @@ fn shift_s_runs_a_restored_query_again_and_the_answers_land_on_screen() { screen.contains("vexq"), "the old query is listed:\n{screen}" ); - assert_eq!(window.tui.state().searches.len(), 1); - assert_eq!(window.search_status(0), SearchStatus::Completed); - assert!( - window.result_names().is_empty(), - "and it has nothing under it" - ); - assert_eq!( - window.tui.state().focused_pane, - FocusedPane::Searches, - "the window opens on the list, so the query is already highlighted" - ); - // One key, no typing. + // One key, no typing: the window opens on the list with the query + // highlighted. window.press(KeyCode::Char('S')); - - assert_eq!(window.tui.state().searches.len(), 1, "no second row"); - assert_eq!(window.search_status(0), SearchStatus::Active); - assert_eq!(window.tui.state().focused_pane, FocusedPane::Results); - - let (found, screen) = - window.run_until(Duration::from_secs(15), shows_the_shared_file); - assert!( - found, - "the sharer's file should reach the screen:\n{screen}" + window.run_until( + Duration::from_secs(15), + "the sharer's file should reach the screen", + shows_the_shared_file, ); - assert!( - window - .result_names() - .iter() - .all(|name| name.contains("tui_probe_rerun")), - "only what the network answered: {:?}", - window.result_names() - ); - // The search runs its window out and settles, keeping what it found. - let (done, screen) = window - .run_until(SEARCH_WINDOW + Duration::from_secs(5), |tui, _| { - tui.state().searches[0].status == SearchStatus::Completed - }); - assert!(done, "the search should finish:\n{screen}"); + let screen = window.run_until( + SEARCH_WINDOW + Duration::from_secs(5), + "the search should finish", + finished, + ); assert!(shows_the_shared_file(&window.tui, &screen)); let first_run = window.result_names(); @@ -287,23 +177,17 @@ fn shift_s_runs_a_restored_query_again_and_the_answers_land_on_screen() { // the same peer has to answer the same searcher a second time, and the // window has to start the display over rather than stack the answers. window.press(KeyCode::Char('1')); - assert_eq!(window.tui.state().focused_pane, FocusedPane::Searches); window.press(KeyCode::Char('S')); - assert_eq!(window.search_status(0), SearchStatus::Active); - assert!( - window.result_names().is_empty(), - "the display starts over: {:?}", - window.result_names() + window.run_until( + Duration::from_secs(15), + "the second run should be answered too", + shows_the_shared_file, + ); + window.run_until( + SEARCH_WINDOW + Duration::from_secs(5), + "the second run should finish", + finished, ); - - let (found, screen) = - window.run_until(Duration::from_secs(15), shows_the_shared_file); - assert!(found, "the second run should be answered too:\n{screen}"); - let (done, _) = window - .run_until(SEARCH_WINDOW + Duration::from_secs(5), |tui, _| { - tui.state().searches[0].status == SearchStatus::Completed - }); - assert!(done); assert_eq!( window.result_names(), first_run,