From 2a19f32237bcac762c860a8fcac16ecc5a9b962f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Nagy?= Date: Mon, 24 Jun 2024 20:50:04 +0100 Subject: [PATCH] refactor(moot): clean up error handling --- crates/kernel/testsuite/moot_suite.rs | 75 ++++++++++--------- crates/moot/src/lib.rs | 101 +++++++++++++++----------- 2 files changed, 98 insertions(+), 78 deletions(-) diff --git a/crates/kernel/testsuite/moot_suite.rs b/crates/kernel/testsuite/moot_suite.rs index 75982f8a..8b023187 100644 --- a/crates/kernel/testsuite/moot_suite.rs +++ b/crates/kernel/testsuite/moot_suite.rs @@ -20,11 +20,12 @@ mod common; use std::{path::Path, sync::Arc}; use common::{create_wiredtiger_db, testsuite_dir}; +use eyre::Context; use moor_db::Database; use moor_kernel::{ config::Config, tasks::{ - scheduler::{Scheduler, SchedulerError}, + scheduler::Scheduler, scheduler_test_utils, sessions::{NoopClientSession, Session}, }, @@ -39,7 +40,7 @@ use common::create_relbox_db; struct SchedulerMootRunner { scheduler: Arc, session: Arc, - eval_result: Option>, + eval_result: Option, } impl SchedulerMootRunner { fn new(scheduler: Arc, session: Arc) -> Self { @@ -52,28 +53,40 @@ impl SchedulerMootRunner { } impl MootRunner for SchedulerMootRunner { type Value = Var; - type Error = SchedulerError; - fn eval>(&mut self, player: Objid, command: S) -> Result<(), SchedulerError> { + fn eval>(&mut self, player: Objid, command: S) -> eyre::Result<()> { let command = command.into(); eprintln!("{player} >> ; {command}"); - self.eval_result = Some(scheduler_test_utils::call_eval( - self.scheduler.clone(), - self.session.clone(), - player, - command, - )); + self.eval_result = Some( + scheduler_test_utils::call_eval( + self.scheduler.clone(), + self.session.clone(), + player, + command.clone(), + ) + .wrap_err(format!( + "SchedulerMootRunner::eval({player}, {:?})", + command + ))?, + ); Ok(()) } - fn command>(&mut self, player: Objid, command: S) -> Result<(), SchedulerError> { - eprintln!("{player} >> ; {}", command.as_ref()); - self.eval_result = Some(scheduler_test_utils::call_command( - self.scheduler.clone(), - self.session.clone(), - player, - command.as_ref(), - )); + fn command>(&mut self, player: Objid, command: S) -> eyre::Result<()> { + let command: &str = command.as_ref(); + eprintln!("{player} >> ; {}", command); + self.eval_result = Some( + scheduler_test_utils::call_command( + self.scheduler.clone(), + self.session.clone(), + player, + command, + ) + .wrap_err(format!( + "SchedulerMootRunner::command({player}, {:?})", + command + ))?, + ); Ok(()) } @@ -81,23 +94,15 @@ impl MootRunner for SchedulerMootRunner { v_none() } - fn read_line(&mut self, _player: Objid) -> Result, Self::Error> { + fn read_line(&mut self, _player: Objid) -> eyre::Result> { unimplemented!("Not supported on SchedulerMootRunner"); } - fn read_eval_result( - &mut self, - player: Objid, - ) -> Result, SchedulerError> { - self.eval_result + fn read_eval_result(&mut self, player: Objid) -> eyre::Result> { + Ok(self + .eval_result .take() - .inspect(|some| { - let _ = some - .as_ref() - .inspect(|var| eprintln!("{player} << {var}")) - .inspect_err(|err| eprintln!("{player} << ERR: {err}")); - }) - .transpose() + .inspect(|var| eprintln!("{player} << {var}"))) } } @@ -122,7 +127,7 @@ fn test(db: Arc, path: &Path) { let scheduler_loop_jh = std::thread::Builder::new() .name("moor-scheduler".to_string()) .spawn(move || loop_scheduler.run()) - .unwrap(); + .expect("Failed to spawn scheduler"); execute_moot_test( SchedulerMootRunner::new(scheduler.clone(), Arc::new(NoopClientSession::new())), @@ -131,8 +136,10 @@ fn test(db: Arc, path: &Path) { scheduler .submit_shutdown(0, Some("Test is done".to_string())) - .unwrap(); - scheduler_loop_jh.join().unwrap(); + .expect("Failed to shut down scheduler"); + scheduler_loop_jh + .join() + .expect("Failed to join() scheduler"); } #[test] diff --git a/crates/moot/src/lib.rs b/crates/moot/src/lib.rs index 68ef7ec3..ba8b3ce1 100644 --- a/crates/moot/src/lib.rs +++ b/crates/moot/src/lib.rs @@ -24,7 +24,7 @@ use std::{ time::{Duration, Instant}, }; -use eyre::WrapErr; +use eyre::{eyre, ContextCompat, WrapErr}; use moor_values::var::Objid; use pretty_assertions::assert_eq; @@ -51,7 +51,7 @@ fn init_logging() { .with_test_writer() .finish(); tracing::subscriber::set_global_default(main_subscriber) - .expect("Unable to set configure logging"); + .expect("Unable to configure logging"); }); } /// Look up the path to Test.db from any crate under the `moor` workspace @@ -61,13 +61,12 @@ pub fn test_db_path() -> PathBuf { pub trait MootRunner { type Value: PartialEq + std::fmt::Debug; - type Error: std::error::Error + Send + Sync + 'static; - fn eval>(&mut self, player: Objid, command: S) -> Result<(), Self::Error>; - fn command>(&mut self, player: Objid, command: S) -> Result<(), Self::Error>; + fn eval>(&mut self, player: Objid, command: S) -> eyre::Result<()>; + fn command>(&mut self, player: Objid, command: S) -> eyre::Result<()>; - fn read_line(&mut self, player: Objid) -> Result, Self::Error>; - fn read_eval_result(&mut self, player: Objid) -> Result, Self::Error>; + fn read_line(&mut self, player: Objid) -> eyre::Result>; + fn read_eval_result(&mut self, player: Objid) -> eyre::Result>; fn none(&self) -> Self::Value; } @@ -258,17 +257,23 @@ impl MootState { expectation: Option<&str>, line_no: usize, // for the assertion message ) -> eyre::Result<()> { + let err_prefix = || format!("assert_eval_result({player}, {expectation:?}, {line_no})"); let actual = runner .read_eval_result(player)? - .expect("No eval result received"); + .ok_or_else(|| eyre!("{}/actual: got no eval result", err_prefix()))?; let expected = if let Some(expectation) = expectation { runner .eval(player, format!("return {expectation};")) - .wrap_err(format!("Failed to compile expected output: {expectation}"))?; + .wrap_err_with(|| { + format!( + "{}/expected: Failed to compile expected output: {expectation}", + err_prefix() + ) + })?; runner .read_eval_result(player)? - .expect("No eval result received") + .wrap_err_with(|| format!("{}/expected: got no eval result", err_prefix()))? } else { runner.none() }; @@ -326,12 +331,14 @@ pub struct MootClient { stream: TcpStream, } impl MootClient { - pub fn new(port: u16) -> Result { - TcpStream::connect(format!("localhost:{port}")).and_then(|stream| { - stream.set_read_timeout(Some(Duration::from_secs(1)))?; - stream.set_write_timeout(Some(Duration::from_secs(1)))?; - Ok(Self { stream }) - }) + pub fn new(port: u16) -> eyre::Result { + TcpStream::connect(format!("localhost:{port}")) + .and_then(|stream| { + stream.set_read_timeout(Some(Duration::from_secs(1)))?; + stream.set_write_timeout(Some(Duration::from_secs(1)))?; + Ok(Self { stream }) + }) + .wrap_err_with(|| format!("MootClient::new({port})")) } fn port(&self) -> u16 { @@ -341,22 +348,26 @@ impl MootClient { .unwrap_or_default() } - pub fn write_line(&mut self, s: S) -> Result<(), std::io::Error> + pub fn write_line(&mut self, s: S) -> eyre::Result<()> where S: AsRef, { eprintln!("{} >> {}", self.port(), s.as_ref()); + let port = self.port(); let mut writer = BufWriter::new(&mut self.stream); - writer.write_all(s.as_ref().as_bytes()).unwrap(); - writer.write_all(b"\n").unwrap(); - Ok(()) + writer + .write_all(s.as_ref().as_bytes()) + .and_then(|_| writer.write_all(b"\n")) + .wrap_err_with(|| format!("writing port={port}")) } - fn read_line(&self) -> Result, std::io::Error> { + fn read_line(&self) -> eyre::Result> { let mut buf = String::new(); match BufReader::new(&self.stream).read_line(&mut buf) { Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => Ok(None), - Err(e) => Err(e), + Err(e) => { + Err(e).wrap_err_with(|| format!("MootClient::read_line port={}", self.port())) + } Ok(0) => Ok(None), Ok(_) => { let line = buf.trim_end_matches(['\r', '\n']).to_string(); @@ -401,51 +412,54 @@ impl TelnetMootRunner { }) } - fn resolve_response( - &mut self, - player: Objid, - response: String, - ) -> Result { + fn resolve_response(&mut self, player: Objid, response: String) -> eyre::Result { let client = self.client(player); // Resolve the response; for example, the test assertion may be `$object`; resolve it to the object's specific number. client.write_line(format!( "; return {response}; \"TelnetMootRunner::resolve_response\";" ))?; - client.read_line().map(Option::unwrap) + client + .read_line() + .wrap_err_with(|| format!("TelnetMoorRunner::resolve_response({player}, {response:?})")) + .and_then(|maybe_line| maybe_line.ok_or(eyre!("received no response from server"))) } } impl MootRunner for TelnetMootRunner { type Value = String; - type Error = std::io::Error; - fn eval>(&mut self, player: Objid, command: S) -> Result<(), Self::Error> { + fn eval>(&mut self, player: Objid, command: S) -> eyre::Result<()> { + let command: String = command.into(); self.client(player) - .write_line(format!("; {} \"TelnetMootRunner::eval\";", command.into()))?; - Ok(()) + .write_line(format!("; {} \"TelnetMootRunner::eval\";", command)) + .with_context(|| format!("TelnetMootRunner::eval({player}, {:?})", command)) } - fn command>(&mut self, player: Objid, command: S) -> Result<(), Self::Error> { - self.client(player).write_line(command) + fn command>(&mut self, player: Objid, command: S) -> eyre::Result<()> { + let command: &str = command.as_ref(); + self.client(player) + .write_line(command) + .with_context(|| format!("TelnetMootRunner::command({player}, {:?}", command)) } fn none(&self) -> Self::Value { "0".to_string() } - fn read_line(&mut self, player: Objid) -> Result, Self::Error> { - Ok(self.client(player).read_line().expect("Reading raw line")) + fn read_line(&mut self, player: Objid) -> eyre::Result> { + self.client(player) + .read_line() + .with_context(|| format!("TelnetMootRunner::read_line({player})")) } - fn read_eval_result(&mut self, player: Objid) -> Result, Self::Error> { + fn read_eval_result(&mut self, player: Objid) -> eyre::Result> { let raw = self .client(player) .read_line() - .expect("Reading raw eval response"); + .with_context(|| format!("TelnetMootRunner::read_eval_result({player}) / read raw"))?; if let Some(raw) = raw { - Ok(self - .resolve_response(player, raw) + self.resolve_response(player, raw) .map(Some) - .expect("Reading resolved eval response")) + .with_context(|| format!("TelnetMootRunner::read_eval_result({player}) / resolve")) } else { Ok(None) } @@ -468,9 +482,8 @@ pub fn execute_moot_test(runner: R, path: &Path) { let line_no = line_no + 1; state = state .process_line(line_no, &line) - .wrap_err(format!("line {}", line_no)) - .unwrap(); + .unwrap_or_else(|_| panic!("{}:{line_no}", path.display())) //eprintln!("[{line_no}] {line}"); } - state.finalize().unwrap(); + state.finalize().expect("EOF"); }