diff --git a/src/common_test.rs b/src/common_test.rs index fdb0ffa..511e5ab 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -185,6 +185,53 @@ pub mod tests { ) } + pub fn build_ownership_with_directory_codeowners_with_glob_metacharacters() -> Result> { + ownership!( + TestProjectFile { + relative_path: "app/{old}/.codeowner".to_owned(), + content: "Bar\n".to_owned(), + }, + TestProjectFile { + relative_path: "!legacy/.codeowner".to_owned(), + content: "Foo\n".to_owned(), + } + ) + } + + #[cfg(unix)] + pub fn build_ownership_with_directory_codeowner_with_an_asterisk() -> Result> { + ownership!(TestProjectFile { + relative_path: "app/*/.codeowner".to_owned(), + content: "Baz\n".to_owned(), + }) + } + + pub fn build_ownership_with_directory_codeowners_at_the_project_root() -> Result> { + ownership!( + TestProjectFile { + relative_path: ".codeowner".to_owned(), + content: "Bam\n".to_owned(), + }, + TestProjectFile { + relative_path: "app/.codeowner".to_owned(), + content: "Bar\n".to_owned(), + } + ) + } + + pub fn build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team() -> Result> { + ownership!( + TestProjectFile { + relative_path: "app/by_handle/.codeowner".to_owned(), + content: "@Bar\n".to_owned(), + }, + TestProjectFile { + relative_path: "app/typo/.codeowner".to_owned(), + content: "Typo\n".to_owned(), + } + ) + } + pub fn build_ownership_with_all_mappers() -> Result> { ownership!( TestProjectFile { @@ -249,6 +296,10 @@ pub mod tests { relative_path: "packs/[admin]/comp.ts".to_owned(), content: "// @team Bar\n".to_owned(), }, + TestProjectFile { + relative_path: "packs/{admin}/comp.ts".to_owned(), + content: "// @team Foo\n".to_owned(), + }, TestProjectFile { relative_path: "packs/bar/comp.rb".to_owned(), content: "# @team Bar\n".to_owned(), @@ -307,6 +358,32 @@ pub mod tests { let test_config = TestConfig::new(temp_dir.path().to_path_buf(), vec![]); build_ownership(test_config) } + + pub fn build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters() -> Result> { + let temp_dir = tempdir()?; + + let test_config = TestConfig { + team_names: vec![], + ..TestConfig::new( + temp_dir.path().to_path_buf(), + vec![ + TestProjectFile { + relative_path: "config/teams/[ops].yml".to_owned(), + content: "name: Ops\ngithub:\n team: \"@Ops\"\nruby:\n owned_gems:\n - \"{taco}\"\n".to_owned(), + }, + TestProjectFile { + relative_path: "config/teams/{dev}.yml".to_owned(), + content: "name: Dev\ngithub:\n team: \"@Dev\"\n".to_owned(), + }, + TestProjectFile { + relative_path: "gems/{taco}/sauce.rb".to_owned(), + content: "class Taco::Sauce\nend\n".to_owned(), + }, + ], + ) + }; + build_ownership(test_config) + } pub fn build_ownership_with_package_codeowners() -> Result> { ownership!( TestProjectFile { @@ -328,6 +405,41 @@ pub mod tests { ) } + pub fn build_ownership_with_package_codeowners_with_glob_metacharacters() -> Result> { + ownership!( + TestProjectFile { + relative_path: "packs/[ab]/package.yml".to_owned(), + content: "owner: Foo\n".to_owned(), + }, + TestProjectFile { + relative_path: "packs/{legacy}/package.yml".to_owned(), + content: "owner: Bar\n".to_owned(), + } + ) + } + + pub fn build_ownership_with_package_codeowners_at_the_project_root() -> Result> { + let temp_dir = tempdir()?; + + let test_config = TestConfig { + code_ownership_config_yml: format!("{DEFAULT_CODE_OWNERSHIP_YML}ruby_package_paths:\n - \"**/*\"\n"), + ..TestConfig::new( + temp_dir.path().to_path_buf(), + vec![ + TestProjectFile { + relative_path: "package.yml".to_owned(), + content: "owner: Foo\n".to_owned(), + }, + TestProjectFile { + relative_path: "packs/inner/package.yml".to_owned(), + content: "owner: Bar\n".to_owned(), + }, + ], + ) + }; + build_ownership(test_config) + } + pub fn vecs_match(a: &Vec, b: &Vec) { // First check lengths match assert_eq!(a.len(), b.len(), "Vectors have different lengths"); diff --git a/src/ownership.rs b/src/ownership.rs index 9cefbd7..ba34b9a 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -179,37 +179,18 @@ impl Ownership { pub(crate) fn owner_lookup(&self) -> OwnerLookup { // `validate` reports a package.yml, package.json or `.codeowner` naming a team that doesn't exist, but a list of files - // isn't checked for that, so here each still names an owner for the files under its directory. The package mappers drop - // such a package, and the directory mapper's glob misses a `.codeowner` at the project root and reads braces as alternatives, - // matching `app/legacy` for one in `app/{legacy}`. So the directory is matched as a path prefix, which replaces that glob - // for such a `.codeowner`: brackets and braces are literal and the root covers every file. + // isn't checked for that, so here each still names an owner for the files under its directory. The directory mapper + // matches a `.codeowner` whatever team it names, but the package mappers drop such a package, so its directory is matched + // as a path prefix. let package_roots = self .project .packages .iter() .filter(|package| !self.project.teams_by_name.contains_key(&package.owner)) .filter_map(|package| package.package_root()); - let directory_roots = self - .project - .directory_codeowner_files - .iter() - .filter(|directory_codeowner_file| !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner)) - .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root()); OwnerLookup { - owner_matchers: self - .mappers() - .iter() - .flat_map(|mapper| mapper.owner_matchers()) - .filter(|owner_matcher| match owner_matcher { - OwnerMatcher::Glob { - source: Source::Directory(_), - team_name, - .. - } => self.project.teams_by_name.contains_key(team_name), - _ => true, - }) - .collect(), - unknown_team_roots: package_roots.chain(directory_roots).map(Path::to_path_buf).collect(), + owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(), + unknown_team_roots: package_roots.map(Path::to_path_buf).collect(), } } diff --git a/src/ownership/file_owner_finder.rs b/src/ownership/file_owner_finder.rs index e89de74..34dff4c 100644 --- a/src/ownership/file_owner_finder.rs +++ b/src/ownership/file_owner_finder.rs @@ -97,4 +97,18 @@ mod tests { Some((&team_name_longest, &source_longest)) ); } + + #[test] + fn test_directory_overrider_ranks_the_project_root_below_a_top_level_directory() { + // The `.codeowner` files come from a parallel walk, so either can be processed first. + let (root_team, root) = ("root_team".to_string(), Source::Directory("".to_string())); + let (app_team, app) = ("app_team".to_string(), Source::Directory("app".to_string())); + for order in [[(&root_team, &root), (&app_team, &app)], [(&app_team, &app), (&root_team, &root)]] { + let mut directory_overrider = DirectoryOverrider::default(); + for (team_name, source) in order { + directory_overrider.process(team_name, source); + } + assert_eq!(directory_overrider.specific_directory_owner(), Some((&app_team, &app))); + } + } } diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 23a5fcc..bc2efe4 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -10,7 +10,10 @@ use glob::glob; use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache, yaml::NodeLimit}; -use super::{FileOwner, mapper::Source}; +use super::{ + FileOwner, + mapper::{Source, directory_glob}, +}; pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) -> Result, String> { let absolute_file_path = if file_path.is_absolute() { @@ -278,7 +281,7 @@ fn nearest_package_owner( Ok(owner) => { if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { let package_path = parent_rel.join("package.yml"); - let package_glob = format!("{rel_str}/**/**"); + let package_glob = directory_glob(rel_str); return Some(( team.name.clone(), Source::Package(package_path.to_string_lossy().to_string(), package_glob), @@ -300,7 +303,7 @@ fn nearest_package_owner( && let Some(team) = teams_by_name.get(&owner) { let package_path = parent_rel.join("package.json"); - let package_glob = format!("{rel_str}/**/**"); + let package_glob = directory_glob(rel_str); return Some(( team.name.clone(), Source::Package(package_path.to_string_lossy().to_string(), package_glob), @@ -584,6 +587,36 @@ mod tests { } } + #[test] + fn test_nearest_package_owner_describes_a_package_by_the_glob_validate_matches() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/{payroll}"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nowner: Payroll\n").unwrap(); + let js_pkg = project_root.join("frontend/[flow]"); + std::fs::create_dir_all(&js_pkg).unwrap(); + std::fs::write(js_pkg.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap(); + std::fs::write(project_root.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Payroll", "UX"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + for (file, package_path, glob) in [ + ("packs/{payroll}/a.rb", "packs/{payroll}/package.yml", "packs/\\{payroll\\}/**/**"), + ("frontend/[flow]/a.ts", "frontend/[flow]/package.json", "frontend/\\[flow\\]/**/**"), + ("app/a.ts", "package.json", "**/**"), + ] { + let (_, source) = nearest_package_owner(project_root, Path::new(file), &config, &tbn).unwrap(); + assert_eq!(source, Source::Package(package_path.to_string(), glob.to_string())); + } + } + #[test] fn test_vendored_gem_owner() { let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index 867e620..b566b2e 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -15,6 +15,7 @@ mod team_yml_mapper; pub use annotated_file_mapper::TeamFileMapper; pub use directory_mapper::DirectoryMapper; +pub use escaper::directory_glob; pub use package_mapper::JavascriptPackageMapper; pub use package_mapper::RubyPackageMapper; pub use team_gem_mapper::TeamGemMapper; @@ -26,6 +27,8 @@ use super::Entry; pub trait Mapper { fn name(&self) -> String; fn entries(&self) -> Vec; + /// A matcher names a team that exists by its `name`, even where a file names it by GitHub team, since `validate` counts + /// each name matching a file as a separate owner. fn owner_matchers(&self) -> Vec; } pub type TeamName = String; @@ -43,7 +46,7 @@ pub enum Source { impl Display for Source { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Source::Directory(path) => write!(f, "Owner specified in `{}/.codeowner`", path), + Source::Directory(path) => write!(f, "Owner specified in `{}`", Path::new(path).join(".codeowner").display()), Source::AnnotatedFile => write!(f, "Owner annotation at the top of the file"), Source::TeamGem => write!(f, "Owner specified in Team YML's `owned_gems`"), Source::TeamGlob(glob) => write!(f, "Owner specified in Team YML as an owned_glob `{}`", glob), @@ -58,7 +61,8 @@ impl Display for Source { impl Source { pub fn len(&self) -> usize { match self { - Source::Directory(path) => path.matches('/').count(), + // Counting components rather than separators ranks the project root, "", below a top-level directory. + Source::Directory(path) => Path::new(path).components().count(), _ => 0, } } @@ -200,6 +204,7 @@ mod tests { Source::Directory("packs/bam".to_string()).to_string(), "Owner specified in `packs/bam/.codeowner`" ); + assert_eq!(Source::Directory("".to_string()).to_string(), "Owner specified in `.codeowner`"); assert_eq!(Source::AnnotatedFile.to_string(), "Owner annotation at the top of the file"); assert_eq!(Source::TeamGem.to_string(), "Owner specified in Team YML's `owned_gems`"); assert_eq!( diff --git a/src/ownership/mapper/annotated_file_mapper.rs b/src/ownership/mapper/annotated_file_mapper.rs index ec69230..cbc61c2 100644 --- a/src/ownership/mapper/annotated_file_mapper.rs +++ b/src/ownership/mapper/annotated_file_mapper.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use std::sync::Arc; use super::Entry; -use super::escaper::escape_brackets; +use super::escaper::escape_glob; use super::{Mapper, OwnerMatcher}; use crate::ownership::mapper::Source; use crate::project::Project; @@ -31,7 +31,7 @@ impl Mapper for TeamFileMapper { let relative_path = self.project.relative_path(&owned_file.path); entries.push(Entry { - path: escape_brackets(&relative_path.to_string_lossy()), + path: escape_glob(&relative_path.to_string_lossy()), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -87,6 +87,12 @@ mod tests { team_name: "Bar".to_owned(), disabled: false, }, + Entry { + path: "packs/\\{admin\\}/comp.ts".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, Entry { path: "packs/jscomponents/comp.ts".to_owned(), github_team: "@Foo".to_owned(), @@ -142,6 +148,7 @@ mod tests { let expected_owner_matchers = vec![OwnerMatcher::ExactMatches( HashMap::from([ (PathBuf::from("packs/[admin]/comp.ts"), "Bar".to_owned()), + (PathBuf::from("packs/{admin}/comp.ts"), "Foo".to_owned()), (PathBuf::from("packs/bar/comp.rb"), "Bar".to_owned()), (PathBuf::from("packs/jscomponents/comp.ts"), "Foo".to_owned()), (PathBuf::from("packs/jscomponents/comp-colon.ts"), "Foo".to_owned()), diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 295e842..6dcdaf5 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use super::escaper::escape_brackets; +use super::escaper::directory_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::Project; @@ -28,7 +28,7 @@ impl Mapper for DirectoryMapper { let team = team_by_name.get(&directory_codeowner_file.owner); if let Some(team) = team { entries.push(Entry { - path: format!("{}/**/**", escape_brackets(&dir_root)), + path: directory_glob(&dir_root), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -43,12 +43,16 @@ impl Mapper for DirectoryMapper { let mut owner_matchers = Vec::new(); for file in &self.project.directory_codeowner_files { + // A `.codeowner` can name its team by GitHub team. + let team_name = self + .project + .teams_by_name + .get(&file.owner) + .map(|team| team.name.clone()) + .unwrap_or_else(|| file.owner.clone()); owner_matchers.push(OwnerMatcher::new_glob( - format!( - "{}/**/**", - escape_brackets(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()) - ), - file.owner.to_owned(), + directory_glob(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()), + team_name, Source::Directory(file.directory_root().map(|p| p.to_string_lossy().to_string()).unwrap_or_default()), )); } @@ -65,8 +69,13 @@ impl Mapper for DirectoryMapper { mod tests { use std::error::Error; + #[cfg(unix)] + use crate::common_test::tests::build_ownership_with_directory_codeowner_with_an_asterisk; use crate::common_test::tests::{ - build_ownership_with_directory_codeowners, build_ownership_with_directory_codeowners_with_brackets, vecs_match, + build_ownership_with_directory_codeowners, build_ownership_with_directory_codeowners_at_the_project_root, + build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team, + build_ownership_with_directory_codeowners_with_brackets, build_ownership_with_directory_codeowners_with_glob_metacharacters, + vecs_match, }; use super::*; @@ -172,4 +181,135 @@ mod tests { ); Ok(()) } + + #[test] + fn test_owner_matchers_name_a_team_by_its_name() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob( + "app/by_handle/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/by_handle".to_owned()), + ), + OwnerMatcher::new_glob( + "app/typo/**/**".to_owned(), + "Typo".to_owned(), + Source::Directory("app/typo".to_owned()), + ), + ], + ); + Ok(()) + } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_with_glob_metacharacters()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "app/\\{old\\}/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + Entry { + path: "\\!legacy/**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_with_glob_metacharacters()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob( + "app/\\{old\\}/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/{old}".to_owned()), + ), + OwnerMatcher::new_glob( + "\\!legacy/**/**".to_owned(), + "Foo".to_owned(), + Source::Directory("!legacy".to_owned()), + ), + ], + ); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn test_entries_and_owner_matchers_with_an_asterisk() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowner_with_an_asterisk()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![Entry { + path: "app/\\*/**/**".to_owned(), + github_team: "@Baz".to_owned(), + team_name: "Baz".to_owned(), + disabled: false, + }], + ); + vecs_match( + &mapper.owner_matchers(), + &vec![OwnerMatcher::new_glob( + "app/\\*/**/**".to_owned(), + "Baz".to_owned(), + Source::Directory("app/*".to_owned()), + )], + ); + Ok(()) + } + + #[test] + fn test_entries_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_at_the_project_root()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "**/**".to_owned(), + github_team: "@Bam".to_owned(), + team_name: "Bam".to_owned(), + disabled: false, + }, + Entry { + path: "app/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_at_the_project_root()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob("**/**".to_owned(), "Bam".to_owned(), Source::Directory("".to_owned())), + OwnerMatcher::new_glob("app/**/**".to_owned(), "Bar".to_owned(), Source::Directory("app".to_owned())), + ], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/escaper.rs b/src/ownership/mapper/escaper.rs index bc1e36d..ca09a25 100644 --- a/src/ownership/mapper/escaper.rs +++ b/src/ownership/mapper/escaper.rs @@ -1,3 +1,94 @@ -pub fn escape_brackets(path: &str) -> String { - path.replace("[", "\\[").replace("]", "\\]") +/// `path` with a `\` before each character that fast_glob reads as a pattern, so a glob built from it matches it literally. `!` is +/// one, since it negates a glob it starts. Nothing else is escaped, since fast_glob reads `\b`, `\n`, `\r` and `\t` as control +/// characters. +pub fn escape_glob(path: &str) -> String { + let mut escaped = String::with_capacity(path.len()); + for c in path.chars() { + if matches!(c, '\\' | '*' | '?' | '[' | ']' | '{' | '}' | '!') { + escaped.push('\\'); + } + escaped.push(c); + } + escaped +} + +/// The glob for every file under `directory`, a project-relative path. That's `**/**` for the project root, the empty path, which +/// CODEOWNERS gets as `/**/**`. +pub fn directory_glob(directory: &str) -> String { + if directory.is_empty() { + "**/**".to_owned() + } else { + format!("{}/**/**", escape_glob(directory)) + } +} + +#[cfg(test)] +mod tests { + use fast_glob::glob_match; + + use super::*; + + #[test] + fn test_escape_glob_escapes_each_metacharacter() { + assert_eq!(escape_glob("a*b"), "a\\*b"); + assert_eq!(escape_glob("a?b"), "a\\?b"); + assert_eq!(escape_glob("[ab]"), "\\[ab\\]"); + assert_eq!(escape_glob("{a,b}"), "\\{a,b\\}"); + assert_eq!(escape_glob("!a"), "\\!a"); + assert_eq!(escape_glob("a!"), "a\\!"); + assert_eq!(escape_glob("a\\b"), "a\\\\b"); + } + + #[test] + fn test_escape_glob_escapes_a_backslash_once() { + assert_eq!(escape_glob("a\\[b"), "a\\\\\\[b"); + assert_eq!(escape_glob("a\\\\b"), "a\\\\\\\\b"); + } + + #[test] + fn test_escape_glob_leaves_other_characters_alone() { + let path = "app/b n r t/(x)/a,b-c^d/e.rb"; + assert_eq!(escape_glob(path), path); + } + + #[test] + fn test_directory_glob() { + assert_eq!(directory_glob("packs/[ab]"), "packs/\\[ab\\]/**/**"); + assert_eq!(directory_glob(""), "**/**"); + assert!(glob_match(directory_glob(""), "x.rb")); + assert!(glob_match(directory_glob(""), "app/x.rb")); + } + + #[test] + fn test_directory_glob_matches_only_its_own_directory() { + // Each name, with directories its unescaped glob would also match. + let names: &[(&str, &[&str])] = &[ + ("[ab]", &["a", "b"]), + ("{a,b}", &["a", "b"]), + ("{legacy}", &["legacy"]), + ("a*", &["a", "ab"]), + ("a?", &["ab"]), + ("!x", &["legacy"]), + ("a\\b", &[]), + ("a]", &[]), + ("a}", &[]), + ("(x)", &[]), + ("a,b", &[]), + ]; + for (name, siblings) in names { + let glob = directory_glob(name); + assert!(glob_match(&glob, format!("{name}/f.rb")), "{glob} should match {name}/f.rb"); + assert!(glob_match(&glob, format!("{name}/x/f.rb")), "{glob} should match {name}/x/f.rb"); + for sibling in *siblings { + assert!( + glob_match(format!("{name}/**/**"), format!("{sibling}/f.rb")), + "the unescaped glob for {name} should match {sibling}/f.rb" + ); + assert!( + !glob_match(&glob, format!("{sibling}/f.rb")), + "{glob} shouldn't match {sibling}/f.rb" + ); + } + } + } } diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index 1ab5d34..bf98a04 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use super::escaper::directory_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::{Package, PackageType, Project}; @@ -75,7 +76,7 @@ impl PackageMapper { if let Some(team) = team { entries.push(Entry { - path: format!("{}/**/**", package_root), + path: directory_glob(&package_root), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -105,9 +106,9 @@ impl PackageMapper { if let Some(team) = team { owner_matchers.push(OwnerMatcher::new_glob( - format!("{}/**/**", package_root), + directory_glob(&package_root), team.name.to_owned(), - Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)), + Source::Package(package.path.to_string_lossy().to_string(), directory_glob(&package_root)), )); } } @@ -139,7 +140,11 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> { mod tests { use super::*; use crate::{ - common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_package_codeowners, vecs_match}, + common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_package_codeowners, + build_ownership_with_package_codeowners_at_the_project_root, build_ownership_with_package_codeowners_with_glob_metacharacters, + vecs_match, + }, ownership::mapper::RubyPackageMapper, project::{Package, PackageType}, }; @@ -180,6 +185,36 @@ mod tests { assert_eq!(package_paths, vec!["packs/a/package.yml", "packs/c/package.yml"]); } + #[test] + fn test_remove_nested_packages_with_a_package_at_the_project_root() { + let packages = [ + Package { + path: Path::new("packs/a/package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_a".to_owned(), + }, + Package { + path: Path::new("package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_root".to_owned(), + }, + Package { + path: Path::new("packs/b/package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_b".to_owned(), + }, + ]; + + let packages = packages.iter().collect_vec(); + + let package_paths = super::remove_nested_packages(&packages) + .iter() + .map(|package| package.path.to_str().unwrap()) + .collect_vec(); + + assert_eq!(package_paths, vec!["package.yml"]); + } + #[test] fn test_entries() -> Result<(), Box> { let ownership = build_ownership_with_all_mappers()?; @@ -217,4 +252,90 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_with_glob_metacharacters()?; + let mapper = RubyPackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "packs/\\[ab\\]/**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + Entry { + path: "packs/\\{legacy\\}/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_with_glob_metacharacters()?; + let mapper = PackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(&PackageType::Ruby), + &vec![ + OwnerMatcher::new_glob( + "packs/\\[ab\\]/**/**".to_owned(), + "Foo".to_owned(), + Source::Package("packs/[ab]/package.yml".to_owned(), "packs/\\[ab\\]/**/**".to_owned()), + ), + OwnerMatcher::new_glob( + "packs/\\{legacy\\}/**/**".to_owned(), + "Bar".to_owned(), + Source::Package("packs/{legacy}/package.yml".to_owned(), "packs/\\{legacy\\}/**/**".to_owned()), + ), + ], + ); + Ok(()) + } + + #[test] + fn test_entries_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_at_the_project_root()?; + let mapper = RubyPackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + Entry { + path: "packs/inner/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_at_the_project_root() -> Result<(), Box> { + // The root package encloses the other, so only it is matched, as for any nested package. + let ownership = build_ownership_with_package_codeowners_at_the_project_root()?; + let mapper = PackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(&PackageType::Ruby), + &vec![OwnerMatcher::new_glob( + "**/**".to_owned(), + "Foo".to_owned(), + Source::Package("package.yml".to_owned(), "**/**".to_owned()), + )], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/team_gem_mapper.rs b/src/ownership/mapper/team_gem_mapper.rs index 9d74914..ab1554a 100644 --- a/src/ownership/mapper/team_gem_mapper.rs +++ b/src/ownership/mapper/team_gem_mapper.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use super::escaper::escape_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::Project; @@ -24,8 +25,9 @@ impl Mapper for TeamGemMapper { let vendored_gem = vendored_gem_by_name.get(owned_gem); if let Some(vendored_gem) = vendored_gem { + let gem_path = escape_glob(&self.project.relative_path(&vendored_gem.path).to_string_lossy()); entries.push(Entry { - path: format!("{}/**/**", self.project.relative_path(&vendored_gem.path).to_string_lossy()), + path: format!("{gem_path}/**/**"), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -46,8 +48,9 @@ impl Mapper for TeamGemMapper { let vendored_gem = vendored_gem_by_name.get(owned_gem); if let Some(vendored_gem) = vendored_gem { + let gem_path = escape_glob(&self.project.relative_path(&vendored_gem.path).to_string_lossy()); owner_matchers.push(OwnerMatcher::new_glob( - format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()), + format!("{gem_path}/**/*"), team.name.clone(), Source::TeamGem, )); @@ -67,7 +70,10 @@ impl Mapper for TeamGemMapper { mod tests { use std::error::Error; - use crate::common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_team_gem_codeowners, vecs_match}; + use crate::common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_team_gem_codeowners, + build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters, vecs_match, + }; use super::*; #[test] @@ -100,4 +106,35 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamGemMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![Entry { + path: "gems/\\{taco\\}/**/**".to_owned(), + github_team: "@Ops".to_owned(), + team_name: "Ops".to_owned(), + disabled: false, + }], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamGemMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![OwnerMatcher::new_glob( + "gems/\\{taco\\}/**/*".to_owned(), + "Ops".to_owned(), + Source::TeamGem, + )], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index be4230b..3be6505 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -41,7 +41,7 @@ impl Mapper for TeamGlobMapper { owner_matchers.push(OwnerMatcher::new_glob_with_candidate_subtracted_globs( owned_glob.clone(), &team_subtracted_globs, - team.github_team.clone(), + team.name.clone(), Source::TeamGlob(owned_glob.clone()), )) } @@ -90,7 +90,7 @@ mod tests { &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( "packs/bar/**".to_owned(), &[], - "@Baz".to_owned(), + "Baz".to_owned(), Source::TeamGlob("packs/bar/**".to_owned()), )], ); @@ -107,7 +107,7 @@ mod tests { &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( "packs/bar/**".to_owned(), &["packs/bar/excluded/**".to_owned()], - "@Baz".to_owned(), + "Baz".to_owned(), Source::TeamGlob("packs/bar/**".to_owned()), )], ); diff --git a/src/ownership/mapper/team_yml_mapper.rs b/src/ownership/mapper/team_yml_mapper.rs index c05c417..83bd096 100644 --- a/src/ownership/mapper/team_yml_mapper.rs +++ b/src/ownership/mapper/team_yml_mapper.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use std::sync::Arc; use super::Entry; +use super::escaper::escape_glob; use super::{Mapper, OwnerMatcher}; use crate::ownership::mapper::Source; use crate::project::Project; @@ -23,7 +24,7 @@ impl Mapper for TeamYmlMapper { for team in &self.project.teams { entries.push(Entry { - path: self.project.relative_path(&team.path).to_string_lossy().to_string(), + path: escape_glob(&self.project.relative_path(&team.path).to_string_lossy()), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -52,7 +53,10 @@ impl Mapper for TeamYmlMapper { mod tests { use std::error::Error; - use crate::common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_team_yml_codeowners, vecs_match}; + use crate::common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters, + build_ownership_with_team_yml_codeowners, vecs_match, + }; use super::*; #[test] @@ -109,4 +113,28 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamYmlMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "config/teams/\\[ops\\].yml".to_owned(), + github_team: "@Ops".to_owned(), + team_name: "Ops".to_owned(), + disabled: false, + }, + Entry { + path: "config/teams/\\{dev\\}.yml".to_owned(), + github_team: "@Dev".to_owned(), + team_name: "Dev".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index e291755..3952046 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -292,10 +292,12 @@ impl Error { format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name) }; if let Some(inherits_from) = inherits_from { - message.push_str(&format!( - "; this directory is currently inheriting its owner from {}", + let inherits_from = if inherits_from.as_os_str().is_empty() { + "the project root".into() + } else { inherits_from.to_string_lossy() - )); + }; + message.push_str(&format!("; this directory is currently inheriting its owner from {inherits_from}")); } vec![message] } diff --git a/tests/optionally_owned_globs_test.rs b/tests/optionally_owned_globs_test.rs index c916837..583f0b8 100644 --- a/tests/optionally_owned_globs_test.rs +++ b/tests/optionally_owned_globs_test.rs @@ -506,7 +506,26 @@ fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_naming Ok(()) } -// A glob built from the project root, `/**/**`, would match no project-relative path. +// `validate` reports an empty `.codeowner` like one naming a team that doesn't exist. +fn add_an_empty_codeowner_file_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "") +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_under_an_empty_codeowner() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_empty_codeowner_file_in_an_optionally_owned_glob, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + +// The project root's glob is `**/**`, since `/**/**` would be `/**/**` there and match no project-relative path. fn make_every_file_optionally_owned_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root( project_root: &Path, ) -> std::io::Result<()> { @@ -529,7 +548,7 @@ fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_at_the Ok(()) } -// Read as a glob, `app/deprecated/{legacy}` would match `app/deprecated/legacy` rather than its own directory. +// Unescaped, `app/deprecated/{legacy}/**/**` would match files in `app/deprecated/legacy` rather than its own directory. fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("app/deprecated/{legacy}"))?; fs::write(project_root.join("app/deprecated/{legacy}/.codeowner"), "Typo\n")?; diff --git a/tests/path_globs_test.rs b/tests/path_globs_test.rs new file mode 100644 index 0000000..2abc86d --- /dev/null +++ b/tests/path_globs_test.rs @@ -0,0 +1,310 @@ +//! The globs built from a package's or `.codeowner`'s directory, or an annotated file's path, are matched by `validate` and written +//! to CODEOWNERS, where `for-file --from-codeowners` and `validate ` match them again. + +use assert_cmd::{assert::Assert, prelude::*}; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; +use tempfile::TempDir; + +mod common; +use common::{git_add_all_files, init_git_repo}; + +// The default ruby_package_paths don't match the project root. +const RUBY_PACKAGES_ANYWHERE: &str = "owned_globs:\n - \"**/*.rb\"\nruby_package_paths:\n - \"**/*\"\n"; + +// A git repo of `files`, teams Foo and Bar and a config owning `**/*.rb`, any of which `files` can replace, with CODEOWNERS +// generated. +fn generated_project(files: &[(&str, &str)]) -> Result> { + let temp_dir = tempfile::tempdir()?; + let project_root = temp_dir.path(); + let defaults = [ + ("config/code_ownership.yml", "owned_globs:\n - \"**/*.rb\"\n"), + ("config/teams/foo.yml", "name: Foo\ngithub:\n team: \"@org/foo\"\n"), + ("config/teams/bar.yml", "name: Bar\ngithub:\n team: \"@org/bar\"\n"), + ]; + for (path, contents) in defaults.iter().chain(files) { + let path = project_root.join(path); + fs::create_dir_all(path.parent().ok_or("a file needs a parent directory")?)?; + fs::write(path, contents)?; + } + init_git_repo(project_root); + git_add_all_files(project_root); + codeowners(project_root, &["generate"])?.success(); + Ok(temp_dir) +} + +fn codeowners(project_root: &Path, args: &[&str]) -> Result> { + Ok(Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert()) +} + +// CODEOWNERS without its comments and blank lines. +fn codeowners_rules(project_root: &Path) -> Result, Box> { + Ok(fs::read_to_string(project_root.join(".github/CODEOWNERS"))? + .lines() + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(str::to_owned) + .collect()) +} + +fn assert_owner(project_root: &Path, file: &str, team: &str) -> Result<(), Box> { + for args in [["for-file", file].as_slice(), &["for-file", "--from-codeowners", file]] { + codeowners(project_root, args)? + .success() + .stdout(predicate::str::starts_with(format!("Team: {team}\n"))); + } + Ok(()) +} + +// The project from #146: each name with glob metacharacters has a sibling that its glob would match instead if unescaped. +fn project_with_glob_metacharacters_in_names() -> Result> { + generated_project(&[ + ("packs/[ab]/package.yml", "owner: Foo\n"), + ("packs/{legacy}/package.yml", "owner: Foo\n"), + ("packs/legacy/package.yml", "owner: Bar\n"), + ("app/{old}/.codeowner", "Foo\n"), + ("app/old/.codeowner", "Bar\n"), + ("lib/{v2}.rb", "# @team Foo\n"), + ("packs/[ab]/x.rb", "puts 1\n"), + ("packs/a/y.rb", "puts 1\n"), + ("packs/{legacy}/x.rb", "puts 1\n"), + ("packs/legacy/z.rb", "puts 1\n"), + ("app/{old}/x.rb", "puts 1\n"), + ("app/old/z.rb", "puts 1\n"), + ("lib/v2.rb", "puts 1\n"), + ]) +} + +const OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES: [(&str, &str); 8] = [ + ("packs/[ab]/x.rb", "Foo"), + ("packs/a/y.rb", "Unowned"), + ("packs/{legacy}/x.rb", "Foo"), + ("packs/legacy/z.rb", "Bar"), + ("app/{old}/x.rb", "Foo"), + ("app/old/z.rb", "Bar"), + ("lib/{v2}.rb", "Foo"), + ("lib/v2.rb", "Unowned"), +]; + +#[test] +fn test_generate_escapes_glob_metacharacters_in_names() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/app/\\{old\\}/**/** @org/foo", + "/app/old/**/** @org/bar", + "/packs/\\[ab\\]/**/** @org/foo", + "/packs/\\{legacy\\}/**/** @org/foo", + "/packs/legacy/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + "/lib/\\{v2\\}.rb @org/foo", + ] + ); + Ok(()) +} + +#[test] +fn test_for_file_agrees_with_codeowners_for_names_with_glob_metacharacters() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + for (file, team) in OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["crosscheck-owners"])? + .success() + .stdout(predicate::str::contains( + "Success! All files match between CODEOWNERS and for-file command.", + )); + Ok(()) +} + +#[test] +fn test_for_file_describes_a_package_by_the_glob_validate_matches() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + codeowners(project.path(), &["for-file", "packs/{legacy}/x.rb"])? + .success() + .stdout(predicate::eq(indoc! {" + Team: Foo + Github Team: @org/foo + Team YML: config/teams/foo.yml + Description: + - Owner defined in `packs/{legacy}/package.yml` with implicitly owned glob: `packs/\\{legacy\\}/**/**` + "})); + Ok(()) +} + +#[test] +fn test_validate_reads_glob_metacharacters_in_names_literally() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + codeowners(project.path(), &["validate"])?.failure().stdout(predicate::eq(indoc! {" + + Some files are missing ownership + - lib/v2.rb + - packs/a/y.rb + + "})); + let files = OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES.map(|(file, _)| file); + codeowners(project.path(), &[["validate"].as_slice(), &files].concat())? + .failure() + .stdout(predicate::eq("Unowned files detected:\n packs/a/y.rb\n lib/v2.rb\n")); + Ok(()) +} + +#[test] +fn test_validate_reads_a_leading_exclamation_mark_in_a_directory_name_literally() -> Result<(), Box> { + // Unescaped, `!legacy/**/**` would match every file not under `legacy`, so `validate` would give app/x.rb an owner. + let project = generated_project(&[ + ("!legacy/.codeowner", "Foo\n"), + ("!legacy/x.rb", "puts 1\n"), + ("app/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/\\!legacy/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + codeowners(project.path(), &["validate"])? + .failure() + .stdout(predicate::eq("\nSome files are missing ownership\n- app/x.rb\n\n")); + Ok(()) +} + +#[test] +fn test_a_codeowner_file_at_the_project_root_owns_every_file() -> Result<(), Box> { + let project = generated_project(&[(".codeowner", "Foo\n"), ("app/x.rb", "puts 1\n")])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + assert_owner(project.path(), "app/x.rb", "Foo")?; + codeowners(project.path(), &["for-file", "app/x.rb"])? + .success() + .stdout(predicate::str::ends_with("Description:\n- Owner specified in `.codeowner`\n")); + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + codeowners(project.path(), &["validate", "app/x.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_package_at_the_project_root_owns_every_file() -> Result<(), Box> { + let project = generated_project(&[ + ("config/code_ownership.yml", RUBY_PACKAGES_ANYWHERE), + ("package.yml", "owner: Foo\n"), + ("app/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + assert_owner(project.path(), "app/x.rb", "Foo")?; + codeowners(project.path(), &["for-file", "app/x.rb"])? + .success() + .stdout(predicate::str::ends_with( + "Description:\n- Owner defined in `package.yml` with implicitly owned glob: `**/**`\n", + )); + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + codeowners(project.path(), &["validate", "app/x.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_codeowner_file_in_a_directory_overrides_one_at_the_project_root() -> Result<(), Box> { + // If `validate` took the root's owner for annotated.rb, the file would have two. + let project = generated_project(&[ + (".codeowner", "Foo\n"), + ("app/.codeowner", "Bar\n"), + ("app/x.rb", "puts 1\n"), + ("app/annotated.rb", "# @team Bar\n"), + ("lib/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/app/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + "/app/annotated.rb @org/bar", + ] + ); + for (file, team) in [("app/x.rb", "Bar"), ("app/annotated.rb", "Bar"), ("lib/x.rb", "Foo")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_validate_counts_a_team_reached_two_ways_as_one_owner() -> Result<(), Box> { + // If `validate` counted a team glob's or a `.codeowner`'s team by its GitHub team, each file would have two owners. + let project = generated_project(&[ + ( + "config/teams/foo.yml", + "name: Foo\ngithub:\n team: \"@org/foo\"\nowned_globs:\n - \"lib/**\"\n", + ), + (".codeowner", "Foo\n"), + ("lib/x.rb", "puts 1\n"), + ("app/.codeowner", "@org/bar\n"), + ("app/annotated.rb", "# @team Bar\n"), + ])?; + for (file, team) in [("lib/x.rb", "Foo"), ("app/annotated.rb", "Bar")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_package_nested_in_one_at_the_project_root_owns_its_files() -> Result<(), Box> { + let project = generated_project(&[ + ("config/code_ownership.yml", RUBY_PACKAGES_ANYWHERE), + ("package.yml", "owner: Foo\n"), + ("packs/inner/package.yml", "owner: Bar\n"), + ("app/x.rb", "puts 1\n"), + ("packs/inner/y.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/packs/inner/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + for (file, team) in [("app/x.rb", "Foo"), ("packs/inner/y.rb", "Bar")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_validate_names_the_project_root_as_what_a_codeowner_file_with_an_invalid_team_inherits_from() -> Result<(), Box> { + let project = generated_project(&[(".codeowner", "Foo\n"), ("app/.codeowner", "Typo\n"), ("app/x.rb", "puts 1\n")])?; + codeowners(project.path(), &["validate"])?.failure().stdout(predicate::str::contains( + "- app/.codeowner is referencing an invalid team - 'Typo'; this directory is currently inheriting its owner from the project root\n", + )); + Ok(()) +}