Add new host-test xtask subcommand (#4085)

playfulFence · web-flow · commit 1597443bf1a8 · 2025-09-09T14:44:36.000Z
* Add new `host-test` xtask subcommand

+ smaller consistency and fmt fixes

* reviews

* reviews
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -188,16 +188,5 @@ jobs:
       # Check metadata generation for all packages:
       - run: cargo xtask update-metadata --check
 
-      # Run tests in esp-config
-      - run: cd esp-config && cargo test --features build,tui
-
-      # Run tests in esp-bootloader-esp-idf
-      - run: cd esp-bootloader-esp-idf && cargo test --features=std
-
-      # Run tests in esp-storage
-      - run: cd esp-storage && cargo test --features=emulation -- --test-threads=1
-      - run: cd esp-storage && cargo test --features=emulation,bytewise-read -- --test-threads=1
-
-      # Miri tests in esp-storage
-      - run: cd esp-storage && cargo +nightly miri test --features=emulation -- --test-threads=1
-      - run: cd esp-storage && cargo +nightly miri test --features=emulation,bytewise-read -- --test-threads=1
+      # Run host tests for all applicable packages: 
+      - run: cargo xtask host-tests
diff --git a/esp-config/src/generate/mod.rs b/esp-config/src/generate/mod.rs
@@ -572,7 +572,7 @@ fn screaming_snake_case(name: &str) -> String {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
     use crate::generate::{validator::Validator, value::Value};
 
diff --git a/esp-config/src/generate/value.rs b/esp-config/src/generate/value.rs
@@ -185,7 +185,7 @@ impl From<String> for Value {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     #[test]
diff --git a/esp-config/src/lib.rs b/esp-config/src/lib.rs
@@ -69,7 +69,7 @@ macro_rules! esp_config_int_parse {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     // We can only test success in the const context
     const _: () = {
         core::assert!(esp_config_int_parse!(i64, "-77777") == -77777);
diff --git a/esp-storage/src/nor_flash.rs b/esp-storage/src/nor_flash.rs
@@ -189,7 +189,7 @@ impl MultiwriteNorFlash for FlashStorage {}
 
 // Run the tests with `--test-threads=1` - the emulation is not multithread safe
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     const WORD_SIZE: u32 = 4;
diff --git a/xtask/README.md b/xtask/README.md
@@ -18,6 +18,7 @@ Commands:
   semver-check               Semver Checks
   check-changelog            Check the changelog for packages
   update-chip-support-table  Re-generate the chip support table in the esp-hal README
+  host-tests                 Run host tests for all the packages where they are present
   help                       Print this message or the help of the given subcommand(s)
 
 Options:
diff --git a/xtask/src/cargo.rs b/xtask/src/cargo.rs
@@ -107,7 +107,7 @@ fn get_cargo() -> String {
     cargo
 }
 
-#[derive(Debug, Default)]
+#[derive(Clone, Debug, Default)]
 pub struct CargoArgsBuilder {
     toolchain: Option<String>,
     subcommand: String,
diff --git a/xtask/src/changelog.rs b/xtask/src/changelog.rs
@@ -342,7 +342,7 @@ fn parse_tag_link(line: &str) -> Result<(&str, &str)> {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     #[test]
diff --git a/xtask/src/commands/release/bump_version.rs b/xtask/src/commands/release/bump_version.rs
@@ -345,7 +345,7 @@ fn finalize_placeholders(
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use toml_edit::DocumentMut;
 
     use super::*;
diff --git a/xtask/src/commands/release/execute_plan.rs b/xtask/src/commands/release/execute_plan.rs
@@ -102,10 +102,20 @@ pub fn execute_plan(workspace: &Path, args: ApplyPlanArgs) -> Result<()> {
         );
     }
 
-    let branch = make_git_changes(!args.no_dry_run, "release-branch", "Finalize crate releases")?;
-
-    open_pull_request(&branch, !args.no_dry_run, args.manual_pull_request, &plan_source, &plan)
-        .with_context(|| "Failed to open pull request")?;
+    let branch = make_git_changes(
+        !args.no_dry_run,
+        "release-branch",
+        "Finalize crate releases",
+    )?;
+
+    open_pull_request(
+        &branch,
+        !args.no_dry_run,
+        args.manual_pull_request,
+        &plan_source,
+        &plan,
+    )
+    .with_context(|| "Failed to open pull request")?;
 
     if !args.no_dry_run {
         println!(
@@ -151,7 +161,11 @@ pub(crate) fn make_git_changes(dry_run: bool, branch_name: &str, commit: &str) -
     if dry_run {
         println!("Dry run: would commit changes to branch: {branch_name}");
     } else {
-        Command::new("git").arg("add").arg(".").status().context("Failed to stage changes")?;
+        Command::new("git")
+            .arg("add")
+            .arg(".")
+            .status()
+            .context("Failed to stage changes")?;
         Command::new("git")
             .arg("commit")
             .arg("-m")
@@ -306,7 +320,7 @@ pub(crate) fn comparison_url(base: &str, url: &str, branch_name: &str) -> Result
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     #[test]
diff --git a/xtask/src/commands/release/plan.rs b/xtask/src/commands/release/plan.rs
@@ -335,15 +335,18 @@ fn topological_sort(dep_graph: &HashMap<Package, Vec<Package>>) -> Vec<Package>
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     #[test]
-    // Dependencies in this test do not always reflect real dependencies, it is only a test of correct function operation. 
+    // Dependencies in this test do not always reflect real dependencies, it is only a test of correct function operation.
     fn test_topological_sort() {
         let mut dep_graph = HashMap::new();
         dep_graph.insert(Package::EspHal, vec![Package::EspAlloc]);
-        dep_graph.insert(Package::EspHalEmbassy, vec![Package::EspHal, Package::EspRadio]);
+        dep_graph.insert(
+            Package::EspHalEmbassy,
+            vec![Package::EspHal, Package::EspRadio],
+        );
         dep_graph.insert(Package::EspRadio, vec![Package::EspHal]);
         dep_graph.insert(Package::EspAlloc, vec![]);
 
diff --git a/xtask/src/commands/release/post_release.rs b/xtask/src/commands/release/post_release.rs
@@ -1,11 +1,11 @@
 use std::fs;
 
-use anyhow::{Context, Result};
-use semver::Version;
-use super::execute_plan::make_git_changes;
 use super::PLACEHOLDER;
 use super::Plan;
+use super::execute_plan::make_git_changes;
 use crate::commands::comparison_url;
+use anyhow::{Context, Result};
+use semver::Version;
 
 pub fn post_release(workspace: &std::path::Path) -> Result<()> {
     // Read the release plan
diff --git a/xtask/src/commands/release/publish_plan.rs b/xtask/src/commands/release/publish_plan.rs
@@ -1,6 +1,6 @@
 use std::{path::Path, process::Command};
 
-use anyhow::{bail, ensure, Context, Result};
+use anyhow::{Context, Result, bail, ensure};
 use clap::Args;
 
 use crate::{
diff --git a/xtask/src/lib.rs b/xtask/src/lib.rs
@@ -135,6 +135,18 @@ impl Package {
         false
     }
 
+    pub fn has_host_tests(&self, workspace: &Path) -> bool {
+        let package_path = workspace.join(self.to_string()).join("src");
+
+        walkdir::WalkDir::new(package_path)
+            .into_iter()
+            .filter_map(Result::ok)
+            .filter(|e| e.path().extension().is_some_and(|ext| ext == "rs"))
+            .any(|entry| {
+                std::fs::read_to_string(entry.path()).map_or(false, |src| src.contains("#[test]"))
+            })
+    }
+
     /// Does the package need to be built with the standard library?
     pub fn needs_build_std(&self) -> bool {
         use Package::*;
@@ -572,6 +584,87 @@ pub fn format_package(workspace: &Path, package: Package, check: bool) -> Result
     Ok(())
 }
 
+pub fn run_host_tests(workspace: &Path, package: Package) -> Result<()> {
+    log::info!("Running host tests for package: {}", package);
+    let package_path = workspace.join(package.as_ref());
+
+    let cmd = CargoArgsBuilder::default();
+
+    match package {
+        Package::EspConfig => {
+            return cargo::run(
+                &cmd.clone()
+                    .subcommand("test")
+                    .features(&vec!["build".into(), "tui".into()])
+                    .build(),
+                &package_path,
+            );
+        }
+
+        Package::EspBootloaderEspIdf => {
+            return cargo::run(
+                &cmd.clone()
+                    .subcommand("test")
+                    .features(&vec!["std".into()])
+                    .build(),
+                &package_path,
+            );
+        }
+
+        Package::EspStorage => {
+            cargo::run(
+                &cmd.clone()
+                    .subcommand("test")
+                    .features(&vec!["emulation".into()])
+                    .arg("--")
+                    .arg("--test-threads=1")
+                    .build(),
+                &package_path,
+            )?;
+
+            cargo::run(
+                &cmd.clone()
+                    .subcommand("test")
+                    .features(&vec!["emulation".into(), "bytewise-read".into()])
+                    .arg("--")
+                    .arg("--test-threads=1")
+                    .build(),
+                &package_path,
+            )?;
+
+            log::info!("Running miri host tests for package: {}", package);
+
+            cargo::run(
+                &cmd.clone()
+                    .toolchain("nightly")
+                    .subcommand("miri")
+                    .subcommand("test")
+                    .features(&vec!["emulation".into()])
+                    .arg("--")
+                    .arg("--test-threads=1")
+                    .build(),
+                &package_path,
+            )?;
+
+            return cargo::run(
+                &cmd.clone()
+                    .toolchain("nightly")
+                    .subcommand("miri")
+                    .subcommand("test")
+                    .features(&vec!["emulation".into(), "bytewise-read".into()])
+                    .arg("--")
+                    .arg("--test-threads=1")
+                    .build(),
+                &package_path,
+            );
+        }
+        _ => Err(anyhow!(
+            "Instructions for host testing were not provided for: '{}'",
+            package,
+        )),
+    }
+}
+
 fn format_package_path(workspace: &Path, package_path: &Path, check: bool) -> Result<()> {
     // We need to list all source files since modules in `unstable_module!` macros
     // won't get picked up otherwise
diff --git a/xtask/src/main.rs b/xtask/src/main.rs
@@ -45,6 +45,8 @@ enum Cli {
     CheckChangelog(CheckChangelogArgs),
     /// Re-generate metadata and the chip support table in the esp-hal README.
     UpdateMetadata(UpdateMetadataArgs),
+    /// Run host-tests in the workspace with `cargo test`
+    HostTests(HostTestsArgs),
 }
 
 #[derive(Debug, Args)]
@@ -76,6 +78,13 @@ struct CleanArgs {
     packages: Vec<Package>,
 }
 
+#[derive(Debug, Args)]
+struct HostTestsArgs {
+    /// Package(s) to target.
+    #[arg(value_enum, default_values_t = Package::iter())]
+    packages: Vec<Package>,
+}
+
 #[derive(Debug, Args)]
 struct LintPackagesArgs {
     /// Package(s) to target.
@@ -176,6 +185,7 @@ fn main() -> Result<()> {
         Cli::SemverCheck(args) => semver_checks(&workspace, args),
         Cli::CheckChangelog(args) => check_changelog(&workspace, &args.packages, args.normalize),
         Cli::UpdateMetadata(args) => update_metadata(&workspace, args.check),
+        Cli::HostTests(args) => host_tests(&workspace, args),
     }
 }
 
@@ -513,3 +523,16 @@ fn run_ci_checks(workspace: &Path, args: CiArgs) -> Result<()> {
 
     runner.finish()
 }
+
+fn host_tests(workspace: &Path, args: HostTestsArgs) -> Result<()> {
+    let mut packages = args.packages;
+    packages.sort();
+
+    for package in packages {
+        if package.has_host_tests(workspace) {
+            xtask::run_host_tests(workspace, package)?;
+        }
+    }
+
+    Ok(())
+}

Original file line number	Diff line number	Diff line change
`@@ -572,7 +572,7 @@ fn screaming_snake_case(name: &str) -> String {`
`572`	`572`	`}`
`573`	`573`
`574`	`574`	`#[cfg(test)]`
`575`		`-mod test {`
	`575`	`+mod tests {`
`576`	`576`	`use super::*;`
`577`	`577`	`use crate::generate::{validator::Validator, value::Value};`
`578`	`578`
Original file line number	Diff line number	Diff line change
`@@ -185,7 +185,7 @@ impl From<String> for Value {`
`185`	`185`	`}`
`186`	`186`
`187`	`187`	`#[cfg(test)]`
`188`		`-mod test {`
	`188`	`+mod tests {`
`189`	`189`	`use super::*;`
`190`	`190`
`191`	`191`	`#[test]`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ macro_rules! esp_config_int_parse {`
`69`	`69`	`}`
`70`	`70`
`71`	`71`	`#[cfg(test)]`
`72`		`-mod test {`
	`72`	`+mod tests {`
`73`	`73`	`// We can only test success in the const context`
`74`	`74`	`const _: () = {`
`75`	`75`	`core::assert!(esp_config_int_parse!(i64, "-77777") == -77777);`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,7 @@ fn get_cargo() -> String {`
`107`	`107`	`cargo`
`108`	`108`	`}`
`109`	`109`
`110`		`-#[derive(Debug, Default)]`
	`110`	`+#[derive(Clone, Debug, Default)]`
`111`	`111`	`pub struct CargoArgsBuilder {`
`112`	`112`	`toolchain: Option<String>,`
`113`	`113`	`subcommand: String,`
Original file line number	Diff line number	Diff line change
`@@ -342,7 +342,7 @@ fn parse_tag_link(line: &str) -> Result<(&str, &str)> {`
`342`	`342`	`}`
`343`	`343`
`344`	`344`	`#[cfg(test)]`
`345`		`-mod test {`
	`345`	`+mod tests {`
`346`	`346`	`use super::*;`
`347`	`347`
`348`	`348`	`#[test]`
Original file line number	Diff line number	Diff line change
`@@ -345,7 +345,7 @@ fn finalize_placeholders(`
`345`	`345`	`}`
`346`	`346`
`347`	`347`	`#[cfg(test)]`
`348`		`-mod test {`
	`348`	`+mod tests {`
`349`	`349`	`use toml_edit::DocumentMut;`
`350`	`350`
`351`	`351`	`use super::*;`