From 6d4fe85574b71fef17e99af56f07b866c77cf6d7 Mon Sep 17 00:00:00 2001 From: Patrick Mooney Date: Fri, 3 May 2024 15:24:06 -0500 Subject: [PATCH] Standardize on `mod test` Keep a consistent style of `mod test`, rather than a mix of that and `mod tests`. Enforce this new rule with a rudimentary check, added as `xtask style`. --- .github/workflows/rust.yml | 2 + bin/propolis-cli/src/main.rs | 118 ++++++++++-------- .../src/lib/migrate/protocol.rs | 2 +- .../src/lib/serial/history_buffer.rs | 2 +- bin/propolis-server/src/lib/server.rs | 2 +- .../src/lib/stats/virtual_machine.rs | 2 +- .../src/lib/vm/request_queue.rs | 2 +- .../src/lib/vm/state_driver.rs | 2 +- crates/propolis-server-config/src/lib.rs | 2 +- lib/propolis-client/src/support.rs | 2 +- lib/propolis/src/chardev/pollers.rs | 2 +- lib/propolis/src/common.rs | 2 +- lib/propolis/src/firmware/smbios/table.rs | 6 +- lib/propolis/src/hw/nvme/queue.rs | 2 +- lib/propolis/src/hw/pci/bar.rs | 2 +- lib/propolis/src/hw/uart/uart16550.rs | 2 +- lib/propolis/src/util/aspace.rs | 2 +- lib/propolis/src/vmm/mem.rs | 2 +- xtask/src/main.rs | 9 ++ xtask/src/task_prepush.rs | 5 +- xtask/src/task_style.rs | 68 ++++++++++ 21 files changed, 168 insertions(+), 70 deletions(-) create mode 100644 xtask/src/task_style.rs diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 16942bff1..fb0e48292 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -21,6 +21,8 @@ jobs: run: cargo fmt -- --version - name: Check style run: cargo fmt -- --check + - name: Check misc. style + run: cargo xtask style check-clippy: runs-on: ubuntu-latest steps: diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 4feecd851..b1f6a6ad8 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -360,57 +360,6 @@ async fn stdin_to_websockets_task( } } -#[tokio::test] -async fn test_stdin_to_websockets_task() { - use tokio::sync::mpsc::error::TryRecvError; - - let (stdintx, stdinrx) = tokio::sync::mpsc::channel(16); - let (wstx, mut wsrx) = tokio::sync::mpsc::channel(16); - - tokio::spawn(async move { stdin_to_websockets_task(stdinrx, wstx).await }); - - // send characters, receive characters - stdintx - .send("test post please ignore".chars().map(|c| c as u8).collect()) - .await - .unwrap(); - let actual = wsrx.recv().await.unwrap(); - assert_eq!(String::from_utf8(actual).unwrap(), "test post please ignore"); - - // don't send ctrl-a - stdintx.send("\x01".chars().map(|c| c as u8).collect()).await.unwrap(); - assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); - - // the "t" here is sent "raw" because of last ctrl-a but that doesn't change anything - stdintx.send("test".chars().map(|c| c as u8).collect()).await.unwrap(); - let actual = wsrx.recv().await.unwrap(); - assert_eq!(String::from_utf8(actual).unwrap(), "test"); - - // ctrl-a ctrl-c = only ctrl-c sent - stdintx.send("\x01\x03".chars().map(|c| c as u8).collect()).await.unwrap(); - let actual = wsrx.recv().await.unwrap(); - assert_eq!(String::from_utf8(actual).unwrap(), "\x03"); - - // same as above, across two messages - stdintx.send("\x01".chars().map(|c| c as u8).collect()).await.unwrap(); - stdintx.send("\x03".chars().map(|c| c as u8).collect()).await.unwrap(); - assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); - let actual = wsrx.recv().await.unwrap(); - assert_eq!(String::from_utf8(actual).unwrap(), "\x03"); - - // ctrl-a ctrl-a = only ctrl-a sent - stdintx.send("\x01\x01".chars().map(|c| c as u8).collect()).await.unwrap(); - let actual = wsrx.recv().await.unwrap(); - assert_eq!(String::from_utf8(actual).unwrap(), "\x01"); - - // ctrl-c on its own means exit - stdintx.send("\x03".chars().map(|c| c as u8).collect()).await.unwrap(); - assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); - - // channel is closed - assert!(wsrx.recv().await.is_none()); -} - async fn serial( addr: SocketAddr, byte_offset: Option, @@ -747,3 +696,70 @@ impl Drop for RawTermiosGuard { } } } + +#[cfg(test)] +mod test { + use super::stdin_to_websockets_task; + + #[tokio::test] + async fn test_stdin_to_websockets_task() { + use tokio::sync::mpsc::error::TryRecvError; + + let (stdintx, stdinrx) = tokio::sync::mpsc::channel(16); + let (wstx, mut wsrx) = tokio::sync::mpsc::channel(16); + + tokio::spawn( + async move { stdin_to_websockets_task(stdinrx, wstx).await }, + ); + + // send characters, receive characters + stdintx + .send("test post please ignore".chars().map(|c| c as u8).collect()) + .await + .unwrap(); + let actual = wsrx.recv().await.unwrap(); + assert_eq!( + String::from_utf8(actual).unwrap(), + "test post please ignore" + ); + + // don't send ctrl-a + stdintx.send("\x01".chars().map(|c| c as u8).collect()).await.unwrap(); + assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); + + // the "t" here is sent "raw" because of last ctrl-a but that doesn't change anything + stdintx.send("test".chars().map(|c| c as u8).collect()).await.unwrap(); + let actual = wsrx.recv().await.unwrap(); + assert_eq!(String::from_utf8(actual).unwrap(), "test"); + + // ctrl-a ctrl-c = only ctrl-c sent + stdintx + .send("\x01\x03".chars().map(|c| c as u8).collect()) + .await + .unwrap(); + let actual = wsrx.recv().await.unwrap(); + assert_eq!(String::from_utf8(actual).unwrap(), "\x03"); + + // same as above, across two messages + stdintx.send("\x01".chars().map(|c| c as u8).collect()).await.unwrap(); + stdintx.send("\x03".chars().map(|c| c as u8).collect()).await.unwrap(); + assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); + let actual = wsrx.recv().await.unwrap(); + assert_eq!(String::from_utf8(actual).unwrap(), "\x03"); + + // ctrl-a ctrl-a = only ctrl-a sent + stdintx + .send("\x01\x01".chars().map(|c| c as u8).collect()) + .await + .unwrap(); + let actual = wsrx.recv().await.unwrap(); + assert_eq!(String::from_utf8(actual).unwrap(), "\x01"); + + // ctrl-c on its own means exit + stdintx.send("\x03".chars().map(|c| c as u8).collect()).await.unwrap(); + assert_eq!(wsrx.try_recv(), Err(TryRecvError::Empty)); + + // channel is closed + assert!(wsrx.recv().await.is_none()); + } +} diff --git a/bin/propolis-server/src/lib/migrate/protocol.rs b/bin/propolis-server/src/lib/migrate/protocol.rs index 70993e1c8..6a278091f 100644 --- a/bin/propolis-server/src/lib/migrate/protocol.rs +++ b/bin/propolis-server/src/lib/migrate/protocol.rs @@ -256,7 +256,7 @@ pub(super) fn select_protocol_from_offer( } #[cfg(test)] -mod tests { +mod test { use super::*; // N.B. The test protocol lists are sorted by version to meet the diff --git a/bin/propolis-server/src/lib/serial/history_buffer.rs b/bin/propolis-server/src/lib/serial/history_buffer.rs index 74ed00aca..1540a72d3 100644 --- a/bin/propolis-server/src/lib/serial/history_buffer.rs +++ b/bin/propolis-server/src/lib/serial/history_buffer.rs @@ -218,7 +218,7 @@ impl HistoryBuffer { } #[cfg(test)] -mod tests { +mod test { use super::*; use SerialHistoryOffset::*; diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index f42c199ad..d3fe4d899 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -1147,7 +1147,7 @@ fn not_created_error() -> HttpError { } #[cfg(test)] -mod tests { +mod test { #[test] fn test_propolis_server_openapi() { let mut buf: Vec = vec![]; diff --git a/bin/propolis-server/src/lib/stats/virtual_machine.rs b/bin/propolis-server/src/lib/stats/virtual_machine.rs index 1e0307c1e..731a55398 100644 --- a/bin/propolis-server/src/lib/stats/virtual_machine.rs +++ b/bin/propolis-server/src/lib/stats/virtual_machine.rs @@ -395,7 +395,7 @@ fn produce_vcpu_usage<'a>( } #[cfg(test)] -mod tests { +mod test { use super::kstat_instance_from_instance_id; use super::kstat_microstate_to_state_name; use super::produce_vcpu_usage; diff --git a/bin/propolis-server/src/lib/vm/request_queue.rs b/bin/propolis-server/src/lib/vm/request_queue.rs index f19b99aac..9d23faa26 100644 --- a/bin/propolis-server/src/lib/vm/request_queue.rs +++ b/bin/propolis-server/src/lib/vm/request_queue.rs @@ -444,7 +444,7 @@ impl ExternalRequestQueue { } #[cfg(test)] -mod tests { +mod test { use super::*; use uuid::Uuid; diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index bbd3543e1..81933831a 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -567,7 +567,7 @@ where } #[cfg(test)] -mod tests { +mod test { use anyhow::bail; use mockall::Sequence; diff --git a/crates/propolis-server-config/src/lib.rs b/crates/propolis-server-config/src/lib.rs index a9f50647a..d6cd72661 100644 --- a/crates/propolis-server-config/src/lib.rs +++ b/crates/propolis-server-config/src/lib.rs @@ -143,7 +143,7 @@ pub fn parse>(path: P) -> Result { } #[cfg(test)] -mod tests { +mod test { use super::*; #[test] diff --git a/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 501286b9f..4e5e66cc9 100644 --- a/lib/propolis-client/src/support.rs +++ b/lib/propolis-client/src/support.rs @@ -449,7 +449,7 @@ fn _assert_impls() { } #[cfg(test)] -mod tests { +mod test { use super::InstanceSerialConsoleControlMessage; use super::InstanceSerialConsoleHelper; use super::Role; diff --git a/lib/propolis/src/chardev/pollers.rs b/lib/propolis/src/chardev/pollers.rs index 4a6b3e61a..05e024d80 100644 --- a/lib/propolis/src/chardev/pollers.rs +++ b/lib/propolis/src/chardev/pollers.rs @@ -555,7 +555,7 @@ impl Params { } #[cfg(test)] -mod tests { +mod test { use super::*; use crate::chardev::*; diff --git a/lib/propolis/src/common.rs b/lib/propolis/src/common.rs index 701a09fdf..558fe2d85 100644 --- a/lib/propolis/src/common.rs +++ b/lib/propolis/src/common.rs @@ -388,7 +388,7 @@ pub const GB: usize = 1024 * 1024 * 1024; pub const TB: usize = 1024 * 1024 * 1024 * 1024; #[cfg(test)] -mod tests { +mod test { use super::*; #[test] diff --git a/lib/propolis/src/firmware/smbios/table.rs b/lib/propolis/src/firmware/smbios/table.rs index ad49a999e..ef131f39e 100644 --- a/lib/propolis/src/firmware/smbios/table.rs +++ b/lib/propolis/src/firmware/smbios/table.rs @@ -416,7 +416,7 @@ pub mod type1 { } #[cfg(test)] - mod tests { + mod test { use super::*; enum_serde_roundtrip_tests! { @@ -607,7 +607,7 @@ pub mod type4 { } #[cfg(test)] - mod tests { + mod test { use super::*; enum_serde_roundtrip_tests! { @@ -759,7 +759,7 @@ pub mod type16 { } #[cfg(test)] - mod tests { + mod test { use super::*; enum_serde_roundtrip_tests! { diff --git a/lib/propolis/src/hw/nvme/queue.rs b/lib/propolis/src/hw/nvme/queue.rs index 6cedff37b..eb06efd52 100644 --- a/lib/propolis/src/hw/nvme/queue.rs +++ b/lib/propolis/src/hw/nvme/queue.rs @@ -877,7 +877,7 @@ pub(super) mod migrate { } #[cfg(test)] -mod tests { +mod test { use rand::Rng; use super::*; diff --git a/lib/propolis/src/hw/pci/bar.rs b/lib/propolis/src/hw/pci/bar.rs index 73ca3fa75..c1dfe8939 100644 --- a/lib/propolis/src/hw/pci/bar.rs +++ b/lib/propolis/src/hw/pci/bar.rs @@ -314,7 +314,7 @@ pub mod migrate { } #[cfg(test)] -mod tests { +mod test { use super::*; fn setup() -> Bars { diff --git a/lib/propolis/src/hw/uart/uart16550.rs b/lib/propolis/src/hw/uart/uart16550.rs index 61f044dc0..c9b2b2af9 100644 --- a/lib/propolis/src/hw/uart/uart16550.rs +++ b/lib/propolis/src/hw/uart/uart16550.rs @@ -635,7 +635,7 @@ impl UartReg { } #[cfg(test)] -mod tests { +mod test { mod bits { #![allow(unused)] diff --git a/lib/propolis/src/util/aspace.rs b/lib/propolis/src/util/aspace.rs index 147f97959..d3cf6f721 100644 --- a/lib/propolis/src/util/aspace.rs +++ b/lib/propolis/src/util/aspace.rs @@ -280,7 +280,7 @@ impl<'a, T> Iterator for Range<'a, T> { } #[cfg(test)] -mod tests { +mod test { use super::*; #[test] diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs index ef1dc0e7e..f6185ceb7 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/lib/propolis/src/vmm/mem.rs @@ -1039,7 +1039,7 @@ impl<'a, T: Copy> Iterator for MemMany<'a, T> { } #[cfg(test)] -pub mod tests { +pub mod test { use super::*; const TEST_LEN: usize = 16 * 1024; diff --git a/xtask/src/main.rs b/xtask/src/main.rs index c431b8ed3..e39c6b273 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -10,6 +10,7 @@ mod task_fmt; mod task_license; mod task_phd; mod task_prepush; +mod task_style; mod util; #[derive(Parser)] @@ -43,6 +44,8 @@ enum Cmds { #[clap(subcommand)] cmd: task_phd::Cmd, }, + /// Perform misc style checks + Style, } fn main() -> Result<()> { @@ -64,5 +67,11 @@ fn main() -> Result<()> { println!("Pre-push checks pass"); Ok(()) } + Cmds::Style => { + task_style::cmd_style()?; + + println!("Style checks pass"); + Ok(()) + } } } diff --git a/xtask/src/task_prepush.rs b/xtask/src/task_prepush.rs index 2a981e7d4..794c6fe1a 100644 --- a/xtask/src/task_prepush.rs +++ b/xtask/src/task_prepush.rs @@ -4,7 +4,7 @@ use anyhow::{bail, Result}; -use crate::{task_clippy, task_fmt, task_license}; +use crate::{task_clippy, task_fmt, task_license, task_style}; pub(crate) fn cmd_prepush() -> Result<()> { let mut errs = Vec::new(); @@ -17,6 +17,9 @@ pub(crate) fn cmd_prepush() -> Result<()> { if task_license::cmd_license().is_err() { errs.push("license"); } + if task_style::cmd_style().is_err() { + errs.push("style"); + } if !errs.is_empty() { bail!("Pre-push error(s) in: {}", errs.join(", ")) diff --git a/xtask/src/task_style.rs b/xtask/src/task_style.rs new file mode 100644 index 000000000..44f2fe636 --- /dev/null +++ b/xtask/src/task_style.rs @@ -0,0 +1,68 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use std::collections::BTreeSet; +use std::io::{BufRead, BufReader}; +use std::process::{Command, Stdio}; + +use anyhow::Result; + +use crate::util::*; + +fn check_test_names() -> Result<()> { + let wroot = workspace_root()?; + + // Get listing of all tests (excluding doctests) + let mut cmd = Command::new("cargo"); + let child = cmd + .args([ + "test", + "--workspace", + "--all-targets", + "--", + "--list", + "--format=terse", + ]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .current_dir(&wroot) + .spawn()?; + + let problem_mods = BufReader::new(child.stdout.expect("stdout is present")) + .lines() + .map_while(std::result::Result::ok) + .filter_map(|line| { + // Look for ": test" + let test_name = match line.rsplit_once(": ") { + Some((p, "test")) => p, + _ => return None, + }; + + // Check for `mod tests` instead of `mod test` as the last component of + // the test name (prior to the test function name itself); + let mut name_parts = test_name.rsplit("::"); + match (name_parts.next(), name_parts.next()) { + (_fn_name, Some("tests")) => { + Some(test_name.rsplit_once("::").unwrap().0.to_owned()) + } + _ => None, + } + }) + .collect::>(); + + if !problem_mods.is_empty() { + eprintln!("The following test module paths should use `mod test` instead of `mod tests`:"); + for path in problem_mods { + eprintln!("\t{path}"); + } + Err(anyhow::anyhow!("Unconforming test module names")) + } else { + Ok(()) + } +} + +pub(crate) fn cmd_style() -> Result<()> { + check_test_names() +}