Improve integration tests

This splits out the changes in the test code from mitsuhiko#532 in order to make that diff smaller.

It divides  the building of the test case files from the test case files on disk + running the test, which make the structs simpler & more obvious

cargo-insta/Cargo.toml

@@ -33,3 +33,4 @@ clap = {version = "=4.1", features = ["derive", "env"]}
 [dev-dependencies]
 walkdir = "2.3.1"
 similar= "2.2.1"
+itertools = "0.10.0"

cargo-insta/tests/main.rs

@@ -1,3 +1,18 @@
+/// Integration tests which allow creating a full repo, running `cargo-insta`
+/// and then checking the output.
+///
+/// We can write more docs if that would be helpful. For the moment one thing to
+/// be aware of: it seems the packages must have different names, or we'll see
+/// interference between the tests.
+///
+/// (That seems to be because they all share the same `target` directory, which
+/// cargo will confuse for each other if they share the same name. I haven't
+/// worked out why — this is the case even if the files are the same between two
+/// tests but with different commands — and those files exist in different
+/// temporary workspace dirs. (We could try to enforce different names, or give
+/// up using a consistent target directory for a cache, but it would slow down
+/// repeatedly running the tests locally. To demonstrate the effect, name crates
+/// the same...)
 use std::collections::HashMap;
 use std::env;
 use std::fs;
@@ -6,36 +21,56 @@ use std::process::Command;
 
 use ignore::WalkBuilder;
 use insta::assert_snapshot;
+use itertools::Itertools;
 use similar::udiff::unified_diff;
 use tempfile::TempDir;
 
