better formatting for multiple ownership validation

Author: perryqh

src/ownership/validator.rs

@@ -173,16 +173,16 @@ impl Error {
     pub fn messages(&self) -> Vec<String> {
         match self {
             Error::FileWithoutOwner { path } => vec![format!("- {}", path.to_string_lossy())],
-            Error::FileWithMultipleOwners { path, owners } => owners
-                .iter()
-                .flat_map(|owner| {
-                    owner
-                        .sources
-                        .iter()
-                        .map(|source| format!("- {} (owner: {}, source: {})", path.to_string_lossy(), owner.team_name, &source))
-                        .collect_vec()
-                })
-                .collect_vec(),
+            Error::FileWithMultipleOwners { path, owners } => {
+                let mut output = vec![format!("\n{}", path.to_string_lossy().to_string())];
+                for owner in owners.iter().sorted_by_key(|owner| owner.team_name.to_lowercase()) {
+                    output.push(format!(" owner: {}", owner.team_name));
+                    for source in &owner.sources {
+                        output.push(format!("  - {}", source));
+                    }
+                }
+                vec![output.join("\n")]
+            }
             Error::CodeownershipFileIsStale => vec![],
             Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)],
         }

tests/invalid_project_test.rs

@@ -4,36 +4,38 @@ use std::{error::Error, process::Command};
 
 #[test]
 fn test_validate() -> Result<(), Box<dyn Error>> {
+    let expected_output = r#" 
+CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file
+
+Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways
+
+gems/payroll_calculator/calculator.rb
+ owner: Payments
+  - Owner annotation at the top of the file
+ owner: Payroll
+  - Owner specified in Team YML's `owned_gems`
+
+ruby/app/services/multi_owned.rb
+ owner: Payments
+  - Owner annotation at the top of the file
+ owner: Payroll
+  - Owner specified in `ruby/app/services/.codeowner`
+
+Found invalid team annotations
+- ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'
+
+Some files are missing ownership
+- ruby/app/models/blockchain.rb
+- ruby/app/unowned.rb
+    "#
+    .trim();
     Command::cargo_bin("codeowners")?
         .arg("--project-root")
         .arg("tests/fixtures/invalid_project")
         .arg("validate")
         .assert()
         .failure()
-        .stdout(predicate::str::contains(
-            "CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file",
-        ))
-        .stdout(predicate::str::contains(
-            "Some files are missing ownership\n- ruby/app/models/blockchain.rb\n- ruby/app/unowned.rb",
-        ))
-        .stdout(predicate::str::contains(
-            "Found invalid team annotations\n- ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'",
-        ))
-        .stdout(predicate::str::contains(
-            "Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways",
-        ))
-        .stdout(predicate::str::contains(
-            "gems/payroll_calculator/calculator.rb (owner: Payments, source: Owner annotation at the top of the file)",
-        ))
-        .stdout(predicate::str::contains(
-            "gems/payroll_calculator/calculator.rb (owner: Payroll, source: Owner specified in Team YML's `owned_gems`)",
-        ))
-        .stdout(predicate::str::contains(
-            "ruby/app/services/multi_owned.rb (owner: Payments, source: Owner annotation at the top of the file)",
-        ))
-        .stdout(predicate::str::contains(
-            "ruby/app/services/multi_owned.rb (owner: Payroll, source: Owner specified in `ruby/app/services/.codeowner`",
-        ));
+        .stdout(predicate::str::contains(expected_output));
 
     Ok(())
 }

tests/valid_project_test.rs

@@ -48,6 +48,21 @@ fn test_for_file() -> Result<(), Box<dyn Error>> {
     Ok(())
 }
 
+#[test]
+fn test_for_file_with_2_ownerships() -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("codeowners")?
+        .arg("--project-root")
+        .arg("tests/fixtures/valid_project")
+        .arg("for-file")
+        .arg("javascript/packages/PayrollFlow/index.tsx")
+        .assert()
+        .success()
+        .stdout(predicate::str::contains("Team: Payroll"))
+        .stdout(predicate::str::contains("Team YML: config/teams/payroll.yml"));
+
+    Ok(())
+}
+
 #[test]
 fn test_for_team() -> Result<(), Box<dyn Error>> {
     let expected_stdout = r#"