Add tests and add is_forever_unstable method to the Package

Author: JurajSadel

xtask/src/commands/release/execute_plan.rs

@@ -4,7 +4,6 @@ use anyhow::{Context, Result, bail, ensure};
 use clap::Args;
 use esp_metadata::Chip;
 use strum::IntoEnumIterator;
-use toml_edit::{Item, Value};
 
 use crate::{
     cargo::CargoToml,
@@ -66,26 +65,12 @@ pub fn execute_plan(workspace: &Path, args: ApplyPlanArgs) -> Result<()> {
             );
         }
 
-        if let Some(metadata) = package.espressif_metadata()
-            && let Some(Item::Value(forever_unstable)) = metadata.get("forever_unstable")
-        {
-            // Special case: some packages are perma-unstable, meaning they won't ever have
-            // a stable release. For these packages, we always use a
-            // patch release.
-            let forever_unstable = if let Value::Boolean(forever_unstable) = forever_unstable {
-                *forever_unstable.value()
-            } else {
-                log::warn!("Invalid value for 'forever_unstable' in metadata - must be a boolean");
-                true
-            };
-
-            if forever_unstable && step.bump != VersionBump::Patch {
-                bail!(
-                    "Cannot bump perma-unstable package {} to a non-patch version",
-                    step.package
-                );
-            }
-        };
+        if package.package.is_forever_unstable() && step.bump != VersionBump::Patch {
+            bail!(
+                "Cannot bump perma-unstable package {} to a non-patch version",
+                step.package
+            );
+        }
 
         let new_version = update_package(&mut package, &step.bump, !args.no_dry_run)?;
 

xtask/src/commands/release/plan.rs

@@ -6,7 +6,6 @@ use clap::Args;
 use esp_metadata::Chip;
 use serde::{Deserialize, Serialize};
 use strum::IntoEnumIterator;
