diff --git a/Cargo.lock b/Cargo.lock index f9afcf0f..f64586e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1171,6 +1171,12 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "downcast" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" + [[package]] name = "dunce" version = "1.0.5" @@ -1421,6 +1427,15 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fragile" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8878864ba14bb86e818a412bfd6f18f9eabd4ec0f008a28e8f7eb61db532fcf9" +dependencies = [ + "futures-core", +] + [[package]] name = "fs_extra" version = "1.3.0" @@ -1989,6 +2004,7 @@ dependencies = [ "anyhow", "hive-priv-sock", "libc", + "mockall", "rustix", "serde_json", "tokio", @@ -3143,6 +3159,32 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "mockall" +version = "0.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1a6ceddfe3ce334925e96bf420fdb2dcee5bed6c632a168ece622676dadeaf8a" +dependencies = [ + "cfg-if", + "downcast", + "fragile", + "mockall_derive", + "predicates", + "predicates-tree", +] + +[[package]] +name = "mockall_derive" +version = "0.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9cfe16fbe8a314aeec0b861ac24e60b1e123e97634bab045475b9d6a18416fd8" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "nats-jwt" version = "0.3.0" @@ -3550,6 +3592,32 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" +[[package]] +name = "predicates" +version = "3.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ada8f2932f28a27ee7b70dd6c1c39ea0675c55a36879ab92f3a715eaa1e63cfe" +dependencies = [ + "anstyle", + "predicates-core", +] + +[[package]] +name = "predicates-core" +version = "1.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cad38746f3166b4031b1a0d39ad9f954dd291e7854fcc0eed52ee41a0b50d144" + +[[package]] +name = "predicates-tree" +version = "1.0.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0de1b847b39c8131db0467e9df1ff60e6d0562ab8e9a16e568ad0fdb372e2f2" +dependencies = [ + "predicates-core", + "termtree", +] + [[package]] name = "problem_details" version = "0.9.0" @@ -4861,6 +4929,7 @@ dependencies = [ "async-nats", "futures-util", "hive-log", + "mockall", "serde_json", "swarm-queue-client", "swarm-secret-client", @@ -5304,6 +5373,12 @@ dependencies = [ "new_debug_unreachable", ] +[[package]] +name = "termtree" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683" + [[package]] name = "thiserror" version = "1.0.69" diff --git a/Cargo.toml b/Cargo.toml index 3157d61b..7c137435 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -287,3 +287,8 @@ utoipa-axum = "0.2" # re-issue it at half its life. `der` and `spki` are already in the tree # through `pkcs8`. x509-cert = { version = "0.2", default-features = false, features = ["pem"] } +# Test doubles with expectations (canned replies, call counts, argument +# matchers). Dev-dependency only, and used behind `#[cfg(test)]`, so release +# builds never compile it. See "Testing seams and test doubles" in +# `docs/process/conventions.md`. +mockall = "0.15" diff --git a/docs/process/conventions.md b/docs/process/conventions.md index 2a4f7afb..4b1898dd 100644 --- a/docs/process/conventions.md +++ b/docs/process/conventions.md @@ -521,6 +521,27 @@ gate locally: `nix flake check` (shares the build farm, use sparingly) or build just the suspect check, for example `nix build .#checks.x86_64-linux.hivectl-docs`. +## Testing seams and test doubles + +- **A new test seam is a trait, used generically** (`impl Seam` or a type + parameter). No `dyn Fn` closure bags: a trait names each question the code + asks and gives the test one place to answer it. +- **A double that only answers and records uses `mockall`** (a + dev-dependency): put `#[cfg_attr(test, mockall::automock)]` on the trait, + set canned replies with `returning`, and assert calls with `times` and + `withf`. The attribute keeps `mockall` out of release builds. Examples: + `Probes` in `hive-priv/src/storage.rs`, `Publisher` and `Tokens` in + `swarm-builder/src/main.rs`. +- **`automock` can't mock a trait with a supertrait** such as `Clone`: a mock + doesn't implement it. Declare the mock with `mockall::mock!` as a plain + struct and implement the trait for `Arc`, as `swarm-builder` does. + Better, leave the supertrait off the trait and bound the callers. +- **Stateful fakes stay hand-written**: in-memory stores whose reads see earlier + writes. Real-process stubs such as `ScriptSpawn` also stay, since they run a + real `sh` on purpose. +- **Nothing migrates in one sweep.** Move an existing expectation-style fake + to `mockall` when you touch its file for another reason. + ## Best-effort oneshot services The harness ships a family of one-shot systemd services that diff --git a/hive-priv/Cargo.toml b/hive-priv/Cargo.toml index 09148524..abcfe7bd 100644 --- a/hive-priv/Cargo.toml +++ b/hive-priv/Cargo.toml @@ -16,3 +16,6 @@ serde_json.workspace = true tokio.workspace = true tracing.workspace = true tracing-subscriber.workspace = true + +[dev-dependencies] +mockall.workspace = true diff --git a/hive-priv/src/storage.rs b/hive-priv/src/storage.rs index 6a7fbd64..c8c84355 100644 --- a/hive-priv/src/storage.rs +++ b/hive-priv/src/storage.rs @@ -179,6 +179,7 @@ fn is_subvolume_root(on_btrfs: bool, ino: Option) -> bool { /// The two questions the storage ops ask the state filesystem. [`RealProbes`] /// is production; tests answer without a btrfs filesystem. +#[cfg_attr(test, mockall::automock)] trait Probes { /// Whether `path` lives on a btrfs filesystem; an error when it cannot be /// told. @@ -1009,7 +1010,7 @@ async fn set_subvolume_quota( #[cfg(test)] mod tests { use super::{ - Command, Env, Probes, agent_disk_paths, cleanup_stale_path, delete_agent_snapshot, + Command, Env, MockProbes, agent_disk_paths, cleanup_stale_path, delete_agent_snapshot, delete_agent_subvolume, du_command, du_total, ensure_agent_subvolume, ensure_btrfs_quota_in, exec_delete_agent_snapshot, exec_delete_agent_subvolume, exec_ensure_agent_subvolume, exec_measure_agent_disk, exec_read_subvolume_usage, @@ -1412,21 +1413,31 @@ mod tests { .collect() } - /// An [`Env`] over this tree, running `btrfs` and the two probes given. + /// An [`Env`] over this tree, running `btrfs` and probes that answer + /// with the functions given. fn env<'a, S: Spawn>( &'a self, spawn: &'a S, on_btrfs: fn(&Path) -> anyhow::Result, is_subvolume: fn(&Path) -> bool, - ) -> Env<'a, &'a S, FnProbes> { + ) -> Env<'a, &'a S, MockProbes> { + let mut probes = MockProbes::new(); + probes.expect_on_btrfs().returning(on_btrfs); + probes.expect_is_subvolume().returning(is_subvolume); + self.env_with(spawn, probes) + } + + /// An [`Env`] over this tree, running `btrfs` and the `probes` given. + fn env_with<'a, S: Spawn>( + &'a self, + spawn: &'a S, + probes: MockProbes, + ) -> Env<'a, &'a S, MockProbes> { Env { state_root: &self.state, staging_root: &self.staging, spawn, - probes: FnProbes { - on_btrfs, - is_subvolume, - }, + probes, } } @@ -1440,22 +1451,6 @@ mod tests { } } - /// Probes answered by the functions given. - struct FnProbes { - on_btrfs: fn(&Path) -> anyhow::Result, - is_subvolume: fn(&Path) -> bool, - } - - impl Probes for FnProbes { - fn on_btrfs(&self, path: &Path) -> anyhow::Result { - (self.on_btrfs)(path) - } - - fn is_subvolume(&self, path: &Path) -> bool { - (self.is_subvolume)(path) - } - } - /// Creates, deletes and snapshots as btrfs does; `send` writes `STREAM`. const LIKE_BTRFS: &str = r#"case "$1 $2" in "subvolume create") mkdir "$3";; @@ -1490,7 +1485,8 @@ esac"#; let fx = Fx::new(); std::fs::create_dir(fx.agent("iris")).unwrap(); let btrfs = fx.btrfs(LIKE_BTRFS); - let env = fx.env(&btrfs, on_btrfs, not_subvol); + // No expectation is set: a probe call would panic. + let env = fx.env_with(&btrfs, MockProbes::new()); assert_eq!( ensure_agent_subvolume(&env, "iris").await.unwrap(), (String::new(), String::new()) diff --git a/swarm-builder/Cargo.toml b/swarm-builder/Cargo.toml index 7e3d90fe..69d2a943 100644 --- a/swarm-builder/Cargo.toml +++ b/swarm-builder/Cargo.toml @@ -26,6 +26,7 @@ tracing.workspace = true # `test-util` for `#[tokio::test(start_paused = true)]`: the slot tests wait # out the build slot's wait limit on paused time. [dev-dependencies] +mockall.workspace = true tokio = { workspace = true, features = ["test-util"] } tracing-subscriber.workspace = true diff --git a/swarm-builder/src/main.rs b/swarm-builder/src/main.rs index 16105071..678ddb47 100644 --- a/swarm-builder/src/main.rs +++ b/swarm-builder/src/main.rs @@ -177,7 +177,7 @@ trait Publisher: Clone + Send + Sync + 'static { } /// The secrets a request reads from the store, once per request. [`Store`] in -/// production; tests answer from memory. +/// production; tests answer with canned replies. trait Tokens: Clone + Send + Sync + 'static { /// The forge token config repos are fetched with. fn forge_token(&self) -> impl Future> + Send; @@ -970,57 +970,79 @@ mod tests { Read::Stuck } - #[derive(Clone)] - struct FakeTokens { - forge: Read, - push: Read, - } - - impl Tokens for FakeTokens { - async fn forge_token(&self) -> Result { - self.forge.get().await - } - - async fn push_token(&self) -> Result { - self.push.get().await + // `Publisher` and `Tokens` have `Clone` as a supertrait, which `automock` + // cannot express (a mock is not `Clone`), so the two mocks are declared by + // hand with `mock!` as plain structs. The traits are implemented for + // `Arc`: every clone the daemon makes shares one set of + // expectations, which are checked when the last clone drops. + mockall::mock! { + Publisher { + fn publish( + &self, + subject: Subject, + body: Vec, + ) -> impl Future> + Send; } } - /// A publisher whose first `failing` publishes fail; the rest are recorded. - #[derive(Clone)] - struct Sink { - sent: tokio::sync::mpsc::UnboundedSender<(String, Value)>, - attempts: Arc, - failing: usize, + impl Publisher for Arc { + fn publish( + &self, + subject: Subject, + body: Vec, + ) -> impl Future> + Send { + MockPublisher::publish(self, subject, body) + } } - impl Publisher for Sink { - async fn publish(&self, subject: Subject, body: Vec) -> Result<(), String> { - if self.attempts.fetch_add(1, Ordering::SeqCst) < self.failing { - return Err("the connection is closed".to_owned()); - } - let body = serde_json::from_slice(&body).expect("a JSON reply"); - self.sent - .send((subject.to_string(), body)) - .expect("test is listening"); - Ok(()) + mockall::mock! { + Tokens { + fn forge_token(&self) -> impl Future> + Send; + fn push_token(&self) -> impl Future> + Send; + } + } + + impl Tokens for Arc { + fn forge_token(&self) -> impl Future> + Send { + MockTokens::forge_token(self) + } + + fn push_token(&self) -> impl Future> + Send { + MockTokens::push_token(self) } } struct Rig { - sink: Sink, - tokens: FakeTokens, + sink: Arc, + tokens: Arc, } + /// A rig whose token reads answer `forge` and `push`, and whose first + /// `failing` publishes fail; the rest are sent to the returned receiver. fn rig_with(forge: Read, push: Read, failing: usize) -> (Rig, Sent) { let (sent, rx) = tokio::sync::mpsc::unbounded_channel(); - let sink = Sink { - sent, - attempts: Arc::default(), - failing, + let mut sink = MockPublisher::new(); + sink.expect_publish() + .times(failing) + .returning(|_, _| Box::pin(async { Err("the connection is closed".to_owned()) })); + sink.expect_publish().returning(move |subject, body| { + let body = serde_json::from_slice(&body).expect("a JSON reply"); + sent.send((subject.to_string(), body)) + .expect("test is listening"); + Box::pin(async { Ok(()) }) + }); + let mut tokens = MockTokens::new(); + tokens + .expect_forge_token() + .returning(move || Box::pin(forge.get())); + tokens + .expect_push_token() + .returning(move || Box::pin(push.get())); + let rig = Rig { + sink: Arc::new(sink), + tokens: Arc::new(tokens), }; - let tokens = FakeTokens { forge, push }; - (Rig { sink, tokens }, rx) + (rig, rx) } fn env() -> (Rig, Sent) {