feat: Don't collect nested tests

This change changes the valid structure of test suites, such that all tests must
be leave nodes. Along side this change comes a new command, `util migrate`,
which can be used to migrate such "internal" tests into their own directories.

This overall simplifies the structure of tests and makes operations on
directories containing them safer. Tests can now also use the reserved path
components because these directories never live alongside other tests.

Therefore tests such as this:

- foo/test.typ
- foo/bar/test.typ

must now be structured as

- foo/qux/test.typ
- foo/bar/test.typ

in order to be picked up, otherwise bar is not discovered.

This makes `test.typ` the only indicator for a test directory.

crates/typst-test-cli/src/cli/mod.rs

@@ -142,7 +142,19 @@ impl Context<'_> {
             |w| {
                 write!(w, "use '")?;
                 ui::write_colored(w, Color::Cyan, |w| write!(w, "all:"))?;
-                write!(w, "{expr}' to confirm using all tests")
+                writeln!(w, "{expr}' to confirm using all tests")
+            },
+        )
+    }
+
+    pub fn error_nested_tests(&self) -> io::Result<()> {
+        self.ui.error_hinted_with(
+            |w| writeln!(w, "Found nested tests"),
+            |w| {
+                writeln!(w, "This is no longer supported")?;
+                write!(w, "You can run ")?;
+                ui::write_colored(w, Color::Cyan, |w| write!(w, "typst-test util migrate"))?;
+                writeln!(w, " to automatically fix the tests")
             },
         )
     }
@@ -223,7 +235,13 @@ impl Context<'_> {
 
     /// Collect and filter tests for the given project.
     pub fn collect_tests(&self, project: &Project, set: &TestSet) -> eyre::Result<Suite> {
+        if !util::migrate::collect_old_structure(project.paths(), "self")?.is_empty() {
+            self.error_nested_tests()?;
+            eyre::bail!(OperationFailure);
+        }
+
         let suite = Suite::collect(project.paths(), set)?;
+
         Ok(suite)
     }
 

crates/typst-test-cli/src/cli/util/fonts.rs

@@ -9,6 +9,7 @@ use crate::ui::Indented;
 use crate::{kit, ui};
 
 #[derive(clap::Args, Debug, Clone)]
