From af4434e7640319a7211c4e088aa049fa2f109bc3 Mon Sep 17 00:00:00 2001 From: Michel de Graaf Date: Sun, 26 Jul 2026 12:56:40 +0200 Subject: [PATCH] test: prove the documented examples against the binary (#15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: prove the documented examples against the binary The README, the website and the shipped SKILL.md all promise specific records, keys and exit codes, and nothing was checking that the binary still agrees. Cover the claims that were unverified: the eleven credential-free commands, the one-day bound on every wait, the config directory relocation, `serve` refusing to run without a listener, the four action commands that answer with an exit code alone, the search filters, `--password-stdin`, `room listen`'s join records, and every `--json` field the docs name for search, browse, whoami, user, shares, room, download, serve and portmap. The library README's examples now compile as doctests: lib.rs includes it as the crate docs and the fences are `no_run`, so a signature change cannot leave the advertised usage stale. Two things were wrong. SKILL.md listed `queued` as a `serve` upload status, which `upload_record` never emits — an agent branching on it would wait for a state that never arrives. And the library README still pointed at the 7.x lib. * test: cover the completions commands the docs now advertise --- soulseek-rs-lib/README.md | 8 +- soulseek-rs-lib/src/lib.rs | 4 + soulseek-rs/src/SKILL.md | 2 +- soulseek-rs/tests/cli_e2e.rs | 540 ++++++++++++++++++++++++++++++++++- 4 files changed, 543 insertions(+), 11 deletions(-) diff --git a/soulseek-rs-lib/README.md b/soulseek-rs-lib/README.md index 61508a2..4ba0c96 100644 --- a/soulseek-rs-lib/README.md +++ b/soulseek-rs-lib/README.md @@ -15,14 +15,14 @@ Add this to your `Cargo.toml`: ```toml [dependencies] -soulseek-rs-lib = "7" +soulseek-rs-lib = "8" ``` ## Example ### Simple Usage -```rust +```rust,no_run use soulseek_rs::Client; use std::time::Duration; @@ -59,7 +59,7 @@ fn main() -> Result<(), Box> { ### Advanced Configuration -```rust +```rust,no_run use soulseek_rs::{Client, ClientSettings, PeerAddress}; fn main() -> Result<(), Box> { @@ -85,7 +85,7 @@ The server-backed lookups are request/poll pairs: you ask, then read the answer once it lands. `request_user_info` clears any previous answer first, so the loop below observes this request rather than the last one. -```rust +```rust,no_run use soulseek_rs::Client; use std::thread::sleep; use std::time::{Duration, Instant}; diff --git a/soulseek-rs-lib/src/lib.rs b/soulseek-rs-lib/src/lib.rs index 25e8d4c..010a577 100644 --- a/soulseek-rs-lib/src/lib.rs +++ b/soulseek-rs-lib/src/lib.rs @@ -1,3 +1,7 @@ +// The README is the crate's front page, and `cargo test --doc` compiles every +// example in it, so a signature change cannot leave the advertised usage stale. +#![doc = include_str!("../README.md")] + // Core modules pub mod actor; pub mod client; diff --git a/soulseek-rs/src/SKILL.md b/soulseek-rs/src/SKILL.md index 719eebb..b374354 100644 --- a/soulseek-rs/src/SKILL.md +++ b/soulseek-rs/src/SKILL.md @@ -81,7 +81,7 @@ credentials and connection work. **`serve`** — stay online sharing files, one object per upload change: `user`, `path`, `size`, `bytes_sent`, `speed`, `status` -(`queued`/`uploading`/`completed`/`cancelled`/`failed`), `reason`. +(`uploading`/`completed`/`cancelled`/`failed`), `reason`. **`shares list`** — one object per configured folder: `directory`, `usable`. **`shares add `** and **`shares remove `** — change the diff --git a/soulseek-rs/tests/cli_e2e.rs b/soulseek-rs/tests/cli_e2e.rs index 7825acc..a687c8e 100644 --- a/soulseek-rs/tests/cli_e2e.rs +++ b/soulseek-rs/tests/cli_e2e.rs @@ -29,7 +29,9 @@ const EXIT_NO_RESULTS: i32 = 4; /// Every environment variable the CLI reads, cleared for each run so a /// developer's shell (or a stray `.env`) cannot change what a test observes. -const CLI_ENV_VARS: [&str; 11] = [ +const CLI_ENV_VARS: [&str; 13] = [ + "SOULSEEK_CONFIG_DIR", + "SOULSEEK_STATE_DIR", "SOULSEEK_USERNAME", "SOULSEEK_PASSWORD", "SOULSEEK_PASSWORD_CMD", @@ -1031,14 +1033,35 @@ fn room_listen_streams_what_is_said_in_the_room() { .say_in_room("cli_e2e_lobby_b", "streamed line") .expect("say"); + // A second arrival: the README says a join is its own record type, with an + // empty message field. + let latecomer = server.client("cli_e2e_room_joiner", Vec::new()); + latecomer.join_room("cli_e2e_lobby_b").expect("join"); + let output = child.wait_with_output().expect("the binary should finish"); assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output)); - let heard = records(&output).iter().any(|line| { - serde_json::from_str::(line) - .is_ok_and(|value| value["message"] == "streamed line") - }); - assert!(heard, "stdout was: {:?}", stdout(&output)); + let events: Vec = records(&output) + .iter() + .filter_map(|line| serde_json::from_str(line).ok()) + .collect(); + + let said = events + .iter() + .find(|event| event["message"] == "streamed line") + .unwrap_or_else(|| panic!("stdout was: {:?}", stdout(&output))); + assert_eq!(said["type"], "message"); + assert_eq!(said["room"], "cli_e2e_lobby_b"); + assert_eq!(said["user"], "cli_e2e_room_speaker"); + + // The listener's own arrival is a join too, so pick the one that matters. + let joined = events + .iter() + .find(|event| event["user"] == "cli_e2e_room_joiner") + .unwrap_or_else(|| panic!("no join record in {:?}", stdout(&output))); + assert_eq!(joined["type"], "join"); + assert_eq!(joined["room"], "cli_e2e_lobby_b"); + assert_eq!(joined["message"], ""); } #[test] @@ -1264,6 +1287,13 @@ fn serve_stays_online_and_reports_an_upload_a_peer_pulls() { .as_str() .is_some_and(|p| p.contains("served.bin")) ); + // The fields the README promises a JSON upload record carries. + for key in ["status", "user", "path", "bytes_sent", "size", "speed"] { + assert!( + last.get(key).is_some(), + "serve should carry `{key}`: {last}" + ); + } } #[test] @@ -1583,3 +1613,501 @@ fn asking_about_an_unknown_user_succeeds_with_an_offline_record() { assert_eq!(record["status"], "offline"); assert_eq!(record["shared_files"], 0); } + +/// Assert that `output` succeeded and that its first record carries every key +/// the documentation promises. `label` names the invocation in the failure. +fn assert_record_keys(output: &Output, label: &str, keys: &[&str]) { + assert_eq!( + code(output), + EXIT_OK, + "`{label}` should succeed, stderr: {}", + stderr(output) + ); + let record: serde_json::Value = serde_json::from_str( + records(output).first().expect("at least one record"), + ) + .expect("valid JSON"); + for key in keys { + assert!( + record.get(*key).is_some(), + "`{label}` should carry `{key}`, got {record}" + ); + } +} + +/// "Thirteen of these need no credentials, because they never touch the +/// network." A script's first call is one of these, so an account must not be +/// a prerequisite for any of them. +#[test] +fn the_commands_advertised_as_credential_free_run_without_an_account() { + let dir = Scratch::new("nocred"); + let config = dir.path().join("config.toml"); + let target = Scratch::new("nocred-skills"); + let skills = target.display(); + let shared = Scratch::new("nocred-shared"); + let shared_path = shared.display(); + // `completions` writes where the shell looks, which is under the home + // directory: give it a throwaway one rather than the developer's. + let home = Scratch::new("nocred-home"); + + let cases: Vec> = vec![ + vec!["config", "path"], + vec!["config", "list"], + vec!["config", "set", "search_timeout", "12"], + vec!["config", "get", "search_timeout"], + vec!["shares", "add", &shared_path], + vec!["shares", "list"], + vec!["shares", "remove", &shared_path], + vec!["skills", "install", "--dir", &skills], + vec!["skills", "list", "--dir", &skills], + vec!["skills", "uninstall", "--dir", &skills], + vec!["completions", "install", "--shell", "fish"], + vec!["completions", "uninstall", "--shell", "fish"], + ]; + + for case in cases { + let mut all = vec!["--config", config.to_str().expect("utf-8 path")]; + all.extend_from_slice(&case); + let mut invocation = command(&all); + invocation.env("HOME", home.path()); + invocation.env("XDG_CONFIG_HOME", home.path().join("config")); + let output = invocation.output().expect("the binary should run"); + assert_eq!( + code(&output), + EXIT_OK, + "`{}` should not need credentials, stderr: {}", + case.join(" "), + stderr(&output) + ); + } + + // `portmap` is the thirteenth: it talks to the router, never to the + // server, so it answers with its own verdict rather than a credentials + // error. + let portmap = with_config(&config, &["portmap"]); + assert!( + matches!(code(&portmap), EXIT_OK | EXIT_NO_RESULTS), + "portmap should not need credentials, got {} / {}", + code(&portmap), + stderr(&portmap) + ); +} + +/// "Every wait expressed in seconds is bounded to one day, so a mistyped flag +/// is rejected rather than hanging until the heat death of the universe." +#[test] +fn a_wait_longer_than_a_day_is_rejected_rather_than_accepted() { + let day = 86_400; + let over = (day + 1).to_string(); + let cases: Vec> = vec![ + vec!["--no-config", "--search-timeout", &over, "search", "x"], + vec![ + "--no-config", + "room", + "listen", + "lobby", + "--duration", + &over, + ], + vec!["--no-config", "message", "read", "--duration", &over], + vec!["--no-config", "serve", "--duration", &over], + vec!["--no-config", "get", "x", "--timeout", &over], + ]; + + // Missing credentials are a usage error too, so the exit code alone would + // not prove the bound rejected anything: the range has to be named. + for case in cases { + let output = run(&case); + assert_eq!( + code(&output), + EXIT_USAGE, + "`{}` should be a usage error, stderr: {}", + case.join(" "), + stderr(&output) + ); + assert!( + stderr(&output).contains(&format!("1..={day}")), + "`{}` should be rejected by the one-day bound, stderr: {}", + case.join(" "), + stderr(&output) + ); + } +} + +/// "`SOULSEEK_CONFIG_DIR` and `SOULSEEK_STATE_DIR` relocate the config and +/// state directories wholesale" — what a containerised run relies on. +#[test] +fn the_config_directory_can_be_relocated_wholesale() { + let home = Scratch::new("relocated"); + let config_dir = home.path().join("config"); + let state_dir = home.path().join("state"); + + let mut set = command(&["config", "set", "search_timeout", "17"]); + set.env("SOULSEEK_CONFIG_DIR", &config_dir); + set.env("SOULSEEK_STATE_DIR", &state_dir); + let output = set.output().expect("the binary should run"); + assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output)); + + let written = config_dir.join("config.toml"); + assert!( + written.exists(), + "the setting should land under SOULSEEK_CONFIG_DIR, not the default" + ); + + let mut path = command(&["config", "path"]); + path.env("SOULSEEK_CONFIG_DIR", &config_dir); + let reported = path.output().expect("the binary should run"); + assert!( + records(&reported)[0].ends_with(written.to_str().expect("utf-8 path")), + "got {:?}", + records(&reported) + ); +} + +/// The shipped SKILL.md tells an agent, key by key, what the offline commands +/// print. An agent has no way to notice a rename, so pin them here. +#[test] +fn the_offline_json_records_carry_the_keys_the_skill_file_names() { + let dir = Scratch::new("skill-keys"); + let config = dir.path().join("config.toml"); + let shared = Scratch::new("skill-keys-shared"); + let shared_path = shared.display(); + let skills = Scratch::new("skill-keys-agent"); + let skills_path = skills.display(); + + with_config(&config, &["shares", "add", &shared_path]); + + let cases: Vec<(Vec<&str>, Vec<&str>)> = vec![ + ( + vec!["--json", "shares", "list"], + vec!["directory", "usable"], + ), + ( + vec!["--json", "skills", "list", "--dir", &skills_path], + vec!["agent", "path", "action"], + ), + (vec!["--json", "config", "list"], vec!["key", "value"]), + (vec!["--json", "config", "path"], vec!["key", "value"]), + ]; + + for (case, keys) in cases { + let output = with_config(&config, &case); + assert_record_keys(&output, &case.join(" "), &keys); + } + + // `completions` writes into the shell's own directory, so it only ever + // runs against a throwaway home. + let home = Scratch::new("skill-keys-home"); + for verb in ["install", "uninstall"] { + let mut invocation = command(&[ + "--no-config", + "--json", + "completions", + verb, + "--shell", + "fish", + ]); + invocation.env("HOME", home.path()); + invocation.env("XDG_CONFIG_HOME", home.path().join("config")); + let output = invocation.output().expect("the binary should run"); + assert_record_keys( + &output, + &format!("completions {verb}"), + &["shell", "path", "action"], + ); + } +} + +/// The website's connectivity section: "`serve` refuses to start with the +/// listener off", because a source nobody can connect to serves nothing. +#[test] +fn serve_refuses_to_run_without_a_listener() { + let share = Scratch::new("serve-nolistener"); + let output = run(&[ + "--no-config", + "--username", + "someone", + "--password", + "pw", + "--no-listener", + "--shared-dir", + &share.display(), + "serve", + "--duration", + "5", + ]); + assert_eq!(code(&output), EXIT_USAGE, "stdout: {}", stdout(&output)); + assert!( + stderr(&output).contains("listener"), + "the error should name the listener, got {}", + stderr(&output) + ); +} + +/// The four commands the README calls out as acting rather than reporting: +/// "say nothing on stdout and answer with their exit code alone". +#[test] +fn the_action_commands_answer_with_their_exit_code_alone() { + let server = server_or_skip!(); + let shared = Scratch::new("action-shared"); + let shared_path = shared.display(); + let dir = Scratch::new("action-config"); + let config = dir.path().join("config.toml"); + let listener = server.client("cli_e2e_action_peer", Vec::new()); + listener.join_room("cli_e2e_action_room").expect("join"); + settle(); + + for case in [ + vec!["room", "say", "cli_e2e_action_room", "hello room"], + vec!["message", "send", "cli_e2e_action_peer", "hello there"], + ] { + let output = cli(&server, "cli_e2e_action", &case); + assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output)); + assert!( + stdout(&output).is_empty(), + "`{}` should print nothing on stdout, got {:?}", + case.join(" "), + stdout(&output) + ); + } + + for case in [ + vec!["shares", "add", shared_path.as_str()], + vec!["shares", "remove", shared_path.as_str()], + ] { + let output = with_config(&config, &case); + assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output)); + assert!( + stdout(&output).is_empty(), + "`{}` should print nothing on stdout, got {:?}", + case.join(" "), + stdout(&output) + ); + } +} + +/// The recipe the README leads with: `--min-bitrate` and `--free-slots` keep +/// only what a script asked for, rather than filtering nothing at all. +#[test] +fn search_filters_drop_what_they_advertise_dropping() { + let server = server_or_skip!(); + let share = Scratch::new("share"); + std::fs::write(share.path().join("cli_probe_filt.bin"), probe_bytes()) + .expect("share file"); + let _sharer = server.client("cli_e2e_sharer_filt", vec![share.display()]); + settle(); + + let unfiltered = cli( + &server, + "cli_e2e_filterer", + &["--search-timeout", "5", "--json", "search", "filt"], + ); + assert_eq!( + code(&unfiltered), + EXIT_OK, + "stderr: {}", + stderr(&unfiltered) + ); + assert!( + records(&unfiltered) + .iter() + .any(|line| line.contains("cli_probe_filt")), + "the probe should be findable before filtering" + ); + + // A .bin carries no bitrate, so a bitrate floor must exclude it rather + // than let an unknown through. + let filtered = cli( + &server, + "cli_e2e_filterer", + &[ + "--search-timeout", + "5", + "search", + "filt", + "--min-bitrate", + "320", + ], + ); + assert_eq!( + code(&filtered), + EXIT_NO_RESULTS, + "a bitrate floor should drop a result with no bitrate, got {:?}", + records(&filtered) + ); + + // `--free-slots` is a narrowing filter too: it may keep everything, but it + // must never invent results the unfiltered search did not have. + let free = cli( + &server, + "cli_e2e_filterer", + &["--search-timeout", "5", "search", "filt", "--free-slots"], + ); + assert!( + matches!(code(&free), EXIT_OK | EXIT_NO_RESULTS), + "stderr: {}", + stderr(&free) + ); + assert!( + records(&free).len() <= records(&unfiltered).len(), + "a filter must not add results" + ); +} + +/// "Nothing sensitive on the command line or in the environment": +/// `--password-stdin` reads the first line of stdin, `docker login` style. +#[test] +fn the_password_can_arrive_on_stdin_instead_of_in_argv() { + let server = server_or_skip!(); + let _owner = server.client("cli_e2e_stdin_pw", Vec::new()); + + let args = [ + "--no-config", + "--quiet", + "--server", + &server.address(), + "--username", + "cli_e2e_stdin_pw", + "--password-stdin", + "--no-listener", + "whoami", + ]; + let mut child = command(&args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("the binary should run"); + child + .stdin + .as_mut() + .expect("stdin") + .write_all(b"pw\n") + .expect("write the password"); + let output = child.wait_with_output().expect("the binary should finish"); + + assert_eq!(code(&output), EXIT_OK, "stderr: {}", stderr(&output)); + assert!( + records(&output)[0].starts_with("cli_e2e_stdin_pw\t"), + "got {:?}", + records(&output) + ); +} + +/// The `--json` paragraph names, field by field, what JSON carries that text +/// does not. A missing key silently breaks the `jq` filters the README sells. +#[test] +fn json_records_carry_every_extra_field_the_readme_names() { + let server = server_or_skip!(); + let share = Scratch::new("share"); + std::fs::write(share.path().join("cli_probe_keys.bin"), probe_bytes()) + .expect("share file"); + let target = Scratch::new("downloads"); + let share_path = share.display(); + let sharer = server.client("cli_e2e_sharer_keys", vec![share.display()]); + sharer.join_room("cli_e2e_keys_room").expect("join"); + settle(); + + let mut expected: Vec<(Vec<&str>, Vec<&str>)> = vec![ + ( + vec!["search", "keys", "--search-timeout", "5"], + vec![ + "user", + "size", + "bitrate", + "path", + "duration", + "slots", + "speed", + "free_slot", + ], + ), + ( + vec!["browse", "cli_e2e_sharer_keys"], + vec!["user", "size", "path", "directory"], + ), + ( + vec!["whoami"], + vec![ + "user", + "server", + "shared_folders", + "shared_files", + "listening", + "listen_port", + "download_dir", + ], + ), + ( + vec!["user", "cli_e2e_sharer_keys"], + vec![ + "user", + "status", + "average_speed", + "shared_files", + "privileged", + "shared_folders", + ], + ), + ( + vec!["shares", "status", "--shared-dir", &share_path], + vec!["folders", "files", "directories"], + ), + ( + vec!["shares", "reindex", "--shared-dir", &share_path], + vec!["folders", "files", "directories"], + ), + ]; + expected.push((vec!["room", "list"], vec!["users", "room"])); + expected.push(( + vec!["room", "users", "cli_e2e_keys_room"], + vec!["room", "user"], + )); + + for (case, keys) in expected { + let mut args: Vec<&str> = vec!["--json"]; + args.extend(case.iter().copied()); + let output = cli(&server, "cli_e2e_keys", &args); + assert_record_keys(&output, &case.join(" "), &keys); + } + + // `download` adds the user, remote path and size alongside the local file. + let listing = + cli(&server, "cli_e2e_keys", &["browse", "cli_e2e_sharer_keys"]); + let remote_path = records(&listing) + .into_iter() + .find(|line| line.contains("cli_probe_keys.bin")) + .and_then(|row| row.rsplit('\t').next().map(String::from)) + .expect("the shared file should be listed"); + // A fresh name: the peer connects back to the port this user announced at + // login, and the browse runs above left an older one registered. + let downloaded = cli( + &server, + "cli_e2e_keys_fetcher", + &[ + "--json", + "--download-dir", + &target.display(), + "download", + "cli_e2e_sharer_keys", + &remote_path, + "--timeout", + "40", + ], + ); + assert_record_keys( + &downloaded, + "download", + &["file", "user", "path", "size"], + ); + + // `portmap` adds the port it probed; the verdict lives in the exit code, + // which is `EXIT_NO_RESULTS` on a network with no cooperating router. + let portmap = cli(&server, "cli_e2e_keys", &["--json", "portmap"]); + let record: serde_json::Value = + serde_json::from_str(records(&portmap).first().expect("one record")) + .expect("valid JSON"); + for key in ["ok", "backend", "external", "port"] { + assert!(record.get(key).is_some(), "portmap should carry `{key}`"); + } +}