-struct TestProject {
+struct TestFiles {
     files: HashMap<PathBuf, String>,
-    /// Temporary directory where the project is created
-    temp_dir: TempDir,
-    /// Path of this repo, so we can have it as a dependency in the test project
-    project_path: Option<PathBuf>,
-    /// File tree at start of test
-    file_tree: Option<String>,
 }
 
-fn workspace_path() -> PathBuf {
+impl TestFiles {
+    fn new() -> Self {
+        Self {
+            files: HashMap::new(),
+        }
+    }
+
+    fn add_file<P: AsRef<Path>>(mut self, path: P, content: String) -> Self {
+        self.files.insert(path.as_ref().to_path_buf(), content);
+        self
+    }
+
+    fn create_project(self) -> TestProject {
+        TestProject::new(self.files)
+    }
+}
+
+/// Path of the insta crate in this repo, which we use as a dependency in the test project
+fn insta_path() -> PathBuf {
     PathBuf::from(env!("CARGO_MANIFEST_DIR"))
         .parent()
         .unwrap()
+        .join("insta")
         .to_path_buf()
 }
 
+/// A shared `target` directory for all tests to use, to allow caching.
 fn target_dir() -> PathBuf {
     let target_dir = env::var("CARGO_TARGET_DIR")
         .map(PathBuf::from)
-        .unwrap_or_else(|_| workspace_path().join("target"))
+        .unwrap_or_else(|_| insta_path().join("target"))
         .join("test-projects");
     fs::create_dir_all(&target_dir).unwrap();
     target_dir
 }
 
 fn assert_success(output: &std::process::Output) {
+    // Print stderr. Cargo test hides this when tests are successful, but if a
+    // test successfully exectues a command but then fails (e.g. on a snapshot),
+    // we would otherwise lose any output from the command such as `dbg!`
+    // statements.
+    eprint!("{}", String::from_utf8_lossy(&output.stderr));
     assert!(
         output.status.success(),
         "Tests failed: {}\n{}",
@@ -44,47 +79,38 @@ fn assert_success(output: &std::process::Output) {
     );
 }
 
-impl TestProject {
-    fn new() -> Self {
-        Self {
-            files: HashMap::new(),
-            temp_dir: TempDir::new().unwrap(),
-            project_path: None,
-            file_tree: None,
-        }
-    }
-
-    fn add_file<P: AsRef<Path>>(mut self, path: P, content: String) -> Self {
-        self.files.insert(path.as_ref().to_path_buf(), content);
-        self
-    }
+struct TestProject {
+    /// Temporary directory where the project is created
+    workspace_dir: PathBuf,
+    /// Original files when the project is created.
+    files: HashMap<PathBuf, String>,
+    /// File tree when the test is created.
+    file_tree: String,
+}
 
-    fn create(mut self) -> Self {
-        let project_path = self.temp_dir.path();
-        let insta_path = workspace_path().join("insta");
+impl TestProject {
+    fn new(files: HashMap<PathBuf, String>) -> TestProject {
+        let workspace_dir = TempDir::new().unwrap().into_path();
 
         // Create files and replace $PROJECT_PATH in all files
-        for (path, content) in &self.files {
-            let full_path = project_path.join(path);
+        for (path, content) in &files {
+            let full_path = workspace_dir.join(path);
             if let Some(parent) = full_path.parent() {
                 fs::create_dir_all(parent).unwrap();
             }
-            let replaced_content = content.replace("$PROJECT_PATH", insta_path.to_str().unwrap());
+            let replaced_content = content.replace("$PROJECT_PATH", insta_path().to_str().unwrap());
             fs::write(full_path, replaced_content).unwrap();
         }
 
-        self.project_path = Some(project_path.to_path_buf());
-        self
+        TestProject {
+            files,
+            file_tree: Self::current_file_tree(&workspace_dir),
+            workspace_dir,
+        }
     }
-
-    fn cmd(&mut self) -> Command {
-        self.file_tree = Some(self.current_file_tree());
-        let project_path = self
-            .project_path
-            .as_ref()
-            .expect("Project has not been created yet. Call create() first.");
+    fn cmd(&self) -> Command {
         let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta"));
-        command.current_dir(project_path);
+        command.current_dir(self.workspace_dir.as_path());
         // Use the same target directory as other tests, consistent across test
         // run. This makes the compilation much faster (though do some tests
         // tread on the toes of others? We could have a different cache for each
@@ -98,7 +124,7 @@ impl TestProject {
 
     fn diff(&self, file_path: &str) -> String {
         let original_content = self.files.get(Path::new(file_path)).unwrap();
-        let file_path_buf = self.project_path.as_ref().unwrap().join(file_path);
+        let file_path_buf = self.workspace_dir.join(file_path);
         let updated_content = fs::read_to_string(&file_path_buf).unwrap();
 
         unified_diff(
@@ -113,27 +139,30 @@ impl TestProject {
         )
     }
 
-    fn current_file_tree(&self) -> String {
-        WalkBuilder::new(&self.temp_dir)
+    fn current_file_tree(workspace_dir: &Path) -> String {
+        WalkBuilder::new(workspace_dir)
             .filter_entry(|e| e.path().file_name() != Some(std::ffi::OsStr::new("target")))
             .build()
             .filter_map(|e| e.ok())
+            .sorted_by(|a, b| a.path().cmp(b.path()))
             .map(|entry| {
                 let path = entry
                     .path()
-                    .strip_prefix(&self.temp_dir)
+                    .strip_prefix(workspace_dir)
                     .unwrap_or(entry.path());
-                format!("{}{}", "  ".repeat(entry.depth()), path.display())
+                // Required for Windows compatibility
+                let path_str = path.to_str().map(|s| s.replace('\\', "/")).unwrap();
+                format!("{}{}", "  ".repeat(entry.depth()), path_str)
             })
+            .chain(std::iter::once(String::new()))
             .collect::<Vec<_>>()
             .join("\n")
     }
-
     fn file_tree_diff(&self) -> String {
         unified_diff(
             similar::Algorithm::Patience,
-            &self.file_tree.clone().unwrap(),
-            self.current_file_tree().as_ref(),
+            &self.file_tree.clone(),
+            Self::current_file_tree(&self.workspace_dir).as_ref(),
             3,
             Some(("Original file tree", "Updated file tree")),
         )
@@ -142,7 +171,7 @@ impl TestProject {
 
 #[test]
 fn test_json_inline() {
-    let mut test_project = TestProject::new()
+    let test_project = TestFiles::new()
         .add_file(
             "Cargo.toml",
             r#"
@@ -181,7 +210,7 @@ fn test_json_snapshot() {
 "#
             .to_string(),
         )
-        .create();
+        .create_project();
 
     let output = test_project
         .cmd()
@@ -211,7 +240,7 @@ fn test_json_snapshot() {
 
 #[test]
 fn test_yaml_inline() {
-    let mut test_project = TestProject::new()
+    let test_project = TestFiles::new()
         .add_file(
             "Cargo.toml",
             r#"
@@ -250,7 +279,7 @@ fn test_yaml_snapshot() {
 "#
             .to_string(),
         )
-        .create();
+        .create_project();
 
     let output = test_project
         .cmd()
@@ -279,7 +308,7 @@ fn test_yaml_snapshot() {
 
 #[test]
 fn test_utf8_inline() {
-    let mut test_project = TestProject::new()
+    let test_project = TestFiles::new()
         .add_file(
             "Cargo.toml",
             r#"
@@ -326,7 +355,7 @@ fn test_trailing_comma_in_inline_snapshot() {
 "#
             .to_string(),
         )
-        .create();
+        .create_project();
 
     let output = test_project
         .cmd()
@@ -380,7 +409,7 @@ fn test_trailing_comma_in_inline_snapshot() {
 #[ignore]
 #[test]
 fn test_nested_crate() {
-    let mut test_project = TestProject::new()
+    let test_project = TestFiles::new()
         .add_file(
             "Cargo.toml",
             r#"
@@ -441,7 +470,7 @@ fn test_root() {
 "#
             .to_string(),
         )
-        .create();
+        .create_project();
 
     let output = test_project
         .cmd()