+#[group(id = "util-font-args")]
 pub struct Args {
     /// List variants alongside fonts
     #[arg(long)]

crates/typst-test-cli/src/cli/util/migrate.rs

@@ -0,0 +1,185 @@
+use std::collections::{BTreeMap, BTreeSet};
+use std::fs;
+use std::io::Write;
+use std::path::{Path, PathBuf};
+
+use color_eyre::eyre::{self, WrapErr};
+use lib::project::Paths;
+use lib::stdx;
+use lib::test::Id;
+use termcolor::Color;
+
+use crate::cli::Context;
+use crate::ui;
+
+#[derive(clap::Args, Debug, Clone)]
+#[group(id = "util-migrate-args")]
+pub struct Args {
+    /// Confirm the migration
+    #[arg(long)]
+    pub confirm: bool,
+
+    /// The name of the new sub directories the tests get moved to
+    #[arg(long, default_value = "self")]
+    pub name: String,
+}
+
+pub fn run(ctx: &mut Context, args: &Args) -> eyre::Result<()> {
+    let project = ctx.project()?;
+    let paths = project.paths();
+    let mut w = ctx.ui.stderr();
+
+    let mappings = collect_old_structure(paths, &args.name)?;
+
+    if mappings.is_empty() {
+        writeln!(w, "No tests need to be moved")?;
+        return Ok(());
+    }
+
+    if args.confirm {
+        writeln!(w, "Moving tests:")?;
+    } else {
+        writeln!(w, "These tests would be moved:")?;
+    }
+
+    for (old, (new, collision)) in &mappings {
+        if *collision {
+            ui::write_bold_colored(&mut w, Color::Red, |w| write!(w, "*"))?;
+            write!(w, " ")?;
+        } else {
+            write!(w, "  ")?;
+        }
+        ui::write_test_id(&mut w, old)?;
+        write!(w, " -> ")?;
+        ui::write_test_id(&mut w, new)?;
+        writeln!(w)?;
+    }
+
+    writeln!(w)?;
+
+    let mut has_colission = false;
+    for (old, (new, collision)) in &mappings {
+        if !*collision {
+            migrate_test(paths, old, new)?;
+        } else {
+            has_colission = true;
+        }
+    }
+
+    if has_colission {
+        ctx.ui.hint_with(|w| {
+            ui::write_bold_colored(w, Color::Red, |w| write!(w, "*"))?;
+            writeln!(
+                w,
+                " denotes paths which were excluded because of another test with the same id."
+            )?;
+            write!(w, "Try another name using ")?;
+            ui::write_colored(w, Color::Cyan, |w| write!(w, "--name"))?;
+            writeln!(w)
+        })?;
+    }
+
+    if !args.confirm {
+        ctx.ui.warning("Make sure to back up your code!")?;
+
+        ctx.ui.hint_with(|w| {
+            write!(w, "Use ")?;
+            ui::write_colored(w, Color::Cyan, |w| write!(w, "--confirm"))?;
+            writeln!(w, " to move the tests automatically")
+        })?;
+        ctx.ui.hint_with(|w| {
+            write!(w, "Use ")?;
+            ui::write_colored(w, Color::Cyan, |w| write!(w, "--name"))?;
+            writeln!(w, " to configure the sub directory name")
+        })?;
+    }
+
+    Ok(())
+}
+
+pub fn collect_old_structure(
+    paths: &Paths,
+    migration_name: &str,
+) -> eyre::Result<BTreeMap<Id, (Id, bool)>> {
+    let mut entries = BTreeSet::new();
+    for entry in paths.test_root().read_dir()? {
+        let entry = entry?;
+        if entry.metadata()?.is_dir() {
+            collect_old_structure_inner(paths, &entry.path(), &mut entries)?;
+        }
+    }
+
+    let mut mappings = BTreeMap::new();
+    'outer: for id in &entries {
+        'inner: for internal in id.ancestors().skip(1) {
+            if !entries.contains(internal) {
+                continue 'inner;
+            }
+
+            let old = Id::new(internal)?;
+            let new = Id::new(format!("{internal}/{migration_name}"))?;
+            let colission = entries.contains(&new);
+
+            if mappings.insert(old, (new, colission)).is_some() {
+                continue 'outer;
+            }
+        }
+    }
+
+    Ok(mappings)
+}
+
+fn collect_old_structure_inner(
+    paths: &Paths,
+    path: &Path,
+    entries: &mut BTreeSet<Id>,
+) -> eyre::Result<()> {
+    if path.join("test.typ").try_exists()? {
+        entries.insert(Id::new_from_path(path.strip_prefix(paths.test_root())?)?);
+    }
+
+    for entry in fs::read_dir(path).wrap_err_with(|| format!("{path:?}"))? {
+        let entry = entry?;
+        let path = entry.path();
+        let name = entry.file_name();
+
+        if name == "ref" || name == "out" || name == "diff" {
+            continue;
+        }
+
+        if entry.metadata()?.is_dir() {
+            collect_old_structure_inner(paths, &path, entries)?;
+        }
+    }
+
+    Ok(())
+}
+
+fn migrate_test_part(
+    paths: &Paths,
+    old: &Id,
+    new: &Id,
+    f: fn(&Paths, &Id) -> PathBuf,
+) -> eyre::Result<()> {
+    let old = f(paths, old);
+    let new = f(paths, new);
+
+    if old.try_exists()? {
+        fs::rename(&old, &new).wrap_err(format!("moving {old:?} to {new:?}"))?;
+    }
+
+    Ok(())
+}
+
+fn migrate_test(paths: &Paths, old: &Id, new: &Id) -> eyre::Result<()> {
+    let test_dir = paths.test_dir(new);
+    stdx::fs::create_dir(&test_dir, true).wrap_err(format!("creating to {test_dir:?}"))?;
+    migrate_test_part(paths, old, new, Paths::test_script)?;
+    migrate_test_part(paths, old, new, Paths::test_ref_script)?;
+    migrate_test_part(paths, old, new, Paths::test_ref_dir)?;
+    let out_dir = paths.test_out_dir(old);
+    stdx::fs::remove_dir(&out_dir, true).wrap_err(format!("removing to {out_dir:?}"))?;
+    let diff_dir = paths.test_diff_dir(old);
+    stdx::fs::remove_dir(&diff_dir, true).wrap_err(format!("removing to {diff_dir:?}"))?;
+    Ok(())
+}

crates/typst-test-cli/src/cli/util/mod.rs

@@ -5,6 +5,7 @@ use super::Context;
 pub mod about;
 pub mod clean;
 pub mod fonts;
+pub mod migrate;
 
 #[derive(clap::Args, Debug, Clone)]
 #[group(id = "util-args")]
