test(plugins): move the plugins' test harnesses onto FakeHost

apps/locallibrary/CONTRIBUTING.md

@@ -39,11 +39,11 @@ The plugins locallibrary ships are a crate each in `plugins/<id>/` (`locallibrar
 written on loclib-plugin's SDK (see [Plugins](docs/plugins.md)). Like the crates above, they never
 depend on `locallibrary`: they read the library through the host's callbacks, and the CLI links
 every one into its binary (`apps/locallibrary/src/plugins.rs`, from the table `build.rs` writes).
-Their tests sit beside their code, driving the plugin over in-memory pipes with a fake host
-(loclib-plugin's `FakeHost` for new ones, see its README's "Testing a plugin"); the
-CLI's tests run them through the real one. Account plugins are the exception: each is a
-`locallibrary-plugin.toml` alone in `plugins/accounts/<id>/`, outside the workspace, which
-`build.rs` includes and nothing runs, so adding one is adding its folder.
+Their tests sit beside their code, driving the plugin over in-memory pipes with loclib-plugin's
+`FakeHost` (see its README's "Testing a plugin"); the CLI's tests run them through the real one.
+Account plugins are the exception: each is a `locallibrary-plugin.toml` alone in
+`plugins/accounts/<id>/`, outside the workspace, which `build.rs` includes and nothing runs, so
+adding one is adding its folder.
 
 `rust-toolchain.toml` pins the Rust that CI and release builds use (1.96.1, with clippy, rustfmt
 and llvm-tools), and rustup installs it the first time cargo runs in the checkout. A Rust that

crates/loclib-plugin/Cargo.toml

@@ -30,8 +30,7 @@ toml = { version = "1.1.6", default-features = false, features = ["std", "serde"
 url = "2.5"
 
 [features]
-# `test_util`: a fake host to test a plugin against over the protocol, and its cancel of the
-# request it's serving.
+# `test_util`: a fake host to test a plugin against over the protocol.
 test-util = ["dep:tempfile"]
 
 [dev-dependencies]

crates/loclib-plugin/README.md

@@ -763,17 +763,23 @@ plugin's tests needn't write one. Add it as a dev-dependency
 in-memory pipes, sends `hello`, and checks that `describe` answers the manifest. `read(method,
 answer)` answers a read every time the plugin calls it. `on_read(method, f)` works the answer out
 from the params instead, and answers any other call too (`accounts/ensure`). `fetch(url, answer)`
-answers `net/fetch` for one URL. A call with no answer fails: `external` for a fetch,
-`method_not_found` for anything else.
+answers `net/fetch` for one URL. `on_call(method, f)` is `on_read` whose `f` returns a
+`Response`: an answer, an answer followed by a cancel of the hook (the user cancelling just as the
+call came back), or `Hold`, which never answers (a server that keeps the plugin waiting until its
+hook is cancelled). A call with no answer fails: `external` for a fetch, `method_not_found` for
+anything else.
 
 `request(method, params)` sends a hook and waits for its typed answer. `send` returns a `Sent`
-instead, which can `cancel()` it (`$/cancel`), `wait()` for it, and has its `progress()`. With the
-`blobs` feature each request gets a fresh `context.scratch` folder (`Sent::scratch`), removed when
-the `Sent` is dropped, as the host removes it once it has read the answer. `cancel_on(method)`
+instead, which can `cancel()` it (`$/cancel`), `wait()` or `wait_timeout(limit)` for it, and has
+its `progress()`. With the `blobs` feature each request gets a fresh `context.scratch` folder
+(`Sent::scratch`), removed when the `Sent` is dropped, as the host removes it once it has read the
+answer. Params that already give `context.scratch` keep it: a folder of the test's, or `null` for
+a host without `blobs`. `cancel_on(method)`
 cancels the hook under way the next time the plugin calls `method`. `calls()` lists every call the
 plugin made, with its params. `event(..)` sends `event/notify`, and `notices()` has what the
 plugin told the user (`host/notify`). `stop()` closes the plugin's input and waits for it to stop:
-after that, every notice is in.
+after that, every notice is in. A call an event's handler is still waiting on fails then, so a
+test whose events make calls first sends one last event and waits for its call, held.
 
 ```rust
 # #[cfg(feature = "test-util")] {

crates/loclib-plugin/src/test_util.rs

@@ -1,54 +1,5 @@
-//! Test support: [`FakeHost`], a host to test a plugin against over the protocol, and
-//! [`Canceller`], for a hand-written one, cancelling a request the plugin is serving right after
-//! the host has answered one of the plugin's calls, the way a user's cancel can land between two
-//! calls.
+//! Test support: [`FakeHost`], a host to test a plugin against over the protocol.
 
 mod fake_host;
 
-use std::sync::Mutex;
-use std::sync::atomic::{AtomicBool, Ordering};
-
-use serde_json::Value;
-
-pub use self::fake_host::{FakeHost, Sent};
-use crate::{Cancel, Peer, Pending, RpcError, method};
-
-/// The request a fake host cancels, and whether the answer it's sending now cancels it.
-///
-/// A harness sends the request to cancel through [`Canceller::send`], calls
-/// [`Canceller::arm`] while it works out the answer that cancels, and answers every call
-/// through [`Canceller::respond`].
-#[derive(Debug, Default)]
-pub struct Canceller {
-    /// The id of the request last sent through [`Canceller::send`]. Held while it's sent, so a
-    /// call the plugin makes for it can't read it before it's there.
-    sent: Mutex<Option<u64>>,
-    /// Whether [`Canceller::respond`] cancels that request once it has answered.
-    armed: AtomicBool,
-}
-
-impl Canceller {
-    /// Sends a request with `send` as the one a later cancel goes to.
-    pub fn send(&self, send: impl FnOnce() -> Pending) -> Pending {
-        let mut sent = self.sent.lock().unwrap();
-        let pending = send();
-        *sent = Some(pending.id());
-        pending
-    }
-
-    /// Makes the next [`Canceller::respond`] cancel the request it was sent for.
-    pub fn arm(&self) {
-        self.armed.store(true, Ordering::SeqCst);
-    }
-
-    /// Answers the plugin's call `id` with `answer` through `host`, and then, when armed, cancels
-    /// the request [`Canceller::send`] sent.
-    pub fn respond(&self, host: &Peer, id: &Value, answer: Result<Value, RpcError>) {
-        host.respond(id, answer);
-        if !self.armed.swap(false, Ordering::SeqCst) {
-            return;
-        }
-        let sent = self.sent.lock().unwrap().expect("a request sent through Canceller::send to cancel");
-        host.notify(method::CANCEL, &Cancel { id: sent.into() });
-    }
-}
+pub use self::fake_host::{FakeHost, Response, Sent};

crates/loclib-plugin/src/test_util/fake_host.rs

@@ -23,7 +23,20 @@ use crate::{
 };
 
 /// Answers a call the plugin makes, given its params.
-type Answer = Box<dyn Fn(Value) -> Result<Value, RpcError> + Send>;
+type Answer = Box<dyn Fn(Value) -> Response + Send>;
+
+/// What the host does with a call of the plugin's, given to [`FakeHost::on_call`].
+#[derive(Debug)]
+pub enum Response {
+    /// Answers it.
+    Answer(Result<Value, RpcError>),
+    /// Answers it, then cancels the hook request last sent: the user cancelling just as the call
+    /// came back, before the plugin's next.
+    AnswerThenCancel(Result<Value, RpcError>),
+    /// Never answers it: a server that keeps the fetch under way until the plugin stops waiting,
+    /// its hook cancelled.
+    Hold,
+}
 
 /// A plugin served on a thread, past `hello` and `describe`, with a host that answers its calls
 /// from what it's given and keeps what the plugin sent it. Stopped when dropped, or by
@@ -99,7 +112,13 @@ impl FakeHost {
     /// read that pages, an error, or `net/fetch` for any URL [`FakeHost::fetch`] has no answer
     /// for.
     pub fn on_read(&self, method: &str, answer: impl Fn(Value) -> Result<Value, RpcError> + Send + 'static) -> &Self {
-        lock(&self.shared.state).answers.insert(method.into(), Box::new(answer));
+        self.on_call(method, move |params| Response::Answer(answer(params)))
+    }
+
+    /// [`FakeHost::on_read`], where `respond` may also cancel the hook request once it has
+    /// answered, or hold the call unanswered.
+    pub fn on_call(&self, method: &str, respond: impl Fn(Value) -> Response + Send + 'static) -> &Self {
+        lock(&self.shared.state).answers.insert(method.into(), Box::new(respond));
         self
     }
 
@@ -117,11 +136,13 @@ impl FakeHost {
     }
 
     /// Sends a hook request (`command/run`), as the host does: `context.scratch` a new, empty
-    /// folder when the plugin has the `blobs` feature, removed once the [`Sent`] is dropped.
+    /// folder when the plugin has the `blobs` feature, removed once the [`Sent`] is dropped. One
+    /// `params` already give is kept: a folder that's gone, or `null` for a host without `blobs`.
     pub fn send(&self, method: &str, params: &impl Serialize) -> Sent {
         let mut params = serde_json::to_value(params).expect("the params are JSON");
+        let given = params["context"].get("scratch").is_some();
         let blobs = self.hello.features.iter().any(|f| f == "blobs");
-        let scratch = blobs.then(|| tempfile::tempdir().expect("a scratch folder"));
+        let scratch = (blobs && !given).then(|| tempfile::tempdir().expect("a scratch folder"));
         if let Some(dir) = &scratch {
             params["context"]["scratch"] = json!(dir.path());
         }
@@ -138,7 +159,9 @@ impl FakeHost {
 
     /// Tells the plugin of an event (`event/notify`). It handles them in order on a thread of its
     /// own: what it tells the user is in [`FakeHost::notices`] once it has, at the latest once
-    /// [`FakeHost::stop`] returns.
+    /// [`FakeHost::stop`] returns. Stopping fails a call a handler is still waiting on, so a test
+    /// whose events make calls waits for them first: one last event whose call it holds
+    /// ([`Response::Hold`]) comes once every event before it has been handled.
     pub fn event(&self, event: &EventNotify) {
         self.peer.notify(method::EVENT_NOTIFY, event);
     }
@@ -184,41 +207,55 @@ fn answer(peer: &Peer, shared: &Shared, incoming: Receiver<Incoming>) {
             }
             Incoming::Notification { .. } => continue,
         };
-        let (answer, cancels) = {
+        let (response, cancels) = {
             let mut state = lock(&shared.state);
             state.calls.push((req.method.clone(), req.params.clone()));
             let cancels = state.cancel_on.take_if(|m| *m == req.method).is_some();
             (state.answer(&req.method, req.params), cancels)
         };
-        if cancels {
+        let cancel = || {
             let sent = lock(&shared.sent).expect("a hook request sent to cancel");
             peer.notify(method::CANCEL, &Cancel { id: sent.into() });
+        };
+        if cancels {
+            cancel();
+        }
+        match response {
+            Response::Answer(answer) => peer.respond(&req.id, answer),
+            Response::AnswerThenCancel(answer) => {
+                peer.respond(&req.id, answer);
+                cancel();
+            }
+            Response::Hold => {}
         }
-        peer.respond(&req.id, answer);
     }
 }
 
 impl State {
     /// A call's answer: a fetch's for its URL, else the method's. One with none fails, as the host
     /// fails a call it can't make: `external` for a fetch, `method_not_found` else.
-    fn answer(&self, method: &str, args: Value) -> Result<Value, RpcError> {
+    fn answer(&self, method: &str, args: Value) -> Response {
         let url = match method {
-            method::NET_FETCH => Some(params::<Fetch>(args.clone())?.url),
+            method::NET_FETCH => match params::<Fetch>(args.clone()) {
+                Ok(fetch) => Some(fetch.url),
+                Err(e) => return Response::Answer(Err(e)),
+            },
             _ => None,
         };
         if let Some(fetched) = url.as_ref().and_then(|url| self.fetches.get(url)) {
-            return fetched.clone().and_then(ok);
+            return Response::Answer(fetched.clone().and_then(ok));
         }
         let Some(answer) = self.answers.get(method) else {
-            return Err(match url {
+            return Response::Answer(Err(match url {
                 Some(url) => RpcError::failed("external", format!("the fake host has no answer for {url}")),
                 None => RpcError::method_not_found(method),
-            });
+            }));
         };
         // An answer that panics (a test's assertion) fails the call rather than leaving the
         // plugin waiting for it.
-        catch_unwind(AssertUnwindSafe(|| answer(args)))
-            .unwrap_or_else(|_| Err(RpcError::internal(format!("the fake host's answer to {method} panicked"))))
+        catch_unwind(AssertUnwindSafe(|| answer(args))).unwrap_or_else(|_| {
+            Response::Answer(Err(RpcError::internal(format!("the fake host's answer to {method} panicked"))))
+        })
     }
 }
 
@@ -246,6 +283,11 @@ impl Sent {
         self.pending.wait_timeout(Duration::MAX).expect("waiting without a limit ends with the answer")
     }
 
+    /// [`Sent::wait`], for at most `limit`: none when the answer hasn't come by then.
+    pub fn wait_timeout<R: DeserializeOwned>(&self, limit: Duration) -> Option<Result<R, RpcError>> {
+        self.pending.wait_timeout(limit)
+    }
+
     /// The progress the plugin has sent about it since last asked, in order: all of it once the
     /// answer has come.
     pub fn progress(&self) -> Vec<ProgressReport> {

crates/loclib-plugin/src/test_util/fake_host/tests.rs

@@ -12,7 +12,8 @@ use crate::peer::tests::json_value;
 use crate::{EventName, Hooks, Host, Progressed, RunCommand};
 
 /// A plugin that makes the call its params name, leaves a file in its scratch folder, or starts
-/// a phase, reads `drive.info` and waits to be cancelled; and tells the user of each event.
+/// a phase, reads `drive.info`, tells the user what it read and waits to be cancelled; and tells
+/// the user of each event.
 const MANIFEST: &str = r#"
 id = "probe"
 name = "Probe"
@@ -47,7 +48,8 @@ fn run(req: RunCommand, host: &Host) -> Result<Value, RpcError> {
         _ => {
             let ctx = host.ctx();
             ctx.phase_start(Phase::Scan, 0, 0);
-            host.call::<_, Value>("drive.info", &json!({}))?;
+            let info: Value = host.call("drive.info", &json!({}))?;
+            host.notify(&HostNotify { message: info.to_string(), ..HostNotify::default() });
             loop {
                 ctx.check()?;
                 std::thread::sleep(Duration::from_millis(5));
@@ -190,6 +192,50 @@ fn cancel_on_cancels_the_request_under_way_once() {
     host.stop();
 }
 
+/// A response that answers and then cancels lands the cancel after the answer: the plugin has
+/// what it read, then stops with `cancelled`.
+#[test]
+fn an_answer_then_cancel_reaches_the_plugin_first() {
+    let host = FakeHost::start(MANIFEST, serve);
+    host.on_call("drive.info", |_| Response::AnswerThenCancel(Ok(json!({ "name": "USB" }))));
+    let e = host.request::<Value>(method::RUN_COMMAND, &json!({ "command": "wait" })).unwrap_err();
+    assert_eq!(e.kind(), Some("cancelled"));
+    host.stop();
+    let messages: Vec<String> = host.notices().into_iter().map(|n| n.message).collect();
+    assert_eq!(messages, [r#"{"name":"USB"}"#]);
+}
+
+/// A call held is never answered: the plugin waits on it until its hook is cancelled.
+#[test]
+fn a_held_call_waits_until_the_hook_is_cancelled() {
+    let host = FakeHost::start(MANIFEST, serve);
+    host.on_call("drive.info", |_| Response::Hold);
+    let waiting = host.send(method::RUN_COMMAND, &json!({ "command": "wait" }));
+    assert!(waiting.wait_timeout::<Value>(Duration::from_millis(200)).is_none(), "under way");
+    waiting.cancel();
+    let answer = waiting.wait_timeout::<Value>(Duration::from_secs(10)).expect("it stops");
+    assert_eq!(answer.unwrap_err().kind(), Some("cancelled"));
+    host.stop();
+    assert!(host.notices().is_empty(), "nothing read");
+    assert_eq!(host.calls(), [("drive.info".to_string(), json!({}))]);
+}
+
+/// A `context.scratch` the params give is kept, and no folder made for it: a folder, or `null`
+/// for none.
+#[test]
+fn a_scratch_folder_given_is_kept() {
+    let host = FakeHost::start(MANIFEST, serve);
+    let dir = tempfile::tempdir().unwrap();
+    let given = host.send(method::RUN_COMMAND, &json!({ "command": "blob", "context": { "scratch": dir.path() } }));
+    let answer: Value = given.wait().unwrap();
+    assert_eq!((&answer["scratch"], given.scratch()), (&json!(dir.path()), None));
+    assert!(dir.path().join("out.txt").exists());
+    let none = json!({ "command": "blob", "context": { "scratch": null } });
+    let e = host.request::<Value>(method::RUN_COMMAND, &none).unwrap_err();
+    assert_eq!(e.message, "invalid params: no scratch folder");
+    host.stop();
+}
+
 /// What the plugin tells the user of each event is kept, in order, by the time it has stopped.
 #[test]
 fn notices_are_kept_in_order() {
@@ -217,3 +263,29 @@ fn prop_canned_reads_reach_the_plugin_unchanged(tc: TestCase) {
     assert_eq!(host.calls(), [(read, Value::Object(given))]);
     host.stop();
 }
+
+/// Any answer given and then cancelled reaches the plugin before the cancel; held, none does, and
+/// the plugin waits until it's cancelled. Either way the hook stops with `cancelled`.
+#[hegel::test(test_cases = 25)]
+fn prop_an_answer_reaches_the_plugin_unless_held(tc: TestCase) {
+    let answer = json_value(&tc, 3);
+    let held = tc.draw(gs::booleans());
+    let host = FakeHost::start(MANIFEST, serve);
+    let (given, (called, made)) = (answer.clone(), std::sync::mpsc::channel());
+    host.on_call("drive.info", move |_| {
+        let _ = called.send(());
+        match held {
+            true => Response::Hold,
+            false => Response::AnswerThenCancel(Ok(given.clone())),
+        }
+    });
+    let waiting = host.send(method::RUN_COMMAND, &json!({ "command": "wait" }));
+    if held {
+        made.recv().unwrap();
+        waiting.cancel();
+    }
+    assert_eq!(waiting.wait::<Value>().unwrap_err().kind(), Some("cancelled"));
+    host.stop();
+    let read: Vec<Value> = host.notices().iter().map(|n| serde_json::from_str(&n.message).unwrap()).collect();
+    assert_eq!(read, if held { Vec::new() } else { vec![answer] });
+}