From ed75a3b90248485dec81865d9ce2a46625c027a2 Mon Sep 17 00:00:00 2001 From: Chris Guidry Date: Sun, 2 Aug 2026 21:42:22 -0400 Subject: [PATCH] Pay down the terminal and test-suite debts from the review Ctrl+C now cancels a running turn and quits only when idle, matching how a REPL reads it. Slash commands match on trimmed input, and Tab completes only a single-line buffer starting with a slash. An idle viewport gives surplus rows back at once instead of holding them until the next reply. The sandbox module joins coverage builds, so its tests run under the enforced hook. The pinned-viewport test double and the real terminal now build their moves from one viewport::Move, so the tests validate the shipping math instead of a copy. On the test side: sha256_hex gets a FIPS known-answer vector, the fidelity fixtures freeze literal bodies instead of computing them with the function under test, disjunctive assertions pin their field, the corpus counts are exact, the symlink-escape test asserts against its own temp root rather than /tmp, and the duplicated fake chat server and tarball builders move into cfg(test) modules owned by chat and srd. cli.rs still holds its copies of the srd helpers; adopting the shared ones is deferred because its tiny_tar_gz builds different contents. Co-Authored-By: Claude Fable 5 --- src/chat/client.rs | 92 ++-------------------- src/chat/mod.rs | 2 + src/chat/testing.rs | 100 +++++++++++++++++++++++ src/cli.rs | 5 +- src/dm/dm_tests.rs | 91 ++------------------- src/play/keys.rs | 9 ++- src/play/mod.rs | 4 +- src/play/sandbox.rs | 62 ++++++++++++--- src/play/screen.rs | 64 +++++++++++---- src/play/screen_pinned_tests.rs | 66 ++++++++++------ src/play/screen_prompt_tests.rs | 23 +++--- src/play/screen_slash_tests.rs | 25 +++++- src/play/screen_tail_tests.rs | 2 +- src/play/screen_turn_tests.rs | 40 ++++++++++ src/play/terminal.rs | 41 +++++----- src/play/thinking.rs | 2 +- src/play/viewport.rs | 71 ++++++++++++++++- src/play/viewport_tests.rs | 54 ++++++++++++- src/srd/fetch.rs | 135 +++++++------------------------- src/srd/mod.rs | 2 + src/srd/testing.rs | 112 ++++++++++++++++++++++++++ src/srd/verify/fidelity.rs | 9 ++- src/srd/verify/fixtures.rs | 15 +++- src/srd/verify/mod.rs | 10 +-- src/srd/verify/stat_block.rs | 43 ++++++---- tests/corpus.rs | 13 ++- 26 files changed, 691 insertions(+), 401 deletions(-) create mode 100644 src/chat/testing.rs create mode 100644 src/srd/testing.rs diff --git a/src/chat/client.rs b/src/chat/client.rs index 5dd71b1..33a94cc 100644 --- a/src/chat/client.rs +++ b/src/chat/client.rs @@ -363,20 +363,15 @@ impl std::error::Error for ChatError {} #[cfg(test)] mod tests { use super::*; - use std::collections::HashMap; - use std::io::{BufRead, Read, Write}; + use crate::chat::testing::{ + CapturedRequest, fake_server as fake_server_serving, http_response, read_request, + sse_response, + }; + use std::io::Write; use std::net::TcpListener; - use std::sync::mpsc::{self, Receiver}; + use std::sync::mpsc::Receiver; use std::thread::JoinHandle; - /// One HTTP request as the fake server saw it. - struct CapturedRequest { - method: String, - path: String, - headers: HashMap, - body: String, - } - /// Serves `response` once to the first connection it accepts, then sends /// the request it received back through the returned channel. /// @@ -386,25 +381,6 @@ mod tests { fake_server_serving(vec![response]) } - /// Serves one response from `responses` per connection, in order, and - /// sends every request it read back through the returned channel. - fn fake_server_serving( - responses: Vec>, - ) -> (String, Receiver, JoinHandle<()>) { - let listener = TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let (sender, receiver) = mpsc::channel(); - let handle = std::thread::spawn(move || { - for response in responses { - let (mut stream, _) = listener.accept().unwrap(); - let captured = read_request(&stream); - stream.write_all(&response).unwrap(); - sender.send(captured).unwrap(); - } - }); - (format!("http://{addr}"), receiver, handle) - } - /// Accepts one connection, reads the request, writes `sent`, and then /// holds the connection open for `hold` without writing anything more. fn stalling_server(sent: Vec, hold: Duration) -> (String, JoinHandle<()>) { @@ -430,62 +406,6 @@ mod tests { } } - /// Reads one HTTP/1.1 request's method, path, headers, and body from - /// `stream`. - fn read_request(stream: &std::net::TcpStream) -> CapturedRequest { - let mut reader = io::BufReader::new(stream); - let mut request_line = String::new(); - reader.read_line(&mut request_line).unwrap(); - let mut parts = request_line.split_whitespace(); - let method = parts.next().unwrap().to_string(); - let path = parts.next().unwrap().to_string(); - - let mut headers = HashMap::new(); - loop { - let mut line = String::new(); - reader.read_line(&mut line).unwrap(); - let line = line.trim_end(); - if line.is_empty() { - break; - } - let (name, value) = line.split_once(':').unwrap(); - headers.insert(name.trim().to_lowercase(), value.trim().to_string()); - } - - let content_length = headers - .get("content-length") - .and_then(|value| value.parse::().ok()) - .unwrap_or(0); - let mut body = vec![0u8; content_length]; - reader.read_exact(&mut body).unwrap(); - - CapturedRequest { - method, - path, - headers, - body: String::from_utf8(body).unwrap(), - } - } - - /// Builds a canned HTTP/1.1 response with `status`, a body of - /// `content_type`, and `Connection: close`. - fn http_response(status: &str, content_type: &str, body: &str) -> Vec { - format!( - "HTTP/1.1 {status}\r\n\ - Content-Type: {content_type}\r\n\ - Content-Length: {}\r\n\ - Connection: close\r\n\ - \r\n\ - {body}", - body.len() - ) - .into_bytes() - } - - fn sse_response(body: &str) -> Vec { - http_response("200 OK", "text/event-stream", body) - } - fn client_for(api_base: String) -> Client { Client { api_base, diff --git a/src/chat/mod.rs b/src/chat/mod.rs index f193b7d..51b4e0d 100644 --- a/src/chat/mod.rs +++ b/src/chat/mod.rs @@ -1,5 +1,7 @@ pub mod client; pub mod sse; +#[cfg(test)] +pub(crate) mod testing; pub mod wire; pub use client::{ChatError, ChatStream, Client, StreamItem}; diff --git a/src/chat/testing.rs b/src/chat/testing.rs new file mode 100644 index 0000000..a48cf8e --- /dev/null +++ b/src/chat/testing.rs @@ -0,0 +1,100 @@ +//! A fake loopback chat completion server for tests: serves canned +//! HTTP/1.1 responses over a local TCP listener and records each request +//! it received, so a test can assert on both what a client sent and how +//! it handled a scripted response. + +use std::collections::HashMap; +use std::io::{self, BufRead, Read, Write}; +use std::net::TcpListener; +use std::sync::mpsc::{self, Receiver}; +use std::thread::JoinHandle; + +/// One HTTP request as the fake server saw it. +pub(crate) struct CapturedRequest { + pub(crate) method: String, + pub(crate) path: String, + pub(crate) headers: HashMap, + pub(crate) body: String, +} + +/// Serves one response from `responses` per connection, in order, and +/// sends every request it read back through the returned channel. +/// +/// Each entry in `responses` must be a complete HTTP/1.1 response, +/// including status line and headers. A write that fails is ignored: a +/// caller that stops reading before the server finishes writing, such as +/// a client that gives up after its own timeout or a cancelled turn that +/// closes the connection early, must not panic the server thread. +pub(crate) fn fake_server( + responses: Vec>, +) -> (String, Receiver, JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let (sender, receiver) = mpsc::channel(); + let handle = std::thread::spawn(move || { + for response in responses { + let (mut stream, _) = listener.accept().unwrap(); + let captured = read_request(&stream); + let _ = stream.write_all(&response); + sender.send(captured).unwrap(); + } + }); + (format!("http://{addr}"), receiver, handle) +} + +/// Reads one HTTP/1.1 request's method, path, headers, and body from +/// `stream`. +pub(crate) fn read_request(stream: &std::net::TcpStream) -> CapturedRequest { + let mut reader = io::BufReader::new(stream); + let mut request_line = String::new(); + reader.read_line(&mut request_line).unwrap(); + let mut parts = request_line.split_whitespace(); + let method = parts.next().unwrap().to_string(); + let path = parts.next().unwrap().to_string(); + + let mut headers = HashMap::new(); + loop { + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + let line = line.trim_end(); + if line.is_empty() { + break; + } + let (name, value) = line.split_once(':').unwrap(); + headers.insert(name.trim().to_lowercase(), value.trim().to_string()); + } + + let content_length = headers + .get("content-length") + .and_then(|value| value.parse::().ok()) + .unwrap_or(0); + let mut body = vec![0u8; content_length]; + reader.read_exact(&mut body).unwrap(); + + CapturedRequest { + method, + path, + headers, + body: String::from_utf8(body).unwrap(), + } +} + +/// Builds a canned HTTP/1.1 response with `status`, a body of +/// `content_type`, and `Connection: close`. +pub(crate) fn http_response(status: &str, content_type: &str, body: &str) -> Vec { + format!( + "HTTP/1.1 {status}\r\n\ + Content-Type: {content_type}\r\n\ + Content-Length: {}\r\n\ + Connection: close\r\n\ + \r\n\ + {body}", + body.len() + ) + .into_bytes() +} + +/// A canned `200 OK` response carrying `body` as an SSE event stream. +pub(crate) fn sse_response(body: &str) -> Vec { + http_response("200 OK", "text/event-stream", body) +} diff --git a/src/cli.rs b/src/cli.rs index d2bc19c..9ef9743 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -88,7 +88,10 @@ pub fn run( cli: Cli, srd_sources: &SrdSources, layer_root: &Path, - session_layers: &[PathBuf], + // `storied sandbox` is the only reader of the layers, and coverage + // builds leave that command out along with the terminal it needs, so + // the parameter has no reader there. + #[cfg_attr(coverage, allow(unused_variables))] session_layers: &[PathBuf], ) -> Result<(), String> { match cli.command { None => { diff --git a/src/dm/dm_tests.rs b/src/dm/dm_tests.rs index 055df46..8b3bb7c 100644 --- a/src/dm/dm_tests.rs +++ b/src/dm/dm_tests.rs @@ -2,98 +2,19 @@ //! keep the production file under the project's file-length guideline. //! The tool-calling round loop has its own sibling test file, which //! shares this file's fake-server harness rather than keeping its own -//! copy. +//! copy. The harness itself lives in `chat::testing`, shared with +//! `chat::client`'s own tests. use super::*; use crate::knowledge::fixtures; -use std::collections::HashMap; use std::fs; -use std::io::{self, BufRead, Read, Write}; -use std::net::TcpListener; use std::sync::Arc; -use std::sync::mpsc::{self, Receiver}; -use std::thread::JoinHandle; use tempfile::TempDir; -/// One HTTP request as the fake server saw it. -pub(super) struct CapturedRequest { - pub(super) body: String, -} - -/// Serves `responses` in order, one per accepted connection, and sends -/// each request it received back through the returned channel in the -/// same order. -/// -/// Each entry in `responses` must be a complete HTTP/1.1 response, -/// including status line and headers. A write that fails is ignored: a -/// cancelled turn can close the connection before the server finishes -/// writing. -pub(super) fn fake_server( - responses: Vec>, -) -> (String, Receiver, JoinHandle<()>) { - let listener = TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let (sender, receiver) = mpsc::channel(); - let handle = std::thread::spawn(move || { - for response in responses { - let (mut stream, _) = listener.accept().unwrap(); - let captured = read_request(&stream); - let _ = stream.write_all(&response); - sender.send(captured).unwrap(); - } - }); - (format!("http://{addr}"), receiver, handle) -} - -/// Reads one HTTP/1.1 request's body from `stream`, skipping the -/// request line and headers. -fn read_request(stream: &std::net::TcpStream) -> CapturedRequest { - let mut reader = io::BufReader::new(stream); - let mut request_line = String::new(); - reader.read_line(&mut request_line).unwrap(); - - let mut headers = HashMap::new(); - loop { - let mut line = String::new(); - reader.read_line(&mut line).unwrap(); - let line = line.trim_end(); - if line.is_empty() { - break; - } - let (name, value) = line.split_once(':').unwrap(); - headers.insert(name.trim().to_lowercase(), value.trim().to_string()); - } - - let content_length = headers - .get("content-length") - .and_then(|value| value.parse::().ok()) - .unwrap_or(0); - let mut body = vec![0u8; content_length]; - reader.read_exact(&mut body).unwrap(); - - CapturedRequest { - body: String::from_utf8(body).unwrap(), - } -} - -/// Builds a canned HTTP/1.1 response with `status`, a body of -/// `content_type`, and `Connection: close`. -fn http_response(status: &str, content_type: &str, body: &str) -> Vec { - format!( - "HTTP/1.1 {status}\r\n\ - Content-Type: {content_type}\r\n\ - Content-Length: {}\r\n\ - Connection: close\r\n\ - \r\n\ - {body}", - body.len() - ) - .into_bytes() -} - -pub(super) fn sse_response(body: &str) -> Vec { - http_response("200 OK", "text/event-stream", body) -} +use crate::chat::testing::http_response; +// Re-exported for `dm_tool_round_tests.rs`, which shares this harness +// rather than keeping its own copy. +pub(super) use crate::chat::testing::{CapturedRequest, fake_server, sse_response}; fn dm_for(api_base: String) -> Dm { Dm::new( diff --git a/src/play/keys.rs b/src/play/keys.rs index ef84495..92decef 100644 --- a/src/play/keys.rs +++ b/src/play/keys.rs @@ -56,6 +56,9 @@ pub enum Key { Quit, /// Stop the reply streaming in, if one is running. Cancel, + /// Stop the reply streaming in while a turn runs, and leave the game + /// when none does. Ctrl+C means this, the way a REPL reads it. + Interrupt, /// Put the viewport back on the last row of the screen and paint /// every row of it again. A terminal that changed size means this, /// and so does Ctrl+L. @@ -93,7 +96,7 @@ fn decode_key(key: &KeyEvent) -> Option { let alt = key.modifiers.contains(KeyModifiers::ALT); let shift = key.modifiers.contains(KeyModifiers::SHIFT); match key.code { - KeyCode::Char('c') if control => Some(Key::Quit), + KeyCode::Char('c') if control => Some(Key::Interrupt), KeyCode::Char('a') if control => Some(Key::Home), KeyCode::Char('e') if control => Some(Key::End), KeyCode::Char('k') if control => Some(Key::KillToEnd), @@ -173,8 +176,8 @@ mod tests { } #[test] - fn control_c_quits() { - assert_eq!(decode(&control(KeyCode::Char('c'))), Some(Key::Quit)); + fn control_c_interrupts() { + assert_eq!(decode(&control(KeyCode::Char('c'))), Some(Key::Interrupt)); } #[test] diff --git a/src/play/mod.rs b/src/play/mod.rs index 9c0e1e7..86a456b 100644 --- a/src/play/mod.rs +++ b/src/play/mod.rs @@ -10,7 +10,8 @@ //! terminal's own scrollback, so the transcript is still there after the //! game ends. //! -//! Coverage builds leave the `terminal` module out. Everything else here +//! Coverage builds leave the `terminal` module out, because every line of +//! it needs a terminal the test suite does not have. Everything else here //! runs under the test suite. mod editor; @@ -18,7 +19,6 @@ mod history; pub mod keys; pub mod panics; mod prompt; -#[cfg(not(coverage))] pub mod sandbox; pub mod screen; pub mod sync; diff --git a/src/play/sandbox.rs b/src/play/sandbox.rs index 907e4b4..2e894d4 100644 --- a/src/play/sandbox.rs +++ b/src/play/sandbox.rs @@ -6,7 +6,9 @@ //! ends. Nothing persists from one sandbox to the next. use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; + +use tempfile::TempDir; /// A throwaway world and player directory for one sandbox session. /// @@ -15,24 +17,42 @@ use std::path::PathBuf; /// are what the session mounts into the DM's knowledge and writes its /// campaign to. pub struct Sandbox { - root: tempfile::TempDir, + root: TempDir, } impl Sandbox { /// Creates a fresh sandbox with empty `world` and `player` - /// directories. + /// directories, under the directory the system keeps temporary files + /// in. pub fn new() -> Result { + Self::inside(&std::env::temp_dir()) + } + + /// Creates the sandbox's own directory under `parent`. + /// + /// The parent is a parameter so that a caller can name a directory + /// the sandbox cannot live in, which is what the failure this reports + /// means: no room, no permission, or nowhere to put the tree. + fn inside(parent: &Path) -> Result { let root = tempfile::Builder::new() .prefix("storied-sandbox-") - .tempdir() + .tempdir_in(parent) .map_err(|error| format!("could not create a sandbox directory: {error}"))?; + Self::under(root) + } + + /// Creates the `world` and `player` directories inside `root`, and + /// takes ownership of the tree. + /// + /// The root is a parameter so that a caller can hand over a root that + /// already holds a file of one of those names, which is what the + /// failure this reports means: the name is taken by something that is + /// not a directory. + fn under(root: TempDir) -> Result { for name in ["world", "player"] { - fs::create_dir_all(root.path().join(name)).map_err(|error| { - format!( - "could not create the sandbox {} directory: {error}", - root.path().join(name).display() - ) - })?; + let directory = root.path().join(name); + fs::create_dir_all(&directory) + .map_err(|error| format!("could not create {}: {error}", directory.display()))?; } Ok(Self { root }) } @@ -62,4 +82,26 @@ mod tests { assert_eq!(std::fs::read_dir(sandbox.player_dir()).unwrap().count(), 0); assert_ne!(sandbox.world_dir(), sandbox.player_dir()); } + + #[test] + fn a_sandbox_with_nowhere_to_live_says_so() { + let parent = TempDir::new().unwrap(); + + let error = Sandbox::inside(&parent.path().join("not-here")) + .err() + .unwrap(); + + assert!(error.contains("could not create a sandbox directory")); + } + + #[test] + fn a_sandbox_whose_world_name_is_taken_names_the_directory() { + let root = TempDir::new().unwrap(); + fs::write(root.path().join("world"), "x").unwrap(); + + let error = Sandbox::under(root).err().unwrap(); + + assert!(error.contains("world")); + assert!(error.contains("could not create")); + } } diff --git a/src/play/screen.rs b/src/play/screen.rs index 5f7c3e4..54c432b 100644 --- a/src/play/screen.rs +++ b/src/play/screen.rs @@ -121,7 +121,9 @@ pub struct SessionText<'a> { /// viewport as they stream, into the terminal's own scrollback, so they /// survive the game. `history` holds the prompts the player can recall /// with Up and Down. Escape while a turn is running sets `worker.cancel`, -/// which the worker checks to stop the turn. +/// which the worker checks to stop the turn. Ctrl+C does the same while a +/// turn runs, so a reply the player no longer wants costs the turn and +/// nothing else; with no turn running it ends the game. /// /// `guard` brackets every render pass in a synchronized update, so the /// terminal paints each pass in one go instead of painting whatever has @@ -193,8 +195,11 @@ pub fn play>( Some(Key::Newline) => screen.input.insert('\n'), Some(Key::Paste(text)) => screen.input.insert_str(&text), Some(Key::Enter) => screen.submit(terminal, worker, text, history, guard, viewport)?, - Some(Key::Cancel) if screen.busy => worker.cancel.store(true, Ordering::Relaxed), + Some(Key::Cancel | Key::Interrupt) if screen.busy => { + worker.cancel.store(true, Ordering::Relaxed); + } Some(Key::Cancel) => {} + Some(Key::Interrupt) => return Ok(()), Some(Key::Tab) => tab_complete(&mut screen), Some(Key::Redraw) => redraw(&screen, terminal, guard, viewport)?, } @@ -210,7 +215,7 @@ fn fit>( ) -> Result<(), B::Error> { let size = terminal.size()?; let forming = screen.transcript.forming_rows(size.width).len(); - let floor = floor(screen.rows, screen.transcript.waiting(), size); + let floor = floor(screen.rows, screen.transcript.waiting(), size, screen.busy); let wanted = wanted_rows(&screen.input, size, forming).max(floor); if wanted == screen.rows { return Ok(()); @@ -250,18 +255,33 @@ fn redraw>( Ok(()) } -/// The fewest rows the viewport may hold now, with `rows` of them now and -/// `waiting` rows of the transcript about to go out. +/// The fewest rows the viewport may hold now, with `rows` of them now, +/// `waiting` rows of the transcript about to go out, and a turn running or +/// not. /// /// A viewport gives its rows back below itself, and the rows the /// transcript puts out above it are what push it back down to the bottom -/// of the screen. It may give back no more of them than those rows fill, -/// so a shrink with nothing to write waits: the viewport keeps the rows, -/// blank, until rows go out that fill them. The cap comes first, though. -/// A screen with no room for the rows the viewport is holding takes them -/// back whether or not anything fills them, because a viewport past the -/// cap leaves the transcript nothing. -fn floor(rows: u16, waiting: u16, size: Size) -> u16 { +/// of the screen. While a turn runs it may give back no more of them than +/// those rows fill, so a shrink with nothing to write waits: the viewport +/// keeps the rows, blank, until rows go out that fill them. More rows are +/// coming, and they arrive within a turn. +/// +/// Between turns nothing is coming. A floor there would hold the rows a +/// prompt gave up, blank, until the next reply: a ten-row paste the +/// player then deletes would keep half a small screen for as long as it +/// took to type the next line. The viewport gives every row back at once +/// instead, and the resize clears them, so nothing of the prompt is left +/// under it. The prompt stops short of the last row until the next rows +/// the transcript writes land in those rows and push it back down, which +/// is the price of giving them back early. +/// +/// The cap comes first either way. A screen with no room for the rows the +/// viewport is holding takes them back whether or not anything fills +/// them, because a viewport past the cap leaves the transcript nothing. +fn floor(rows: u16, waiting: u16, size: Size, busy: bool) -> u16 { + if !busy && waiting == 0 { + return 0; + } rows.saturating_sub(waiting).min(cap(size)) } @@ -379,10 +399,16 @@ impl Screen { guard: &mut G, viewport: &mut V, ) -> Result<(), B::Error> { - if self.busy || self.input.text().trim().is_empty() { + // One trim answers every question the rest of this asks: whether + // the line says anything, whether it names a slash command, and + // what the DM reads. A command the player ended with a space is + // still that command, not a turn. The take that follows clears + // the prompt and gives nothing back that this does not have. + let input = self.input.text().trim().to_string(); + if self.busy || input.is_empty() { return Ok(()); } - let input = self.input.take(); + self.input.take(); // Slash commands stay local. /context shows the DM's system prompt. if input == "/context" { @@ -415,11 +441,15 @@ impl Screen { } } -/// If the input starts with `/`, cycles through matching slash commands. -/// Otherwise does nothing. +/// If the input is a single line starting with `/`, cycles through +/// matching slash commands. Otherwise does nothing. +/// +/// A completion replaces the whole input, so an input of several rows +/// whose first character happens to be `/` is not a command name and Tab +/// leaves it alone rather than throwing the rest of it away. fn tab_complete(screen: &mut Screen) { let text = screen.input.text(); - if !text.starts_with('/') { + if !text.starts_with('/') || text.contains('\n') { return; } // Find the next command whose prefix matches what's been typed so far. diff --git a/src/play/screen_pinned_tests.rs b/src/play/screen_pinned_tests.rs index 22f9302..9868436 100644 --- a/src/play/screen_pinned_tests.rs +++ b/src/play/screen_pinned_tests.rs @@ -30,11 +30,12 @@ use super::*; /// terminal opens on the screen and the scrollback the old one left, the /// way stdout leaves them for the real one. /// -/// The row arithmetic is [`crate::play::viewport`]'s, the same functions -/// the real one calls, so a screen this leaves behind is the screen a -/// terminal is left holding. Recording the heights and the resizes the -/// way `RecordingViewport` does lets the harness report both the same -/// way, whichever fake a test runs on. +/// The rows every write goes to come from [`viewport::Move`], which is +/// where the real one gets them too, so a screen this leaves behind is +/// the screen a terminal is left holding and neither can drift from the +/// other. Recording the heights and the resizes the way +/// `RecordingViewport` does lets the harness report both the same way, +/// whichever fake a test runs on. pub(super) struct PinnedViewport { pub(super) rows: Rc>>, pub(super) timeline: Rc>>, @@ -51,8 +52,7 @@ impl ViewportRows for PinnedViewport { self.timeline.borrow_mut().push(Recorded::Resize); let old_top = terminal.get_frame().area().y; let height = terminal.size()?.height; - let target = viewport::next_top(height, rows, old_top); - move_viewport(terminal, rows, old_top, target) + move_viewport(terminal, viewport::Move::resize(height, rows, old_top)) } /// Changes the size of the screen first, when a `Resize` step asked @@ -70,8 +70,7 @@ impl ViewportRows for PinnedViewport { self.timeline.borrow_mut().push(Recorded::Resize); let old_top = terminal.get_frame().area().y; let height = terminal.size()?.height; - let target = viewport::pinned_top(height, rows); - move_viewport(terminal, rows, old_top, target) + move_viewport(terminal, viewport::Move::repin(height, rows, old_top)) } } @@ -79,29 +78,25 @@ impl ViewportRows for PinnedViewport { /// keys fake, which is where a test schedules one. pub(super) type Resized = Rc>>; -/// Puts a viewport of `rows` rows on `target`, with its old top on -/// `old_top`, the way `terminal::move_viewport` does on a real screen. +/// Writes `plan` to a `TestBackend`, in order, the way +/// `terminal::move_viewport` writes it to a real screen. fn move_viewport( terminal: &mut Terminal, - rows: u16, - old_top: u16, - target: u16, + plan: viewport::Move, ) -> Result<(), Infallible> { let height = terminal.size()?.height; - let scroll = viewport::scroll_up_needed(old_top, target); let backend = terminal.backend_mut(); - if scroll > 0 { - backend.scroll_region_up(0..height, scroll)?; + if plan.scroll > 0 { + backend.scroll_region_up(0..height, plan.scroll)?; } - let clear = viewport::clear_from(old_top.saturating_sub(scroll), target); - backend.set_cursor_position(Position::new(0, clear))?; + backend.set_cursor_position(Position::new(0, plan.clear))?; backend.clear_region(ClearType::AfterCursor)?; - backend.set_cursor_position(Position::new(0, target))?; + backend.set_cursor_position(Position::new(0, plan.top))?; let backend = backend.clone(); *terminal = Terminal::with_options( backend, TerminalOptions { - viewport: Viewport::Inline(rows), + viewport: Viewport::Inline(plan.rows), }, )?; Ok(()) @@ -226,13 +221,38 @@ fn the_transcript_reaches_the_viewport_while_a_block_is_still_forming() { } #[test] -fn the_prompt_stays_on_the_last_row_when_the_input_gives_a_row_back() { +fn the_row_the_input_gives_back_leaves_nothing_behind_it() { let mut steps = typing(&"ab".repeat(20)); steps.push(press(Key::Backspace)); steps.push(press(Key::Backspace)); let mut played = play_pinned(ROOMY, steps); - assert_eq!(played.viewport_bottom(), ROOMY); + // The viewport keeps its top and ends one row short of the last row, + // and the row it gave back holds none of the prompt it used to. + assert_eq!(played.viewport_bottom(), ROOMY - 1); + assert_eq!(played.row(ROOMY - 1), (String::new(), false)); assert_eq!(played.printed(), ["a banner"]); } + +#[test] +fn the_line_the_player_submits_takes_back_the_row_the_prompt_gave_up() { + let mut steps = typing(&"ab".repeat(20)); + steps.push(press(Key::Backspace)); + steps.push(press(Key::Backspace)); + steps.push(press(Key::Enter)); + + let mut played = play_pinned(ROOMY, steps); + + // The blank row is the one the transcript puts between two blocks of + // different kinds, the banner and what the player said. + assert_eq!(played.viewport_bottom(), ROOMY); + assert_eq!( + played.printed(), + [ + "a banner".to_string(), + String::new(), + format!("> {}", "ab".repeat(19)), + ] + ); +} diff --git a/src/play/screen_prompt_tests.rs b/src/play/screen_prompt_tests.rs index dd67b40..c42a947 100644 --- a/src/play/screen_prompt_tests.rs +++ b/src/play/screen_prompt_tests.rs @@ -82,30 +82,33 @@ fn an_input_that_wraps_to_two_rows_asks_for_one_taller_viewport() { } #[test] -fn deleting_back_to_one_row_keeps_the_taller_viewport_until_a_row_goes_out() { +fn deleting_back_to_one_row_gives_the_row_back_between_turns() { let mut steps = typing(&"ab".repeat(20)); steps.push(press(Key::Backspace)); steps.push(press(Key::Backspace)); let played = play_script_on(12, steps); - // The row the prompt no longer needs goes back below the viewport, - // where a row of the transcript has to land to push the prompt down - // to the bottom of the screen again. Nothing is waiting to go out - // here, so the viewport holds the row, blank, instead. - assert_eq!(played.requested(), vec![6]); + // No turn is running and nothing waits to go out, so no rows are + // coming to fill the row the prompt no longer needs. The viewport + // gives it back now rather than holding it blank until the next + // reply. + assert_eq!(played.requested(), vec![6, 5]); } #[test] -fn the_line_the_player_submits_takes_back_the_row_the_prompt_gave_up() { - let mut steps = typing(&"ab".repeat(20)); +fn deleting_back_to_one_row_keeps_the_taller_viewport_while_a_turn_runs() { + let mut steps = typing("hi"); + steps.push(press(Key::Enter)); + steps.extend(typing(&"ab".repeat(20))); steps.push(press(Key::Backspace)); steps.push(press(Key::Backspace)); - steps.push(press(Key::Enter)); let played = play_script_on(12, steps); - assert_eq!(played.requested(), vec![6, 5]); + // The reply is still arriving, so the rows it writes are what the + // row the prompt gave up waits for. + assert_eq!(played.requested(), vec![6]); } #[test] diff --git a/src/play/screen_slash_tests.rs b/src/play/screen_slash_tests.rs index ce29ed3..8d07ebe 100644 --- a/src/play/screen_slash_tests.rs +++ b/src/play/screen_slash_tests.rs @@ -2,7 +2,7 @@ //! instead of going to the worker, and Tab completing command names on //! the prompt. The harness lives in `screen_tests.rs`. -use super::tests::{SLASH_CONTEXT, play_script, press, typing}; +use super::tests::{SLASH_CONTEXT, play_script, play_script_on, press, typing}; use super::*; #[test] @@ -36,6 +36,17 @@ fn slash_context_clears_the_prompt_for_the_next_input() { assert_eq!(played.prompt(), ">"); } +#[test] +fn a_trailing_space_still_names_the_command() { + let mut steps = typing("/context "); + steps.push(press(Key::Enter)); + + let played = play_script(steps); + + assert!(played.transcript().contains(SLASH_CONTEXT)); + assert!(played.nothing_submitted()); +} + #[test] fn tab_completes_a_slash_command_prefix() { let mut steps = typing("/c"); @@ -75,3 +86,15 @@ fn tab_leaves_an_unknown_slash_prefix_alone() { assert_eq!(played.prompt(), "> /x"); } + +#[test] +fn tab_leaves_an_input_of_several_rows_alone() { + let steps = vec![ + press(Key::Paste("/c\nthe rest of it".to_string())), + press(Key::Tab), + ]; + + let played = play_script_on(10, steps); + + assert_eq!(played.prompt(), " the rest of it"); +} diff --git a/src/play/screen_tail_tests.rs b/src/play/screen_tail_tests.rs index c6dbe2e..d6b2906 100644 --- a/src/play/screen_tail_tests.rs +++ b/src/play/screen_tail_tests.rs @@ -75,7 +75,7 @@ fn a_finished_reply_empties_the_tail() { /// Whether `c` is one of the sparkle glyphs the thinking indicator /// pulses through. fn is_sparkle(c: char) -> bool { - c == '\u{B7}' || ('\u{2726}'..='\u{2739}').contains(&c) + thinking::SPARKS.contains(&c) } #[test] diff --git a/src/play/screen_turn_tests.rs b/src/play/screen_turn_tests.rs index 75caaa4..fef8a87 100644 --- a/src/play/screen_turn_tests.rs +++ b/src/play/screen_turn_tests.rs @@ -321,6 +321,46 @@ fn escape_while_idle_does_nothing() { assert!(!played.cancel.load(Ordering::Relaxed)); } +#[test] +fn control_c_mid_turn_sets_the_cancel_flag() { + let mut steps = typing("hi"); + steps.push(press(Key::Enter)); + steps.push(press(Key::Interrupt)); + + let played = play_script(steps); + + assert!(played.cancel.load(Ordering::Relaxed)); +} + +#[test] +fn control_c_mid_turn_leaves_the_game_running() { + let mut steps = typing("hi"); + steps.push(press(Key::Enter)); + steps.push(press(Key::Interrupt)); + steps.push(Step::Turn(TurnEvent::Cancelled("You wa".to_string()))); + steps.extend(typing("again")); + steps.push(press(Key::Enter)); + + let played = play_script(steps); + + assert_eq!(played.submitted(), "hi"); + assert_eq!(played.submitted(), "again"); +} + +#[test] +fn control_c_while_idle_ends_the_game() { + let mut steps = typing("hi"); + steps.push(press(Key::Interrupt)); + steps.extend(typing("more")); + + let played = play_script(steps); + + // The loop draws before it reads a key, so the prompt shows what the + // player had typed when Ctrl+C arrived, and nothing after it. + assert_eq!(played.prompt(), "> hi"); + assert!(!played.cancel.load(Ordering::Relaxed)); +} + #[test] fn a_cancelled_turn_shows_the_partial_text_then_the_interrupted_aside() { let mut steps = typing("hi"); diff --git a/src/play/terminal.rs b/src/play/terminal.rs index 82e259e..793e110 100644 --- a/src/play/terminal.rs +++ b/src/play/terminal.rs @@ -206,8 +206,10 @@ impl ViewportRows> for CrosstermViewport { ) -> io::Result<()> { let old_top = terminal.get_frame().area().y; let screen_height = terminal.size()?.height; - let target = viewport::next_top(screen_height, rows, old_top); - move_viewport(terminal, rows, old_top, target) + move_viewport( + terminal, + viewport::Move::resize(screen_height, rows, old_top), + ) } /// Puts the viewport back on the last row of the screen, wherever a @@ -225,19 +227,20 @@ impl ViewportRows> for CrosstermViewport { ) -> io::Result<()> { let old_top = terminal.get_frame().area().y; let screen_height = terminal.size()?.height; - let target = viewport::pinned_top(screen_height, rows); - move_viewport(terminal, rows, old_top, target) + move_viewport( + terminal, + viewport::Move::repin(screen_height, rows, old_top), + ) } } -/// Puts a viewport of `rows` rows on `target`, with its old top on -/// `old_top`, and builds the terminal that draws it. +/// Writes `plan` to stdout, in order, and builds the terminal that draws +/// the viewport it leaves behind. /// -/// Rows above `target` that content still owns scroll up out of the way -/// first, the same as a program whose output ran past the last row, so -/// they go into the scrollback intact rather than getting overwritten. -/// The clear then takes every row the viewport holds now, and every row -/// it held before, off the screen. +/// The scroll is crossterm's `ScrollUp`, which the terminal treats the +/// same as a program whose output ran past the last row, so the rows it +/// takes off the top go into the scrollback intact rather than getting +/// overwritten. /// /// `Terminal::with_options` reads the cursor back from the row this just /// set and reserves the new viewport's height below it; since that row @@ -259,26 +262,22 @@ impl ViewportRows> for CrosstermViewport { /// as any other I/O failure here. fn move_viewport( terminal: &mut Terminal>, - rows: u16, - old_top: u16, - target: u16, + plan: viewport::Move, ) -> io::Result<()> { - let scroll = viewport::scroll_up_needed(old_top, target); let mut stdout = io::stdout(); - if scroll > 0 { - execute!(stdout, ScrollUp(scroll))?; + if plan.scroll > 0 { + execute!(stdout, ScrollUp(plan.scroll))?; } - let clear = viewport::clear_from(old_top.saturating_sub(scroll), target); execute!( stdout, - MoveTo(0, clear), + MoveTo(0, plan.clear), Clear(ClearType::FromCursorDown), - MoveTo(0, target) + MoveTo(0, plan.top) )?; *terminal = Terminal::with_options( CrosstermBackend::new(io::stdout()), TerminalOptions { - viewport: Viewport::Inline(rows), + viewport: Viewport::Inline(plan.rows), }, )?; Ok(()) diff --git a/src/play/thinking.rs b/src/play/thinking.rs index 2f5578e..277a426 100644 --- a/src/play/thinking.rs +++ b/src/play/thinking.rs @@ -8,7 +8,7 @@ /// The sparkle pulse, one glyph per animation frame, dim to bright and /// back. Dice glyphs are reserved for real rolls when the dice tool /// arrives. -const SPARKS: [char; 8] = ['·', '✧', '✦', '✶', '✷', '✶', '✦', '✧']; +pub const SPARKS: [char; 8] = ['·', '✧', '✦', '✶', '✷', '✶', '✦', '✧']; /// The phrase shown while the DM is narrating. pub const STORYTELLING: &str = "storytelling"; diff --git a/src/play/viewport.rs b/src/play/viewport.rs index b3b5010..2ea7ae7 100644 --- a/src/play/viewport.rs +++ b/src/play/viewport.rs @@ -60,6 +60,69 @@ pub fn clear_from(current_top: u16, pinned_top: u16) -> u16 { current_top.min(pinned_top) } +/// The writes one change of the viewport's height takes, in the order +/// they go out. +/// +/// An implementation of [`ViewportRows`] does these three writes and then +/// builds the new terminal, and nothing else decides how far to scroll, +/// where to clear from, or where the new top belongs. The test double and +/// the real terminal both take their rows from here, so a screen a test +/// leaves behind is the screen a terminal is left holding. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Move { + /// How many rows to scroll the screen up first. Zero means the screen + /// stays where it is. + pub scroll: u16, + /// The row to clear from, down to the last row of the screen. + pub clear: u16, + /// The row the new viewport's top lands on. + pub top: u16, + /// How many rows the new viewport holds. + pub rows: u16, +} + +impl Move { + /// The move that gives the viewport `rows` rows on a screen + /// `screen_height` rows tall, with its top on `current_top` now. + /// + /// The top follows [`next_top`], so a viewport that grows takes rows + /// from the transcript above and one that shrinks gives its rows back + /// below itself. + pub fn resize(screen_height: u16, rows: u16, current_top: u16) -> Self { + Self::to( + next_top(screen_height, rows, current_top), + rows, + current_top, + ) + } + + /// The move that puts a viewport of `rows` rows back on the last row + /// of a screen `screen_height` rows tall, with its top on + /// `current_top` now. + /// + /// The top follows [`pinned_top`], so the viewport lands flush with + /// the last row whether that moves it up or down. + pub fn repin(screen_height: u16, rows: u16, current_top: u16) -> Self { + Self::to(pinned_top(screen_height, rows), rows, current_top) + } + + /// The move from `current_top` to `target`, with the scroll and the + /// clear that take it there. + /// + /// The scroll comes first, and it carries the old top up with the + /// rest of the screen, so the clear starts from where that row sits + /// after the scroll rather than from where it sat before. + fn to(target: u16, rows: u16, current_top: u16) -> Self { + let scroll = scroll_up_needed(current_top, target); + Self { + scroll, + clear: clear_from(current_top.saturating_sub(scroll), target), + top: target, + rows, + } + } +} + /// Changes the height of the inline viewport pinned to the bottom of the /// screen. /// @@ -81,10 +144,14 @@ pub fn clear_from(current_top: u16, pinned_top: u16) -> u16 { /// ran past the last row. Shrinking leaves that row where it is and /// ends the viewport short of the last row of the screen, so clear from /// it down to take the rows the viewport gave back off the screen. +/// [`Move`] works out all three rows, so an implementation does its +/// writes in order and decides none of them itself. /// - A viewport that shrank stops short of the bottom of the screen until /// the rows the transcript puts above it push it back down. Ask for a -/// shrink only with rows waiting to go out, and put them out before the -/// next draw, or the prompt sits off the bottom of the screen. +/// shrink with rows waiting to go out, and put them out before the next +/// draw, and the prompt stays on the last row. Ask for one with nothing +/// waiting and the prompt sits off the bottom of the screen until rows +/// go out that fill the rows the viewport gave back. /// - Every resize costs exactly one cursor-position query, `ESC [ 6 n`, /// and crossterm blocks up to two seconds for the reply. Call this only /// when the number of rows really changes, never on every keystroke, diff --git a/src/play/viewport_tests.rs b/src/play/viewport_tests.rs index a5882a8..8a5df30 100644 --- a/src/play/viewport_tests.rs +++ b/src/play/viewport_tests.rs @@ -2,7 +2,7 @@ //! bottom of the screen: where its top belongs, and how much of the //! screen above has to scroll to get there without overwriting anything. -use super::{clear_from, next_top, pinned_top, scroll_up_needed}; +use super::{Move, clear_from, next_top, pinned_top, scroll_up_needed}; #[test] fn a_viewport_shorter_than_the_screen_sits_flush_with_the_last_row() { @@ -58,3 +58,55 @@ fn a_viewport_that_moves_down_clears_from_the_row_it_came_from() { fn a_viewport_that_moves_up_clears_from_the_row_it_lands_on() { assert_eq!(clear_from(15, 5), 5); } + +#[test] +fn a_resize_that_grows_scrolls_the_screen_and_clears_from_the_row_it_lands_on() { + assert_eq!( + Move::resize(24, 6, 20), + Move { + scroll: 2, + clear: 18, + top: 18, + rows: 6 + } + ); +} + +#[test] +fn a_resize_that_shrinks_keeps_its_top_and_clears_the_rows_below_it() { + assert_eq!( + Move::resize(24, 4, 18), + Move { + scroll: 0, + clear: 18, + top: 18, + rows: 4 + } + ); +} + +#[test] +fn a_repin_that_moves_down_clears_from_the_row_it_came_from() { + assert_eq!( + Move::repin(24, 4, 18), + Move { + scroll: 0, + clear: 18, + top: 20, + rows: 4 + } + ); +} + +#[test] +fn a_repin_that_moves_up_scrolls_the_screen_by_as_much_as_the_top_moves() { + assert_eq!( + Move::repin(24, 8, 20), + Move { + scroll: 4, + clear: 16, + top: 16, + rows: 8 + } + ); +} diff --git a/src/srd/fetch.rs b/src/srd/fetch.rs index f13f5f5..b8ad286 100644 --- a/src/srd/fetch.rs +++ b/src/srd/fetch.rs @@ -476,7 +476,7 @@ fn download(url: &str) -> Result, FetchError> { .map_err(|error| FetchError::Download(error.to_string())) } -fn sha256_hex(bytes: &[u8]) -> String { +pub(crate) fn sha256_hex(bytes: &[u8]) -> String { let mut hasher = Sha256::new(); hasher.update(bytes); hasher @@ -489,94 +489,10 @@ fn sha256_hex(bytes: &[u8]) -> String { #[cfg(test)] mod tests { use super::*; - use std::io::{Read, Write}; - use std::net::TcpListener; - use std::thread::JoinHandle; + use crate::srd::testing::{append_entry, fixture_meta, serve_once, tiny_tar_gz}; + use std::io::Write; use tempfile::TempDir; - /// Serves `body` once to the first connection it accepts, as a complete - /// HTTP/1.1 response, then returns the URL to fetch it from. - fn serve_once(body: Vec) -> (String, JoinHandle<()>) { - let listener = TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let handle = std::thread::spawn(move || { - let (mut stream, _) = listener.accept().unwrap(); - let mut request = [0u8; 1024]; - let _ = stream.read(&mut request).unwrap(); - let header = format!( - "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", - body.len() - ); - stream.write_all(header.as_bytes()).unwrap(); - stream.write_all(&body).unwrap(); - }); - (format!("http://{addr}/fixture.pdf"), handle) - } - - /// Appends a tar entry for `path` to `builder`, as a directory when - /// `contents` is `None` or as a file with that content otherwise. - fn append_entry(builder: &mut tar::Builder>, path: &str, contents: Option<&[u8]>) { - let mut header = tar::Header::new_gnu(); - header.set_path(path).unwrap(); - header.set_mode(0o644); - match contents { - Some(bytes) => { - header.set_size(bytes.len() as u64); - header.set_cksum(); - builder.append(&header, bytes).unwrap(); - } - None => { - header.set_size(0); - header.set_entry_type(tar::EntryType::Directory); - header.set_cksum(); - builder.append(&header, io::empty()).unwrap(); - } - } - } - - /// Builds a tiny gzipped tarball with one top-level directory (`top_dir`, - /// itself stored as a bare directory entry), shaped like the real - /// dnd.srd.5.2.1 repo: a numbered content directory and a top-level - /// licensing file that `should_vendor` keeps, alongside `.github/`, - /// `docs_original/`, and an unlisted top-level file that it drops. - fn tiny_tar_gz(top_dir: &str) -> Vec { - let mut builder = tar::Builder::new(Vec::new()); - append_entry(&mut builder, &format!("{top_dir}/"), None); - append_entry(&mut builder, &format!("{top_dir}/07_Spells/"), None); - append_entry( - &mut builder, - &format!("{top_dir}/07_Spells/Fireball.md"), - Some(b"# Fireball\n"), - ); - append_entry( - &mut builder, - &format!("{top_dir}/Legal.md"), - Some(b"CC-BY-4.0\n"), - ); - append_entry(&mut builder, &format!("{top_dir}/.github/"), None); - append_entry( - &mut builder, - &format!("{top_dir}/.github/FUNDING.yml"), - Some(b"github: []\n"), - ); - append_entry(&mut builder, &format!("{top_dir}/docs_original/"), None); - append_entry( - &mut builder, - &format!("{top_dir}/docs_original/dnd_srd_5.2.1_cc.pdf"), - Some(b"%PDF-fixture"), - ); - append_entry( - &mut builder, - &format!("{top_dir}/SRD-reForged.png"), - Some(b"\x89PNG-fixture"), - ); - let tar_bytes = builder.into_inner().unwrap(); - - let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); - encoder.write_all(&tar_bytes).unwrap(); - encoder.finish().unwrap() - } - /// Builds a tarball where a file entry (`07_Spells`) is later reused as /// a directory prefix (`07_Spells/b.md`), so unpacking it hits a real /// filesystem collision when it tries to create `07_Spells` as a @@ -659,11 +575,11 @@ mod tests { } /// Builds a tarball with a symlink entry (`07_Spells/x`, targeting - /// `/tmp`) followed by a regular file entry whose path walks through - /// that symlink (`07_Spells/x/evil.md`), the way a malicious upstream - /// commit could try to write outside the unpack destination via a - /// symlinked directory rather than a `..` path component. - fn tar_gz_with_a_symlink_escape(top_dir: &str) -> Vec { + /// `symlink_target`) followed by a regular file entry whose path walks + /// through that symlink (`07_Spells/x/evil.md`), the way a malicious + /// upstream commit could try to write outside the unpack destination + /// via a symlinked directory rather than a `..` path component. + fn tar_gz_with_a_symlink_escape(top_dir: &str, symlink_target: &Path) -> Vec { let mut builder = tar::Builder::new(Vec::new()); append_entry(&mut builder, &format!("{top_dir}/"), None); append_entry(&mut builder, &format!("{top_dir}/07_Spells/"), None); @@ -673,7 +589,7 @@ mod tests { .set_path(format!("{top_dir}/07_Spells/x")) .unwrap(); symlink_header.set_entry_type(tar::EntryType::Symlink); - symlink_header.set_link_name("/tmp").unwrap(); + symlink_header.set_link_name(symlink_target).unwrap(); symlink_header.set_mode(0o777); symlink_header.set_size(0); symlink_header.set_cksum(); @@ -691,17 +607,18 @@ mod tests { encoder.finish().unwrap() } - fn fixture_meta() -> SrdMeta { - SrdMeta { - version: "5.2.1".to_string(), - pdf_path: "sources/SRD_CC_v5.2.1.pdf".to_string(), - pdf_url: "https://example.test/srd.pdf".to_string(), - pdf_sha256: "a".repeat(64), - text_path: "sources/dnd.srd.5.2.1/".to_string(), - text_url: "https://example.test/dnd.srd.5.2.1".to_string(), - text_commit: "deadbeef".to_string(), - text_tarball_sha256: "b".repeat(64), - } + #[test] + fn sha256_hex_matches_the_published_fips_180_vector_for_abc() { + // Every other test in this module computes its expected hash by + // calling `sha256_hex` itself, so a bug that truncates or + // otherwise corrupts the hex encoding would pass all of them. + // This pins one published test vector as a literal, so the + // function's actual output is checked against a value nothing in + // this file derived. + assert_eq!( + sha256_hex(b"abc"), + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad" + ); } #[test] @@ -1279,7 +1196,13 @@ mod tests { #[test] fn fetch_text_rejects_a_symlink_entry() { - let body = tar_gz_with_a_symlink_escape("dnd.srd.5.2.1-deadbeef"); + // The symlink targets a directory under this test's own temp root, + // rather than a fixed path like `/tmp`, so the assertion below + // checks the actual destination of a real escape instead of a + // path an unrelated file could already occupy, or a real escape + // could land outside of. + let escape_dir = TempDir::new().unwrap(); + let body = tar_gz_with_a_symlink_escape("dnd.srd.5.2.1-deadbeef", escape_dir.path()); let expected = sha256_hex(&body); let (url, server) = serve_once(body); let destination_dir = TempDir::new().unwrap(); @@ -1297,7 +1220,7 @@ mod tests { assert!(matches!(error, FetchError::UnsafeEntry(_))); assert!(error.to_string().contains("07_Spells/x")); assert!(error.to_string().contains("Symlink")); - assert!(!Path::new("/tmp/evil.md").exists()); + assert!(!escape_dir.path().join("evil.md").exists()); server.join().unwrap(); } diff --git a/src/srd/mod.rs b/src/srd/mod.rs index 65c2944..1f6dac2 100644 --- a/src/srd/mod.rs +++ b/src/srd/mod.rs @@ -1,4 +1,6 @@ pub mod fetch; +#[cfg(test)] +pub(crate) mod testing; pub mod verify; /// Where the SRD 5.2 corpus layer lives, relative to the repository root. diff --git a/src/srd/testing.rs b/src/srd/testing.rs new file mode 100644 index 0000000..e680f8c --- /dev/null +++ b/src/srd/testing.rs @@ -0,0 +1,112 @@ +//! Test-support fixtures shared by srd's own tests and by anything else +//! in the crate that exercises `fetch`: a loopback HTTP server that +//! serves one canned response, tarball builders for that server to +//! serve, and a fixture `SrdMeta`. + +use std::io::{Read, Write}; +use std::net::TcpListener; +use std::thread::JoinHandle; + +use super::fetch::SrdMeta; + +/// Serves `body` once to the first connection it accepts, as a complete +/// HTTP/1.1 response, then returns the URL to fetch it from. +pub(crate) fn serve_once(body: Vec) -> (String, JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0u8; 1024]; + let _ = stream.read(&mut request).unwrap(); + let header = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + stream.write_all(header.as_bytes()).unwrap(); + stream.write_all(&body).unwrap(); + }); + (format!("http://{addr}/fixture.pdf"), handle) +} + +/// Appends a tar entry for `path` to `builder`, as a directory when +/// `contents` is `None` or as a file with that content otherwise. +pub(crate) fn append_entry( + builder: &mut tar::Builder>, + path: &str, + contents: Option<&[u8]>, +) { + let mut header = tar::Header::new_gnu(); + header.set_path(path).unwrap(); + header.set_mode(0o644); + match contents { + Some(bytes) => { + header.set_size(bytes.len() as u64); + header.set_cksum(); + builder.append(&header, bytes).unwrap(); + } + None => { + header.set_size(0); + header.set_entry_type(tar::EntryType::Directory); + header.set_cksum(); + builder.append(&header, std::io::empty()).unwrap(); + } + } +} + +/// Builds a tiny gzipped tarball with one top-level directory (`top_dir`, +/// itself stored as a bare directory entry), shaped like the real +/// dnd.srd.5.2.1 repo: a numbered content directory and a top-level +/// licensing file that `should_vendor` keeps, alongside `.github/`, +/// `docs_original/`, and an unlisted top-level file that it drops. +pub(crate) fn tiny_tar_gz(top_dir: &str) -> Vec { + let mut builder = tar::Builder::new(Vec::new()); + append_entry(&mut builder, &format!("{top_dir}/"), None); + append_entry(&mut builder, &format!("{top_dir}/07_Spells/"), None); + append_entry( + &mut builder, + &format!("{top_dir}/07_Spells/Fireball.md"), + Some(b"# Fireball\n"), + ); + append_entry( + &mut builder, + &format!("{top_dir}/Legal.md"), + Some(b"CC-BY-4.0\n"), + ); + append_entry(&mut builder, &format!("{top_dir}/.github/"), None); + append_entry( + &mut builder, + &format!("{top_dir}/.github/FUNDING.yml"), + Some(b"github: []\n"), + ); + append_entry(&mut builder, &format!("{top_dir}/docs_original/"), None); + append_entry( + &mut builder, + &format!("{top_dir}/docs_original/dnd_srd_5.2.1_cc.pdf"), + Some(b"%PDF-fixture"), + ); + append_entry( + &mut builder, + &format!("{top_dir}/SRD-reForged.png"), + Some(b"\x89PNG-fixture"), + ); + let tar_bytes = builder.into_inner().unwrap(); + + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + encoder.write_all(&tar_bytes).unwrap(); + encoder.finish().unwrap() +} + +/// A fixture `SrdMeta`, standing in for the pinned SRD 5.2.1 record with +/// values a test can distinguish from the real thing at a glance. +pub(crate) fn fixture_meta() -> SrdMeta { + SrdMeta { + version: "5.2.1".to_string(), + pdf_path: "sources/SRD_CC_v5.2.1.pdf".to_string(), + pdf_url: "https://example.test/srd.pdf".to_string(), + pdf_sha256: "a".repeat(64), + text_path: "sources/dnd.srd.5.2.1/".to_string(), + text_url: "https://example.test/dnd.srd.5.2.1".to_string(), + text_commit: "deadbeef".to_string(), + text_tarball_sha256: "b".repeat(64), + } +} diff --git a/src/srd/verify/fidelity.rs b/src/srd/verify/fidelity.rs index 629f2fd..7beb3dc 100644 --- a/src/srd/verify/fidelity.rs +++ b/src/srd/verify/fidelity.rs @@ -387,8 +387,13 @@ mod tests { let source = PathBuf::from("sources/dnd.srd.5.2.1/11_Monsters/Monsters_Each/Rat.md"); let source_text = "# Rat\n\n*Small Beast, Unaligned*\n\n**AC** 10\n\n**HP** 1 (1d4)\n\n**Speed** 5 ft.\n\n| | MOD | SAVE | | MOD | SAVE | | MOD | SAVE |\n| :- | :- | :- | :- | :- | :- | :- | :- | :- |\n| **Str 1** | +0 | +0 | **Dex 1** | +0 | +0 | **Con 1** | +0 | +0 |\n| **Int 1** | +0 | +0 | **Wis 1** | +0 | +0 | **Cha 1** | +0 | +0 |\n\n**CR** 0\n\n## Actions\n\nBite.\n"; write_file(&layer_root.path().join(&source), source_text); - let body = stat_block::source_outside_region(source_text); - let file = corpus_file(Kind::Monster, &body); + // `source_text` with its stat-block region cut, frozen as a literal + // rather than computed by calling `stat_block::source_outside_region` + // itself, so a region-boundary bug in that function shows up as a + // fidelity mismatch here instead of shifting the fixture and the + // check together. + let body = "# Rat\n\n*Small Beast, Unaligned*\n\n## Actions\n\nBite.\n"; + let file = corpus_file(Kind::Monster, body); fn ability() -> crate::srd::verify::monster::Ability { crate::srd::verify::monster::Ability { score: 1, diff --git a/src/srd/verify/fixtures.rs b/src/srd/verify/fixtures.rs index ee41afa..1bfa0ce 100644 --- a/src/srd/verify/fixtures.rs +++ b/src/srd/verify/fixtures.rs @@ -43,6 +43,12 @@ pub const ANIMAL_SOURCE: &str = "sources/dnd.srd.5.2.1/12_Animals/Animals_Each/W const FIREBALL_BODY: &str = "# Fireball\n\n*Level 3 Evocation (Sorcerer, Wizard)*\n\n**Casting Time:** Action\n\n**Range:** 150 feet\n\n**Components:** V, S, M\n\n**Duration:** Instantaneous\n\nA bright streak flashes from you.\n"; const GOBLIN_SOURCE_TEXT: &str = "# Goblin Warrior\n\n*Small Fey (Goblinoid), Chaotic Neutral*\n\n**AC** 15\n\n**HP** 10 (3d6)\n\n**Speed** 30 ft.\n\n| | MOD | SAVE | | MOD | SAVE | | MOD | SAVE |\n| :- | :- | :- | :- | :- | :- | :- | :- | :- |\n| **Str 8** | -1 | -1 | **Dex 15** | +2 | +2 | **Con 10** | +0 | +0 |\n| **Int 10** | +0 | +0 | **Wis 8** | -1 | -1 | **Cha 8** | -1 | -1 |\n\n**CR** 1/4\n\n## Actions\n\n***Scimitar.*** *Melee Attack Roll:* +4, reach 5 ft.\n"; +/// `GOBLIN_SOURCE_TEXT` with its stat-block region cut, the same way +/// `stat_block::source_outside_region` cuts it. Frozen as a literal +/// rather than computed at fixture-build time, so a region-boundary bug +/// in that function shows up as a fidelity mismatch here instead of +/// shifting the fixture and the check together. +const GOBLIN_BODY: &str = "# Goblin Warrior\n\n*Small Fey (Goblinoid), Chaotic Neutral*\n\n## Actions\n\n***Scimitar.*** *Melee Attack Roll:* +4, reach 5 ft.\n"; const AMULET_SOURCE_TEXT: &str = "# Amulet of Health\n\n*Wondrous Item, Rare (Requires Attunement)*\n\nYour Constitution is 19 while you wear this amulet.\n"; const AMULET_FIELD_LINES: &str = @@ -57,6 +63,9 @@ const SPECIES_BODY: &str = "# Elf\n\nElves are a magical people.\n"; const BACKGROUND_BODY: &str = "# Acolyte\n\nYou spent your life in service to a temple.\n"; const ANIMAL_SOURCE_TEXT: &str = "# Wolf\n\n*Medium Beast, Unaligned*\n\n**AC** 12\n\n**HP** 11 (2d8 + 2)\n\n**Speed** 40 ft.\n\n| | MOD | SAVE | | MOD | SAVE | | MOD | SAVE |\n| :- | :- | :- | :- | :- | :- | :- | :- | :- |\n| **Str 12** | +1 | +1 | **Dex 15** | +2 | +2 | **Con 12** | +1 | +1 |\n| **Int 3** | -4 | -4 | **Wis 12** | +1 | +1 | **Cha 6** | -2 | -2 |\n\n**Skills** Perception +3, Stealth +4\n\n**CR** 1/4\n\n## Actions\n\n***Bite.*** *Melee Attack Roll:* +3, reach 5 ft.\n"; +/// `ANIMAL_SOURCE_TEXT` with its stat-block region cut. See +/// `GOBLIN_BODY` for why this is a frozen literal rather than computed. +const ANIMAL_BODY: &str = "# Wolf\n\n*Medium Beast, Unaligned*\n\n## Actions\n\n***Bite.*** *Melee Attack Roll:* +3, reach 5 ft.\n"; /// A small valid corpus layer: one entry per kind, its matching fake /// vendored source, and a README and meta.yaml that satisfy the @@ -84,11 +93,10 @@ impl Layer { ); layer.write(GOBLIN_SOURCE, GOBLIN_SOURCE_TEXT); - let goblin_body = super::stat_block::source_outside_region(GOBLIN_SOURCE_TEXT); layer.write( "rules/monsters/goblin-warrior.md", &format!( - "---\nname: Goblin Warrior\ntype: monster\nsource: {GOBLIN_SOURCE}\nsize: Small\ncreature_type: Fey (Goblinoid)\nalignment: Chaotic Neutral\nac: '15'\nhp: 10 (3d6)\nspeed: 30 ft.\ncr: 1/4\nabilities:\n str: {{score: 8, mod: -1, save: -1}}\n dex: {{score: 15, mod: 2, save: 2}}\n con: {{score: 10, mod: 0, save: 0}}\n int: {{score: 10, mod: 0, save: 0}}\n wis: {{score: 8, mod: -1, save: -1}}\n cha: {{score: 8, mod: -1, save: -1}}\n---\n{goblin_body}" + "---\nname: Goblin Warrior\ntype: monster\nsource: {GOBLIN_SOURCE}\nsize: Small\ncreature_type: Fey (Goblinoid)\nalignment: Chaotic Neutral\nac: '15'\nhp: 10 (3d6)\nspeed: 30 ft.\ncr: 1/4\nabilities:\n str: {{score: 8, mod: -1, save: -1}}\n dex: {{score: 15, mod: 2, save: 2}}\n con: {{score: 10, mod: 0, save: 0}}\n int: {{score: 10, mod: 0, save: 0}}\n wis: {{score: 8, mod: -1, save: -1}}\n cha: {{score: 8, mod: -1, save: -1}}\n---\n{GOBLIN_BODY}" ), ); @@ -153,11 +161,10 @@ impl Layer { ); layer.write(ANIMAL_SOURCE, ANIMAL_SOURCE_TEXT); - let animal_body = super::stat_block::source_outside_region(ANIMAL_SOURCE_TEXT); layer.write( "rules/animals/wolf.md", &format!( - "---\nname: Wolf\ntype: animal\nsource: {ANIMAL_SOURCE}\nsize: Medium\ncreature_type: Beast\nalignment: Unaligned\nac: '12'\nhp: 11 (2d8 + 2)\nspeed: 40 ft.\ncr: 1/4\nskills: Perception +3, Stealth +4\nabilities:\n str: {{score: 12, mod: 1, save: 1}}\n dex: {{score: 15, mod: 2, save: 2}}\n con: {{score: 12, mod: 1, save: 1}}\n int: {{score: 3, mod: -4, save: -4}}\n wis: {{score: 12, mod: 1, save: 1}}\n cha: {{score: 6, mod: -2, save: -2}}\n---\n{animal_body}" + "---\nname: Wolf\ntype: animal\nsource: {ANIMAL_SOURCE}\nsize: Medium\ncreature_type: Beast\nalignment: Unaligned\nac: '12'\nhp: 11 (2d8 + 2)\nspeed: 40 ft.\ncr: 1/4\nskills: Perception +3, Stealth +4\nabilities:\n str: {{score: 12, mod: 1, save: 1}}\n dex: {{score: 15, mod: 2, save: 2}}\n con: {{score: 12, mod: 1, save: 1}}\n int: {{score: 3, mod: -4, save: -4}}\n wis: {{score: 12, mod: 1, save: 1}}\n cha: {{score: 6, mod: -2, save: -2}}\n---\n{ANIMAL_BODY}" ), ); diff --git a/src/srd/verify/mod.rs b/src/srd/verify/mod.rs index 1bcc1dc..8eaa4fa 100644 --- a/src/srd/verify/mod.rs +++ b/src/srd/verify/mod.rs @@ -288,12 +288,10 @@ mod tests { let report = verify(layer_root.path()); - assert!( - report - .failures - .iter() - .any(|f| f.message.contains("Source") || f.message.contains("Different")) - ); + assert!(report.failures.iter().any(|f| { + f.message + .contains("source has \"Source\" but the corpus body has \"Different\"") + })); } #[test] diff --git a/src/srd/verify/stat_block.rs b/src/srd/verify/stat_block.rs index 7aae683..39f39d9 100644 --- a/src/srd/verify/stat_block.rs +++ b/src/srd/verify/stat_block.rs @@ -411,11 +411,16 @@ mod tests { check_accounting(GOBLIN, &fields, &path, &mut failures); - assert!(!failures.is_empty()); - assert!( - failures - .iter() - .any(|f| f.message.contains("Skills") || f.message.contains("Senses")) + assert_eq!(failures.len(), 2); + assert_eq!( + failures[0].message, + "frontmatter 'Skills' value 'Darkvision 60 ft.; Passive Perception 9' does not \ + match the stat block's Skills segment 'Stealth +6'" + ); + assert_eq!( + failures[1].message, + "frontmatter 'Senses' value 'Stealth +6' does not match the stat block's Senses \ + segment 'Darkvision 60 ft.; Passive Perception 9'" ); } @@ -454,11 +459,16 @@ mod tests { check_accounting(GOBLIN, &fields, &path, &mut failures); - assert!(!failures.is_empty()); - assert!( - failures - .iter() - .any(|f| f.message.contains("abilities.str") || f.message.contains("abilities.dex")) + assert_eq!(failures.len(), 2); + assert_eq!( + failures[0].message, + "frontmatter abilities.str (score 15, mod +2, save +2) does not match the stat \ + block's str row (score 8, mod -1, save -1)" + ); + assert_eq!( + failures[1].message, + "frontmatter abilities.dex (score 8, mod -1, save -1) does not match the stat \ + block's dex row (score 15, mod +2, save +2)" ); } @@ -471,11 +481,14 @@ mod tests { check_accounting(GOBLIN, &fields, &path, &mut failures); - assert!(!failures.is_empty()); - assert!( - failures - .iter() - .any(|f| f.message.contains("'AC'") || f.message.contains("'HP'")) + assert_eq!(failures.len(), 2); + assert_eq!( + failures[0].message, + "frontmatter 'AC' value '10 (3d6)' does not match the stat block's AC segment '15'" + ); + assert_eq!( + failures[1].message, + "frontmatter 'HP' value '15' does not match the stat block's HP segment '10 (3d6)'" ); } diff --git a/tests/corpus.rs b/tests/corpus.rs index 13ca393..0933d75 100644 --- a/tests/corpus.rs +++ b/tests/corpus.rs @@ -101,12 +101,17 @@ fn count_md_files(kind_dir: &str) -> usize { .count() } +// These counts are exact, not floors: pinning the real committed count +// catches a corpus that loses half its entries just as well as one that +// loses one, which a "some minimum" floor would miss. Update the number +// only when the corpus is deliberately grown or trimmed. + #[test] -fn feats_directory_has_at_least_seventeen_entries() { - assert!(count_md_files("feats") >= 17); +fn feats_directory_has_exactly_seventeen_entries() { + assert_eq!(count_md_files("feats"), 17); } #[test] -fn glossary_directory_has_at_least_one_hundred_fifty_entries() { - assert!(count_md_files("glossary") >= 150); +fn glossary_directory_has_exactly_one_hundred_fifty_four_entries() { + assert_eq!(count_md_files("glossary"), 154); } -- 2.51.2