@@ -27,6 +28,10 @@ pub enum Command {
     /// List all available fonts
     #[command()]
     Fonts(fonts::Args),
+
+    /// Migrate the test structure to the new version
+    #[command()]
+    Migrate(migrate::Args),
 }
 
 impl Command {
@@ -35,6 +40,7 @@ impl Command {
             Command::About => about::run(ctx),
             Command::Clean => clean::run(ctx),
             Command::Fonts(args) => fonts::run(ctx, args),
+            Command::Migrate(args) => migrate::run(ctx, args),
         }
     }
 }

crates/typst-test-lib/src/test/id.rs

@@ -15,20 +15,12 @@ use thiserror::Error;
 /// Each part of the path must be a simple id containing only ASCII
 /// alpha-numeric characters, dashes `-` or underscores `_` and start with an
 /// alphabetic character. This restriction may be lifted in the future.
-///
-/// Some ids are reserved for special folders:
-/// - `ref`: reference image storage
-/// - `out`: output image storage
-/// - `diff`: difference image storage
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize)]
 pub struct Id(EcoString);
 
 impl Id {
     /// The test component separator.
     pub const SEPARATOR: &'static str = "/";
-
-    /// The reserved ids, these cannot be used as parts in a path.
-    pub const RESERVED_COMPONENTS: &'static [&'static str] = &["ref", "out", "diff"];
 }
 
 impl Id {
@@ -116,7 +108,6 @@ impl Id {
     /// assert!( Id::is_valid("a"));
     /// assert!(!Id::is_valid("a//b"));  // empty component
     /// assert!(!Id::is_valid("a/"));    // empty component
-    /// assert!(!Id::is_valid("a/ref")); // reserved component
     /// ```
     pub fn is_valid<S: AsRef<str>>(string: S) -> bool {
         Self::validate(string).is_ok()
@@ -139,7 +130,6 @@ impl Id {
     /// assert!( Id::is_component_valid("a1"));
     /// assert!(!Id::is_component_valid("1a"));  // invalid char
     /// assert!(!Id::is_component_valid("a "));  // invalid char
-    /// assert!(!Id::is_component_valid("ref")); // reserved component
     /// ```
     pub fn is_component_valid<S: AsRef<str>>(component: S) -> bool {
         Self::validate_component(component).is_ok()
@@ -153,10 +143,6 @@ impl Id {
             return Err(ParseIdError::Empty);
         }
 
-        if let Some(reserved) = Self::RESERVED_COMPONENTS.iter().find(|&&r| r == component) {
-            return Err(ParseIdError::ReservedFragment(reserved));
-        }
-
         let mut chars = component.chars().peekable();
         if !chars.next().unwrap().is_ascii_alphabetic() {
             return Err(ParseIdError::InvalidFragment);
@@ -454,14 +440,11 @@ pub enum ParseIdError {
     #[error("id contained an invalid fragment")]
     InvalidFragment,
 
-    /// An id contained a reserved fragment.
-    #[error("id contained a reserved fragment: '{0}'")]
-    ReservedFragment(&'static str),
-
     /// An id contained empty or no fragments.
     #[error("id contained empty or no fragments")]
     Empty,
 }
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -513,7 +496,6 @@ mod tests {
         assert!(Id::new("/a").is_err());
         assert!(Id::new("a/").is_err());
         assert!(Id::new("a//b").is_err());
-        assert!(Id::new("a/ref").is_err());
 
         assert!(Id::new("a ").is_err());
         assert!(Id::new("1a").is_err());

crates/typst-test-lib/src/test/suite.rs

@@ -87,26 +87,19 @@ impl Suite {
                 tracing::debug!(id = %test.id(), "filtered test");
                 self.filtered.insert(id, test);
             }
-        }
-
-        for entry in fs::read_dir(&abs)? {
-            let entry = entry?;
-
-            if entry.metadata()?.is_dir() {
-                let abs = entry.path();
-                let rel = abs
-                    .strip_prefix(paths.test_root())
-                    .expect("entry must be in full");
-
-                tracing::trace!(path = ?rel, "reading directory entry");
-
-                let name = entry.file_name();
-
-                if Id::RESERVED_COMPONENTS.iter().any(|&r| r == name) {
-                    continue;
+        } else {
+            for entry in fs::read_dir(&abs)? {
+                let entry = entry?;
+
+                if entry.metadata()?.is_dir() {
+                    let abs = entry.path();
+                    let rel = abs
+                        .strip_prefix(paths.test_root())
+                        .expect("entry must be in full");
+
+                    tracing::trace!(path = ?rel, "reading directory entry");
+                    self.collect_dir(paths, rel, test_set)?;
                 }
-
-                self.collect_dir(paths, rel, test_set)?;
             }
         }