-use toml_edit::{Item, Value};
 
 use crate::{
     Package,
@@ -135,24 +134,7 @@ pub fn plan(workspace: &Path, args: PlanArgs) -> Result<()> {
             let amount = if package.is_semver_checked() {
                 min_package_update(workspace, package, &all_chips)?
             } else {
-                let forever_unstable = if let Some(metadata) =
-                    package_tomls[&package].espressif_metadata()
-                    && let Some(Item::Value(forever_unstable)) = metadata.get("forever-unstable")
-                {
-                    // Special case: some packages are perma-unstable, meaning they won't ever have
-                    // a stable release. For these packages, we always use a
-                    // patch release.
-                    if let Value::Boolean(forever_unstable) = forever_unstable {
-                        *forever_unstable.value()
-                    } else {
-                        log::warn!(
-                            "Invalid value for 'forever-unstable' in metadata - must be a boolean"
-                        );
-                        true
-                    }
-                } else {
-                    false
-                };
+                let forever_unstable = package_tomls[&package].package.is_forever_unstable();
 
                 if forever_unstable {
                     ReleaseType::Patch

xtask/src/lib.rs

@@ -464,6 +464,22 @@ impl Package {
             .as_bool()
             .expect("semver-checked must be a boolean")
     }
+
+    #[cfg(feature = "semver-checks")]
+    fn is_forever_unstable(&self) -> bool {
+        match self
+            .toml()
+            .espressif_metadata()
+            .and_then(|m| m.get("forever-unstable"))
+        {
+            Some(Item::Value(Value::Boolean(b))) => *b.value(),
+            Some(Item::Value(_)) => {
+                log::warn!("Invalid value for 'forever-unstable' in metadata - must be a boolean");
+                true
+            }
+            _ => false,
+        }
+    }
 }
 
 #[derive(Debug, Clone, Copy, strum::Display, clap::ValueEnum, Serialize, Deserialize)]

xtask/src/semver_check.rs

@@ -7,7 +7,6 @@ use std::{
 use anyhow::{Context, Error};
 use cargo_semver_checks::{Check, GlobalConfig, ReleaseType, Rustdoc};
 use esp_metadata::{Chip, Config};
-use toml_edit::{Item, Value};
 
 use crate::{Package, cargo::CargoArgsBuilder, commands::checker::download_baselines};
 
@@ -47,40 +46,15 @@ pub fn minimum_update(
     let mut cfg = GlobalConfig::new();
     cfg.set_log_level(Some(log::Level::Info));
     let result = semver_check.check_release(&mut cfg)?;
+    log::info!("Result {:?}", result);
 
-    let mut min_required_update = ReleaseType::Patch;
-    for (_, report) in result.crate_reports() {
-        if let Some(required_bump) = report.required_bump() {
-            let required_is_stricter = (min_required_update == ReleaseType::Patch)
-                || (required_bump == ReleaseType::Major);
-            if required_is_stricter {
-                min_required_update = required_bump;
-            }
-        }
-    }
-
-    let forever_unstable = package
-        .toml()
-        .espressif_metadata()
-        .and_then(|metadata| metadata.get("forever-unstable"))
-        .map(|item| match item {
-            Item::Value(Value::Boolean(b)) => *b.value(),
-            Item::Value(_) => {
-                log::warn!("Invalid value for 'forever-unstable' in metadata - must be a boolean");
-                true
-            }
-            _ => false,
-        })
-        .unwrap_or(false);
-
-    if forever_unstable && min_required_update == ReleaseType::Major {
-        log::warn!(
-            "Downgrading required bump from Minor to Patch for unstable package: {}",
-            package
-        );
-        min_required_update = ReleaseType::Minor;
-    }
+    let required_bumps: Vec<ReleaseType> = result
+        .crate_reports()
+        .into_iter()
+        .filter_map(|(_, report)| report.required_bump())
+        .collect();
 
+    let min_required_update = required_bump(&required_bumps, package.is_forever_unstable());
     Ok(min_required_update)
 }
 
@@ -142,3 +116,80 @@ pub(crate) fn build_doc_json(
         .with_context(|| format!("Failed to run `cargo rustdoc` with {cargo_args:?}",))?;
     Ok(current_path)
 }
+
+fn required_bump(required_bumps: &[ReleaseType], forever_unstable: bool) -> ReleaseType {
+    let mut min_required_update = ReleaseType::Patch;
+
+    for &required_bump in required_bumps {
+        let required_is_stricter =
+            (min_required_update == ReleaseType::Patch) || (required_bump == ReleaseType::Major);
+
+        if required_is_stricter {
+            min_required_update = required_bump;
+        }
+    }
+
+    if forever_unstable && min_required_update == ReleaseType::Major {
+        log::warn!("Downgrading required bump from Minor to Patch for unstable package",);
+        min_required_update = ReleaseType::Minor;
+    }
+
+    min_required_update
+}
+
+#[cfg(test)]
+mod tests {
+
+    use super::*;
+
+    #[test]
+    // Semver-check requiring a major bump for a 0.x crate
+    fn major_bump_from_0x_to_0x_plus_1() {
+        let bumps = [ReleaseType::Major];
+
+        let result = required_bump(&bumps, false);
+
+        assert_eq!(result, ReleaseType::Major);
+    }
+
+    #[test]
+    // For crates >= 1.0.0, Major is still Major
+    fn major_bump_from_1x_to_2x() {
+        let bumps = [ReleaseType::Major];
+
+        let result = required_bump(&bumps, false);
+
+        assert_eq!(result, ReleaseType::Major);
+    }
+
+    #[test]
+    fn forever_unstable_downgrades_major_to_minor() {
+        let bumps = [ReleaseType::Major];
+
+        let result = required_bump(&bumps, true);
+
+        assert_eq!(
+            result,
+            ReleaseType::Minor,
+            "forever-unstable packages must never require a major bump"
+        );
+    }
+
+    #[test]
+    fn minor_stays_minor() {
+        let bumps = [ReleaseType::Minor];
+
+        let result = required_bump(&bumps, false);
+
+        assert_eq!(result, ReleaseType::Minor);
+    }
+
+    #[test]
+    fn multiple_bumps_select_major() {
+        let bumps = [ReleaseType::Patch, ReleaseType::Minor, ReleaseType::Major];
+
+        let result = required_bump(&bumps, false);
+
+        assert_eq!(result, ReleaseType::Major);
+    }
+}