diff --git a/ADVANCED_USAGE.md b/ADVANCED_USAGE.md index 54f7e68..9590ff3 100644 --- a/ADVANCED_USAGE.md +++ b/ADVANCED_USAGE.md @@ -1,7 +1,274 @@ -# Packs first mode +# Advanced Usage + +This document covers advanced configuration options and features in `pks`. + +## Packs First Mode Packs first mode can be used if your entire team is using `packs`. Currently, the only thing this does is change the copy in `package_todo.yml` files to reference `pks` instead of `packwerk`. There are two ways to enable this: 1. Rename `packwerk.yml` to `packs.yml` and packs first mode will be automatically enabled. 2. Set `packs_first_mode: true` in your `packwerk.yml` + +--- + +## Gitignore Support + +### Overview + +By default, `pks` automatically respects `.gitignore` files when analyzing your codebase. This means any files or directories listed in your `.gitignore` will be excluded from pack analysis. + +This feature: +- ✅ Reduces noise by excluding generated files, temporary files, and vendor code +- ✅ Improves performance by skipping ignored directories during traversal +- ✅ Works automatically without any configuration +- ✅ Can be disabled if you need behavior identical to `packwerk` + +### What Files Are Respected? + +`pks` checks multiple gitignore sources in this order: + +1. **Local `.gitignore`** - The `.gitignore` file in your repository root +2. **Global gitignore** - Your user-level gitignore file from `git config --global core.excludesFile` +3. **Git exclude file** - The `.git/info/exclude` file in your repository + +All standard gitignore features are supported: +- Pattern matching (e.g., `*.log`, `tmp/`) +- Directory exclusion (e.g., `node_modules/`) +- Negation patterns (e.g., `!important.log`) +- Comments (lines starting with `#`) + +### Configuration + +#### Disabling Gitignore Support + +If you need to disable automatic gitignore support, add this to your `packwerk.yml` or `packs.yml`: + +```yaml +# Disable automatic gitignore support +respect_gitignore: false +``` + +#### When to Disable? + +You might want to disable gitignore support if: +- You have files in `.gitignore` that should still be analyzed by `pks` +- You're migrating from `packwerk` and want identical behavior +- You have custom `exclude:` patterns that you prefer to manage manually +- You need to analyze generated code that's normally gitignored + +#### Default Behavior + +If not specified, `respect_gitignore` defaults to `true` (enabled). + +### Precedence of Ignore Rules + +When determining whether to process a file, `pks` applies rules in this order (highest priority first): + +1. **Include patterns** - Files matching `include:` patterns in configuration +2. **Gitignore patterns** - Files/directories in `.gitignore` (if `respect_gitignore: true`) +3. **Exclude patterns** - Files matching `exclude:` patterns in configuration +4. **Default exclusions** - Hardcoded exclusions (e.g., `{bin,node_modules,script,tmp,vendor}/**/*`) + +This precedence allows you to: +- Override gitignore by explicitly including files via `include:` patterns +- Add additional exclusions beyond what's in `.gitignore` via `exclude:` patterns +- Layer multiple levels of filtering for fine-grained control + +### Example Configuration + +```yaml +# packwerk.yml + +# Enable gitignore support (this is the default) +respect_gitignore: true + +# Include patterns (highest priority - override gitignore) +include: + - "**/*.rb" + - "**/*.rake" + - "**/*.erb" + +# Exclude patterns (lower priority than gitignore) +exclude: + - "{bin,node_modules,script,tmp,vendor}/**/*" + - "test/fixtures/**/*" +``` + +**Example scenario:** + +Given this configuration and a `.gitignore` containing `debug.log`: + +- `app/models/user.rb` → ✅ Analyzed (matches include pattern) +- `tmp/cache/foo.rb` → ❌ Skipped (matches default exclusion) +- `debug.log` → ❌ Skipped (matches gitignore) +- `test/fixtures/data.rb` → ❌ Skipped (matches exclude pattern) + +### How It Works + +When `respect_gitignore: true` (default): +- ✅ Files in `.gitignore` are automatically skipped during directory walking +- ✅ Directories in `.gitignore` are not traversed (improves performance) +- ✅ Global gitignore patterns are applied +- ✅ Gitignore negation patterns (e.g., `!important.log`) are respected +- ✅ `.git/info/exclude` patterns are applied + +When `respect_gitignore: false`: +- ❌ `.gitignore` files are completely ignored +- ✅ Only `include:` and `exclude:` patterns from configuration are used +- ✅ Behavior matches `packwerk` exactly + +### Performance Implications + +Enabling gitignore support typically has **neutral to positive** performance impact: +- ✅ Ignored directories are skipped entirely (faster directory walking) +- ✅ Fewer files need to be processed +- ✅ Pattern matching is highly optimized (uses the same engine as `ripgrep`) +- ✅ Gitignore matcher is built once and reused throughout the walk + +In practice, this means: +- Large ignored directories like `node_modules/`, `tmp/`, or `vendor/` are skipped immediately +- No time wasted parsing or analyzing files that would be ignored anyway +- Memory usage is lower due to fewer files being tracked + +### Troubleshooting + +#### Files Are Unexpectedly Ignored + +If files are being ignored that shouldn't be: + +1. **Check your `.gitignore`** - Run `git check-ignore -v path/to/file.rb` to see which pattern is matching + ```bash + git check-ignore -v app/models/user.rb + # Output: .gitignore:10:*.rb app/models/user.rb + ``` + +2. **Check global gitignore** - See where your global gitignore is configured: + ```bash + # Check if core.excludesFile is set + git config --global core.excludesFile + # Output: /Users/you/.config/git/ignore (or your custom path) + + # View its contents if set + cat $(git config --global core.excludesFile) + ``` + +3. **Disable temporarily** - Set `respect_gitignore: false` to confirm it's a gitignore issue + ```yaml + # packwerk.yml + respect_gitignore: false + ``` + +4. **Use negation patterns** - Add `!path/to/file.rb` to your `.gitignore` to explicitly include it + ```gitignore + # .gitignore + *.log + !important.log # This file should NOT be ignored + ``` + +5. **Override with include patterns** - Add explicit include patterns in your `packwerk.yml`: + ```yaml + include: + - "path/to/important/file.rb" + ``` + +#### Files Are Still Analyzed Despite Being in .gitignore + +If gitignored files are still being analyzed: + +1. **Check configuration** - Ensure `respect_gitignore: true` (or not set, since it defaults to `true`) + ```yaml + # packwerk.yml should have either: + respect_gitignore: true + # or nothing (defaults to true) + ``` + +2. **Check include patterns** - Include patterns override gitignore; files matching `include:` will be processed even if gitignored + ```yaml + include: + - "**/*.rb" # This will include ALL .rb files, even if gitignored + ``` + +3. **Check file location** - Only files within the project root can be affected by gitignore + - Files must be relative to the repository root + - Symlinked files outside the repo may not respect gitignore + +4. **Verify .gitignore syntax** - Ensure your patterns are valid + ```bash + # Test if git itself recognizes the pattern + git status # Should not show the file if properly ignored + git check-ignore path/to/file.rb # Should output the path if ignored + ``` + +#### Debugging Commands + +Useful commands for debugging gitignore behavior: + +```bash +# List all files that pks will analyze +pks list-included-files + +# Check if a specific file would be ignored by git +git check-ignore -v path/to/file.rb + +# See your global gitignore location +git config --global core.excludesFile + +# View your global gitignore contents (if core.excludesFile is set) +cat $(git config --global core.excludesFile) + +# View repository-specific exclusions +cat .git/info/exclude + +# Test gitignore patterns +echo "path/to/file.rb" | git check-ignore --stdin -v +``` + +### Compatibility with Packwerk + +This feature is a **new addition** in `pks` and does not exist in `packwerk`. + +#### Migrating from Packwerk + +If you're migrating from `packwerk` to `pks`: + +1. **Default behavior change**: `pks` will automatically respect `.gitignore` files, while `packwerk` does not +2. **Files that may be affected**: Any files in your `.gitignore` that were previously analyzed by `packwerk` will now be skipped +3. **To get identical behavior**: Set `respect_gitignore: false` in your configuration +4. **Recommended approach**: Try the default behavior first; it usually works better and is faster + +#### Example Migration + +```yaml +# packwerk.yml - for packwerk-identical behavior +respect_gitignore: false + +# Or accept the new default (recommended) +# respect_gitignore: true # This is the default, no need to specify +``` + +--- + +## Custom Error Messages + +The error messages resulting from running `pks check` can be customized with mustache-style interpolation. The available variables are: +- `violation_name` +- `referencing_pack_name` +- `defining_pack_name` +- `constant_name` +- `reference_location` +- `referencing_pack_relative_yml` + +Layer violations also have: +- `defining_layer` +- `referencing_layer` + +Example: +```yaml +# packwerk.yml +checker_overrides: + privacy_error_template: "{{reference_location}}Privacy violation: `{{constant_name}}` is private to `{{defining_pack_name}}`. See https://go/pks-privacy" + dependency_error_template: "{{reference_location}}Dependency violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`. See https://go/pks-dependency" +``` + +See the main [README.md](README.md) for more details. diff --git a/Cargo.toml b/Cargo.toml index c2e8a29..ac0c130 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,7 @@ serde_magnus = "0.7.0" # permits a ruby gem to interface with this library tracing = "0.1.37" # logging tracing-subscriber = { version = "0.3.16", features = ["env-filter"] } # logging glob = "0.3.1" # globbing -globset = "0.4.10" # globbing +globset = "=0.4.16" # globbing - pinned to avoid edition2024 requirement in 0.4.18 lib-ruby-parser = "4.0.6" # ruby parser md5 = "0.7.0" # md5 hashing to take and compare md5 digests of file contents to ensure cache validity line-col = "0.2.1" # for creating source maps of violations @@ -48,6 +48,7 @@ ruby_inflector = '0.0.8' # for inflecting strings, e.g. turning `has_many :compa petgraph = "0.6.3" # for running graph algorithms (e.g. does the dependency graph contain a cycle?) fnmatch-regex2 = "0.3.0" strip-ansi-escapes = "0.2.0" +ignore = "=0.4.23" # for gitignore pattern matching - pinned to avoid edition2024 requirement in 0.4.25 [dev-dependencies] assert_cmd = "2.0.10" # testing CLI @@ -55,3 +56,4 @@ rusty-hook = "^0.11.2" # git hooks predicates = "3.0.2" # kind of like rspec assertions pretty_assertions = "1.3.0" # Shows a more readable diff when comparing objects serial_test = "3.1.1" # Run specific tests in serial +tempfile = "3.8.0" # for creating temporary directories in tests diff --git a/README.md b/README.md index f759ae4..1394225 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,11 @@ Currently, it ships the following checkers to help improve the boundaries betwee See [Checkers](CHECKERS.md) for detailed descriptions. +## Automatic Gitignore Support +- Automatically respects `.gitignore` files (both local and global) +- Improves performance by skipping ignored directories during traversal +- Can be disabled via `respect_gitignore: false` configuration if needed + # Fork This repo was forked directly from https://github.com/alexevanczuk/packs @@ -90,6 +95,18 @@ There are still some known behavioral differences between `pks` and `packwerk`. - `package_paths` must not end in a slash, e.g. `pks/*/` is not supported, but `pks/*` is. - A `**` in `package_paths` is supported, but is not a substitute for a single `*`, e.g. `pks/**` is supported and will match `pks/*/*/package.yml`, but will not match `pks/*/package.yml`. `pks/*` must be used to match that. +## Gitignore Support (pks-specific feature) +`pks` automatically respects `.gitignore` files when analyzing your codebase. This means: +- Files listed in `.gitignore` are automatically excluded from analysis +- Respects global gitignore from `core.excludesFile` git config +- Respects `.git/info/exclude` +- Improves performance by skipping ignored directories entirely +- Can be disabled by setting `respect_gitignore: false` in `packwerk.yml` + +This feature is **enabled by default**. If you need behavior identical to `packwerk`, set `respect_gitignore: false`. + +For detailed configuration, precedence rules, and troubleshooting, see [ADVANCED_USAGE.md](ADVANCED_USAGE.md). + ## Default Namespaces `pks` supports Zeitwerk default namespaces. However, since it doesn't have access to the Rails runtime, you need to explicitly specify the namespaces in `packwerk.yml`. diff --git a/src/packs.rs b/src/packs.rs index b10f66d..4bc54fe 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -17,7 +17,7 @@ pub(crate) mod monkey_patch_detection; pub(crate) mod pack; pub(crate) mod parsing; pub(crate) mod raw_configuration; -pub(crate) mod walk_directory; +pub mod walk_directory; mod constant_dependencies; mod file_utils; diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 1937503..8d4118b 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -53,6 +53,17 @@ impl Cache for PerFileCache { let cache_data = serde_json::to_string(&cache_entry) .context("Failed to serialize references")?; + + // Ensure parent directory exists + if let Some(parent) = empty_cache_entry.cache_file_path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + anyhow::Error::new(e).context(format!( + "Failed to create cache directory {:?}", + parent + )) + })?; + } + let mut file = File::create(&empty_cache_entry.cache_file_path) .map_err(|e| { anyhow::Error::new(e).context(format!( diff --git a/src/packs/raw_configuration.rs b/src/packs/raw_configuration.rs index 2bcc14a..3f94f66 100644 --- a/src/packs/raw_configuration.rs +++ b/src/packs/raw_configuration.rs @@ -78,6 +78,10 @@ pub(crate) struct RawConfiguration { #[serde(default)] pub checker_overrides: Option, + + // Whether to automatically respect .gitignore files + #[serde(default = "default_respect_gitignore")] + pub respect_gitignore: bool, } /// Customize violation names and error messages #[derive(Debug, Deserialize, Serialize)] @@ -173,6 +177,10 @@ fn default_cache_directory() -> String { String::from("tmp/cache/packwerk") } +fn default_respect_gitignore() -> bool { + true +} + fn string_or_vec<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, diff --git a/src/packs/walk_directory.rs b/src/packs/walk_directory.rs index 96d36a5..6688db7 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -1,7 +1,8 @@ +use ignore::gitignore::{Gitignore, GitignoreBuilder}; use jwalk::WalkDirGeneric; use std::{ collections::{HashMap, HashSet}, - path::PathBuf, + path::{Path, PathBuf}, sync::Arc, }; use tracing::debug; @@ -27,6 +28,117 @@ impl jwalk::ClientState for ProcessReadDirState { type DirEntryState = ProcessReadDirState; } +/// Expands tilde (~) in paths to the user's home directory. +/// +/// This function is specifically needed to handle `core.excludesFile` paths from git config, +/// which commonly use tilde notation (e.g., `~/.gitignore_global`). Git returns the literal +/// tilde string, but Rust's PathBuf doesn't automatically expand it. +/// +/// # Arguments +/// * `path` - A path string that may contain a leading tilde +/// +/// # Returns +/// A PathBuf with the tilde expanded to the home directory, or the original path if +/// no tilde is present or HOME is not set. +/// +/// # Example +/// ``` +/// // Git config might return "~/.gitignore_global" +/// // This expands it to "/Users/username/.gitignore_global" +/// ``` +pub fn expand_tilde(path: &str) -> PathBuf { + if let Some(stripped) = path.strip_prefix("~/") { + if let Some(home) = std::env::var_os("HOME") { + return PathBuf::from(home).join(stripped); + } + } + PathBuf::from(path) +} + +/// Attempts to locate the global gitignore file from git config. +/// +/// This function reads the `core.excludesFile` setting from git config, +/// which is the standard way to configure a global gitignore file. +/// +/// # Returns +/// `Some(PathBuf)` if `core.excludesFile` is configured and the file exists, +/// `None` otherwise. +pub fn get_global_gitignore() -> Option { + // Read core.excludesFile from git config + if let Ok(output) = std::process::Command::new("git") + .args(["config", "--global", "core.excludesFile"]) + .output() + { + if output.status.success() { + let path_str = + String::from_utf8_lossy(&output.stdout).trim().to_string(); + if !path_str.is_empty() { + // Git config returns literal tilde (e.g., "~/.gitignore_global") + // so we need to expand it to the actual home directory path + let expanded = expand_tilde(&path_str); + if expanded.exists() { + return Some(expanded); + } + } + } + } + + None +} + +/// Builds a gitignore matcher that respects local and global gitignore files. +/// +/// This function constructs a `Gitignore` matcher by combining: +/// - Local `.gitignore` file in the repository root +/// - Global gitignore file (from `core.excludesFile` git config) +/// - `.git/info/exclude` file in the repository +/// +/// # Arguments +/// * `absolute_root` - The absolute path to the repository root +/// +/// # Returns +/// A `Gitignore` matcher that can be used to check if paths should be ignored, +/// or an error if the matcher cannot be built. +pub fn build_gitignore_matcher( + absolute_root: &Path, +) -> anyhow::Result { + let mut builder = GitignoreBuilder::new(absolute_root); + + // Add local .gitignore + let local_gitignore = absolute_root.join(".gitignore"); + if local_gitignore.exists() { + if let Some(err) = builder.add(&local_gitignore) { + return Err(anyhow::anyhow!( + "Failed to add local .gitignore: {}", + err + )); + } + } + + // Add global gitignore + if let Some(global_gitignore) = get_global_gitignore() { + if let Some(err) = builder.add(&global_gitignore) { + return Err(anyhow::anyhow!( + "Failed to add global gitignore: {}", + err + )); + } + } + + // Add .git/info/exclude + let git_exclude = absolute_root.join(".git/info/exclude"); + if git_exclude.exists() { + if let Some(err) = builder.add(&git_exclude) { + return Err(anyhow::anyhow!( + "Failed to add .git/info/exclude: {}", + err + )); + } + } + + Ok(builder.build()?) +} + // We use jwalk to walk directories in parallel and compare them to the `include` and `exclude` patterns // specified in the `RawConfiguration` // https://docs.rs/jwalk/0.8.1/jwalk/struct.WalkDirGeneric.html#method.process_read_dir @@ -72,6 +184,22 @@ pub(crate) fn walk_directory( let excludes_set = build_glob_set(&raw.exclude); let package_paths_set = build_glob_set(&raw.package_paths); + // Build gitignore matcher if enabled + let gitignore_matcher = if raw.respect_gitignore { + match build_gitignore_matcher(&absolute_root) { + Ok(matcher) => Some(Arc::new(matcher)), + Err(e) => { + debug!("Failed to build gitignore matcher: {}. Continuing without gitignore support.", e); + None + } + } + } else { + None + }; + + let gitignore_ref = Arc::new(gitignore_matcher); + let gitignore_ref_for_loop = gitignore_ref.clone(); + // TODO: Pull directory walker into separate module. Allow it to be called with implementations of a trait // so separate concerns can each be in their own place. // @@ -97,6 +225,7 @@ pub(crate) fn walk_directory( // (with an increase to the reference count). let cloned_excluded_dirs = excluded_dirs_ref.clone(); let cloned_absolute_root = absolute_root_ref.clone(); + let cloned_gitignore = gitignore_ref.clone(); let package_yml = absolute_dirname.join("package.yml"); // Even if the parent has set this on children, the existence of a new @@ -116,6 +245,21 @@ pub(crate) fn walk_directory( let relative_path = child_absolute_dirname .strip_prefix(cloned_absolute_root.as_ref()) .unwrap(); + + // Check gitignore for directories only (optimization: prune ignored directory trees early) + // Files are checked separately in the main loop below (see line ~304) + if let Some(gitignore) = cloned_gitignore.as_ref() { + let is_dir = child_dir_entry.file_type.is_dir(); + if is_dir + && gitignore + .matched(relative_path, true) + .is_ignore() + { + child_dir_entry.read_children_path = None; + } + } + + // Then check explicit exclusions if cloned_excluded_dirs.as_ref().is_match(relative_path) { child_dir_entry.read_children_path = None; @@ -158,6 +302,13 @@ pub(crate) fn walk_directory( .unwrap() .to_owned(); + // Skip gitignored files (if gitignore support is enabled) + if let Some(gitignore) = gitignore_ref_for_loop.as_ref() { + if gitignore.matched(&relative_path, false).is_ignore() { + continue; + } + } + let current_package_yml = &unwrapped_entry.client_state.current_package_yml; @@ -207,6 +358,9 @@ mod tests { use crate::packs::{ raw_configuration::RawConfiguration, walk_directory::walk_directory, }; + use serial_test::serial; + + use super::{build_gitignore_matcher, expand_tilde, get_global_gitignore}; #[test] fn test_walk_directory() -> anyhow::Result<()> { @@ -235,4 +389,168 @@ mod tests { Ok(()) } + + #[test] + #[serial] + fn test_expand_tilde_with_tilde() { + // Save and restore HOME to avoid test interaction + let original_home = std::env::var_os("HOME"); + + // Set HOME for this test + std::env::set_var("HOME", "/test/home"); + + let expanded = expand_tilde("~/some/path"); + assert_eq!(expanded, PathBuf::from("/test/home/some/path")); + + // Restore original HOME + if let Some(home) = original_home { + std::env::set_var("HOME", home); + } else { + std::env::remove_var("HOME"); + } + } + + #[test] + fn test_expand_tilde_without_tilde() { + let expanded = expand_tilde("/absolute/path"); + assert_eq!(expanded, PathBuf::from("/absolute/path")); + } + + #[test] + fn test_expand_tilde_relative_path() { + let expanded = expand_tilde("relative/path"); + assert_eq!(expanded, PathBuf::from("relative/path")); + } + + #[test] + fn test_get_global_gitignore_returns_option() { + // This test just ensures the function runs without panicking + // and returns the correct type. We can't guarantee what it returns + // since it depends on the environment. + let result = get_global_gitignore(); + + // If it returns Some, the path should exist + if let Some(path) = result { + assert!(path.exists()); + } + } + + #[test] + fn test_build_gitignore_matcher_with_simple_app() -> anyhow::Result<()> { + let absolute_path = PathBuf::from("tests/fixtures/simple_app") + .canonicalize() + .expect("Could not canonicalize path"); + + // Should succeed even if no .gitignore exists + let result = build_gitignore_matcher(&absolute_path); + assert!(result.is_ok()); + + Ok(()) + } + + #[test] + fn test_build_gitignore_matcher_returns_usable_matcher( + ) -> anyhow::Result<()> { + let absolute_path = PathBuf::from("tests/fixtures/simple_app") + .canonicalize() + .expect("Could not canonicalize path"); + + let matcher = build_gitignore_matcher(&absolute_path)?; + + // The matcher should be usable (this just tests it doesn't panic) + let test_path = PathBuf::from("test.rb"); + let _result = matcher.matched(&test_path, false); + + Ok(()) + } + + #[test] + #[serial] + fn test_expand_tilde_without_home_env() { + // Save original HOME value + let original_home = std::env::var_os("HOME"); + + // Temporarily unset HOME + std::env::remove_var("HOME"); + + let result = expand_tilde("~/test/path"); + + // When HOME is not set, should return path as-is + assert_eq!(result, PathBuf::from("~/test/path")); + + // Restore HOME if it was set + if let Some(home) = original_home { + std::env::set_var("HOME", home); + } + } + + #[test] + fn test_build_gitignore_matcher_with_malformed_gitignore( + ) -> anyhow::Result<()> { + use std::fs; + use std::io::Write; + + // Create a temporary directory for this test + let temp_dir = + std::env::temp_dir().join("pks_test_malformed_gitignore"); + fs::create_dir_all(&temp_dir)?; + + // Create a gitignore with potentially problematic content + // Note: The ignore crate is quite permissive, so most "malformed" + // content is actually handled gracefully + let gitignore_path = temp_dir.join(".gitignore"); + let mut file = fs::File::create(&gitignore_path)?; + + // Write some edge case patterns + writeln!(file, "# This is a comment")?; + writeln!(file)?; // Blank line + writeln!(file, "*.log")?; + writeln!(file, " ")?; // Whitespace-only line + writeln!(file, "temp/")?; + + // The matcher should build successfully even with edge cases + let result = build_gitignore_matcher(&temp_dir); + assert!( + result.is_ok(), + "Should handle gitignore with comments, blank lines, and whitespace" + ); + + // Clean up + fs::remove_dir_all(&temp_dir)?; + + Ok(()) + } + + #[test] + fn test_build_gitignore_matcher_with_git_info_exclude() -> anyhow::Result<()> + { + use std::fs; + use std::io::Write; + + // Create a temporary directory structure + let temp_dir = std::env::temp_dir().join("pks_test_git_exclude"); + let git_info_dir = temp_dir.join(".git/info"); + fs::create_dir_all(&git_info_dir)?; + + // Create .git/info/exclude file + let exclude_path = git_info_dir.join("exclude"); + let mut file = fs::File::create(&exclude_path)?; + writeln!(file, "excluded_by_git.txt")?; + + // Build matcher + let matcher = build_gitignore_matcher(&temp_dir)?; + + // Test that the pattern from .git/info/exclude is respected + let excluded_file = PathBuf::from("excluded_by_git.txt"); + let result = matcher.matched(&excluded_file, false); + assert!( + result.is_ignore(), + "Should respect patterns from .git/info/exclude" + ); + + // Clean up + fs::remove_dir_all(&temp_dir)?; + + Ok(()) + } } diff --git a/tests/add_constant_dependencies.rs b/tests/add_constant_dependencies.rs index cb718fb..cb9bbed 100644 --- a/tests/add_constant_dependencies.rs +++ b/tests/add_constant_dependencies.rs @@ -9,7 +9,7 @@ mod common; #[test] #[serial] fn test_add_constant_dependencies() -> anyhow::Result<()> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_missing_dependencies") .arg("update-dependencies-for-constant") @@ -43,7 +43,7 @@ fn test_add_constant_dependencies() -> anyhow::Result<()> { #[test] #[serial] fn test_add_constant_dependencies_no_dependencies() -> anyhow::Result<()> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_missing_dependencies") .arg("update-dependencies-for-constant") diff --git a/tests/add_dependency_test.rs b/tests/add_dependency_test.rs index cd37824..fb0ca65 100644 --- a/tests/add_dependency_test.rs +++ b/tests/add_dependency_test.rs @@ -9,7 +9,7 @@ mod common; #[test] #[serial] fn test_add_dependency() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_missing_dependency") .arg("add-dependency") @@ -41,7 +41,7 @@ fn test_add_dependency() -> Result<(), Box> { #[test] #[serial] fn test_add_dependency_creating_cycle() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_missing_dependency") .arg("add-dependency") @@ -80,7 +80,7 @@ fn test_add_dependency_creating_cycle() -> Result<(), Box> { #[test] fn test_add_dependency_unnecessarily() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_missing_dependency") .arg("add-dependency") diff --git a/tests/check_test.rs b/tests/check_test.rs index ae6b135..7d6deed 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -11,7 +11,7 @@ pub fn stripped_output(output: Vec) -> String { #[test] fn test_check_with_privacy_dependency_error_template_overrides( ) -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/privacy_violation_overrides") .arg("--debug") @@ -33,7 +33,7 @@ fn test_check_with_privacy_dependency_error_template_overrides( } #[test] fn test_check() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -56,7 +56,7 @@ fn test_check() -> Result<(), Box> { #[test] fn test_check_enforce_privacy_disabled() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -79,7 +79,7 @@ fn test_check_enforce_privacy_disabled() -> Result<(), Box> { #[test] fn test_check_enforce_dependency_disabled() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -102,7 +102,7 @@ fn test_check_enforce_dependency_disabled() -> Result<(), Box> { #[test] fn test_check_with_single_file() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -127,7 +127,7 @@ fn test_check_with_single_file() -> Result<(), Box> { #[test] fn test_check_with_single_file_experimental_parser( ) -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -152,7 +152,7 @@ fn test_check_with_single_file_experimental_parser( #[test] fn test_check_with_package_todo_file() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_package_todo") .arg("--debug") @@ -168,7 +168,7 @@ fn test_check_with_package_todo_file() -> Result<(), Box> { #[test] fn test_check_with_package_todo_file_csv() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_package_todo") .arg("--debug") @@ -188,7 +188,7 @@ fn test_check_with_package_todo_file_csv() -> Result<(), Box> { #[test] fn test_check_with_package_todo_file_ignoring_recorded_violations( ) -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_package_todo") .arg("--debug") @@ -212,8 +212,7 @@ fn test_check_with_package_todo_file_ignoring_recorded_violations( #[test] fn test_check_with_experimental_parser() -> Result<(), Box> { - let output = Command::cargo_bin("pks") - .unwrap() + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--experimental-parser") @@ -237,8 +236,7 @@ fn test_check_with_experimental_parser() -> Result<(), Box> { #[test] fn test_check_with_stale_violations() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_stale_violations") .arg("check") @@ -255,8 +253,7 @@ fn test_check_with_stale_violations() -> Result<(), Box> { #[test] fn test_check_with_stale_violations_when_file_no_longer_exists( ) -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_stale_violations_no_file") .arg("check") @@ -272,8 +269,7 @@ fn test_check_with_stale_violations_when_file_no_longer_exists( #[test] fn test_check_with_relationship_violations() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_rails_relationships") .arg("check") @@ -289,8 +285,7 @@ fn test_check_with_relationship_violations() -> Result<(), Box> { #[test] fn test_check_without_stale_violations() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_package_todo") .arg("check") @@ -309,8 +304,7 @@ fn test_check_without_stale_violations() -> Result<(), Box> { #[test] fn test_check_with_strict_mode() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -329,8 +323,7 @@ fn test_check_with_strict_mode() -> Result<(), Box> { #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -354,7 +347,7 @@ fn test_check_contents() -> Result<(), Box> { let foo_rb_contents = fs::read_to_string(format!("{}/{}", project_root, relative_path))?; - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg(project_root) .arg("--debug") @@ -385,7 +378,7 @@ fn test_check_contents_ignoring_recorded_violations( let foo_rb_contents = fs::read_to_string(format!("{}/{}", project_root, relative_path))?; - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg(project_root) .arg("--debug") diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 640259e..723cb61 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -4,7 +4,7 @@ use std::{error::Error, fs, process::Command}; mod common; fn assert_check_unused_dependencies(cmd: &str) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_dependency_cycles") .arg("--debug") @@ -51,7 +51,7 @@ fn assert_auto_correct_unused_dependencies( let foo_package_yml = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(foo_package_yml, expected_before_autocorrect); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_unnecessary_dependencies") .arg("--debug") @@ -94,7 +94,7 @@ fn test_auto_correct_unnecessary_dependencies() -> Result<(), Box> { #[test] fn test_check_unnecessary_dependencies_no_issue() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") diff --git a/tests/corrupt_todo_test.rs b/tests/corrupt_todo_test.rs index ec94340..3a767c8 100644 --- a/tests/corrupt_todo_test.rs +++ b/tests/corrupt_todo_test.rs @@ -4,7 +4,7 @@ mod common; #[test] fn test_check_with_corrupt_todo() -> anyhow::Result<()> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_corrupt_todo") .arg("--debug") diff --git a/tests/create_test.rs b/tests/create_test.rs index 42c2d94..92f26c4 100644 --- a/tests/create_test.rs +++ b/tests/create_test.rs @@ -9,7 +9,7 @@ mod common; fn test_create() -> Result<(), Box> { common::delete_foobar(); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("create") @@ -76,7 +76,7 @@ fn test_create_with_readme_template_default_path() -> Result<(), Box> "This is a test custom README template", )?; - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_packs_first_app") .arg("create") @@ -106,7 +106,7 @@ fn test_create_with_readme_template_custom_path() -> Result<(), Box> { common::delete_foobar_app_with_custom_readme(); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_custom_readme") .arg("create") @@ -131,7 +131,7 @@ fn test_create_with_readme_template_custom_path() -> Result<(), Box> #[test] fn test_create_already_exists() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_packs_first_app") .arg("create") diff --git a/tests/delete_cache_test.rs b/tests/delete_cache_test.rs index ec5d658..8256682 100644 --- a/tests/delete_cache_test.rs +++ b/tests/delete_cache_test.rs @@ -34,7 +34,7 @@ fn test_delete_cache() -> Result<(), Box> { assert!(!is_tmp_cache_packwerk_empty().unwrap()); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--debug") .arg("--project-root") .arg("tests/fixtures/simple_app") diff --git a/tests/dependencies_test.rs b/tests/dependencies_test.rs index 648465f..e146436 100644 --- a/tests/dependencies_test.rs +++ b/tests/dependencies_test.rs @@ -7,7 +7,7 @@ mod common; #[test] fn test_list_pack_dependencies_with_explicit_dependencies( ) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -25,7 +25,7 @@ fn test_list_pack_dependencies_with_explicit_dependencies( #[test] fn list_pack_dependencies_with_implicit_dependencies( ) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_package_todo") .arg("--debug") diff --git a/tests/enforcement_globs.rs b/tests/enforcement_globs.rs index b1b3706..7f7c633 100644 --- a/tests/enforcement_globs.rs +++ b/tests/enforcement_globs.rs @@ -6,7 +6,7 @@ mod common; #[test] fn test_check() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app_with_enforcement_globs") .arg("--debug") diff --git a/tests/expose_monkey_patches_test.rs b/tests/expose_monkey_patches_test.rs index f01545d..792cb54 100644 --- a/tests/expose_monkey_patches_test.rs +++ b/tests/expose_monkey_patches_test.rs @@ -10,7 +10,7 @@ fn test_expose_monkey_patches() -> Result<(), Box> { let expected_message_portion = String::from( "The following is a list of constants that are redefined by your app.", ); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_monkey_patches") .arg("--experimental-parser") diff --git a/tests/fixtures/app_with_gitignore/.gitignore b/tests/fixtures/app_with_gitignore/.gitignore new file mode 100644 index 0000000..c4b3fc8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/.gitignore @@ -0,0 +1,8 @@ +# Test gitignore file for pks +*.log +!important.log +*.tmp +temp/ +ignored_folder/ +ignored_file.rb + diff --git a/tests/fixtures/app_with_gitignore/package.yml b/tests/fixtures/app_with_gitignore/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore/packs/bar/app/services/bar.rb b/tests/fixtures/app_with_gitignore/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5d60a96 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/bar/app/services/bar.rb @@ -0,0 +1,8 @@ +module Bar + class Service + def call + puts "Bar service" + end + end +end + diff --git a/tests/fixtures/app_with_gitignore/packs/bar/package.yml b/tests/fixtures/app_with_gitignore/packs/bar/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/bar/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore/packs/foo/app/services/foo.rb b/tests/fixtures/app_with_gitignore/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..2eb9711 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/foo/app/services/foo.rb @@ -0,0 +1,9 @@ +module Foo + class Service + def call + # This should cause a dependency violation that SHOULD be caught + Bar::Service.new.call + end + end +end + diff --git a/tests/fixtures/app_with_gitignore/packs/foo/important.log b/tests/fixtures/app_with_gitignore/packs/foo/important.log new file mode 100644 index 0000000..179a122 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/foo/important.log @@ -0,0 +1,2 @@ +This file should NOT be ignored due to negation pattern !important.log + diff --git a/tests/fixtures/app_with_gitignore/packs/foo/included_file.rb b/tests/fixtures/app_with_gitignore/packs/foo/included_file.rb new file mode 100644 index 0000000..d1404c8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/foo/included_file.rb @@ -0,0 +1,4 @@ +# This file should be included +class IncludedClass +end + diff --git a/tests/fixtures/app_with_gitignore/packs/foo/package.yml b/tests/fixtures/app_with_gitignore/packs/foo/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packs/foo/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore/packwerk.yml b/tests/fixtures/app_with_gitignore/packwerk.yml new file mode 100644 index 0000000..8f06a37 --- /dev/null +++ b/tests/fixtures/app_with_gitignore/packwerk.yml @@ -0,0 +1,7 @@ +include: + - '**/*.rb' +exclude: + - '{tmp,node_modules,vendor}/**/*' +package_paths: + - 'packs/*' + diff --git a/tests/fixtures/app_with_gitignore_disabled/.gitignore b/tests/fixtures/app_with_gitignore_disabled/.gitignore new file mode 100644 index 0000000..49bc496 --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/.gitignore @@ -0,0 +1,6 @@ +*.log +*.tmp +temp/ +ignored_folder/ +ignored_file.rb + diff --git a/tests/fixtures/app_with_gitignore_disabled/ignored_folder/violating.rb b/tests/fixtures/app_with_gitignore_disabled/ignored_folder/violating.rb new file mode 100644 index 0000000..df1d231 --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/ignored_folder/violating.rb @@ -0,0 +1,3 @@ +# This file is in .gitignore but should be processed when respect_gitignore: false +::Bar::Service + diff --git a/tests/fixtures/app_with_gitignore_disabled/package.yml b/tests/fixtures/app_with_gitignore_disabled/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore_disabled/packs/bar/app/services/bar.rb b/tests/fixtures/app_with_gitignore_disabled/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..f9beabc --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/packs/bar/app/services/bar.rb @@ -0,0 +1,5 @@ +module Bar + class Service + end +end + diff --git a/tests/fixtures/app_with_gitignore_disabled/packs/bar/package.yml b/tests/fixtures/app_with_gitignore_disabled/packs/bar/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/packs/bar/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore_disabled/packs/foo/package.yml b/tests/fixtures/app_with_gitignore_disabled/packs/foo/package.yml new file mode 100644 index 0000000..2a935d8 --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/packs/foo/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +enforce_privacy: true + diff --git a/tests/fixtures/app_with_gitignore_disabled/packwerk.yml b/tests/fixtures/app_with_gitignore_disabled/packwerk.yml new file mode 100644 index 0000000..9ff70cf --- /dev/null +++ b/tests/fixtures/app_with_gitignore_disabled/packwerk.yml @@ -0,0 +1,8 @@ +respect_gitignore: false +inflections_file: config/inflections.yml +package_paths: + - "." + - "packs/*" +exclude: + - "{bin,node_modules,script,tmp,vendor}/**/*" + diff --git a/tests/folder_privacy_test.rs b/tests/folder_privacy_test.rs index 3f72f32..782ca63 100644 --- a/tests/folder_privacy_test.rs +++ b/tests/folder_privacy_test.rs @@ -5,7 +5,7 @@ mod common; #[test] fn test_check() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/folder_privacy_violations") .arg("--debug") @@ -20,7 +20,7 @@ fn test_check() -> Result<(), Box> { #[test] fn test_check_with_error_template_overrides() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/folder_privacy_violations_with_overrides") .arg("--debug") @@ -35,7 +35,7 @@ fn test_check_with_error_template_overrides() -> Result<(), Box> { #[test] fn test_check_enforce_folder_privacy_disabled() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/folder_privacy_violations") .arg("--debug") @@ -51,7 +51,7 @@ fn test_check_enforce_folder_privacy_disabled() -> Result<(), Box> { #[test] fn test_invisible_pack_violation_with_deprecated_enforce_folder_visibility( ) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/folder_visibility_violations") .arg("--debug") diff --git a/tests/gitignore_test.rs b/tests/gitignore_test.rs new file mode 100644 index 0000000..4a2cb7b --- /dev/null +++ b/tests/gitignore_test.rs @@ -0,0 +1,431 @@ +use assert_cmd::prelude::*; +use packs::packs::walk_directory::build_gitignore_matcher; +use predicates::prelude::*; +use serial_test::serial; +use std::fs; +use std::path::PathBuf; +use std::{error::Error, process::Command}; +use tempfile::TempDir; + +mod common; + +/// Test that gitignored files are completely excluded from violation checking. +/// The fixture has a violation in ignored_folder/violating.rb which should NOT be reported. +#[test] +fn test_check_ignores_violations_in_gitignored_files( +) -> Result<(), Box> { + // The fixture has: + // - packs/foo/app/services/foo.rb with violation (NOT ignored) + // - ignored_folder/violating.rb with violation (IS ignored) + + let result = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/app_with_gitignore") + .arg("check") + .assert() + .failure(); // Still fails due to violation in foo.rb + + let output = result.get_output(); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + // Violations are printed to stdout + // Should report violation in non-ignored file + assert!( + stdout.contains("packs/foo/app/services/foo.rb") + || stdout.contains("foo.rb"), + "Should detect violation in non-ignored file.\nstdout: {}\nstderr: {}", + stdout, + stderr + ); + + // Should NOT report violation in ignored file + assert!( + !stdout.contains("ignored_folder") && !stdout.contains("violating.rb"), + "Should NOT detect violations in gitignored files.\nstdout: {}\nstderr: {}", stdout, stderr + ); + + common::teardown(); + Ok(()) +} + +/// Test that list-included-files respects gitignore patterns. +#[test] +fn test_list_included_files_excludes_gitignored() -> Result<(), Box> +{ + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/app_with_gitignore") + .arg("list-included-files") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8_lossy(&output); + + // Should include non-ignored Ruby files + assert!( + stdout.contains("included_file.rb"), + "Should include non-ignored files" + ); + assert!( + stdout.contains("foo.rb") || stdout.contains("bar.rb"), + "Should include package files" + ); + + // Should NOT include gitignored files + assert!( + !stdout.contains("ignored_file.rb"), + "Should NOT include files matched by gitignore patterns" + ); + assert!( + !stdout.contains("debug.log"), + "Should NOT include *.log files" + ); + assert!( + !stdout.contains("ignored_folder"), + "Should NOT include files in ignored directories" + ); + + common::teardown(); + Ok(()) +} + +/// Test that the application works correctly even without a .gitignore file. +#[test] +fn test_check_works_without_gitignore() -> Result<(), Box> { + // simple_app doesn't have a .gitignore file + // This should still work (and report violations as usual) + + Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("--debug") + .arg("check") + .assert() + .failure() // Has violations + .stdout(predicate::str::contains("violation(s) detected")); + + common::teardown(); + Ok(()) +} + +/// Test that basic gitignore functionality works at the library level. +/// This is a sanity check that our helper functions work correctly. +#[test] +fn test_gitignore_matcher_functions() -> Result<(), Box> { + let absolute_path = PathBuf::from("tests/fixtures/app_with_gitignore") + .canonicalize() + .expect("Could not canonicalize path"); + + let matcher = build_gitignore_matcher(&absolute_path)?; + + // Test that patterns in .gitignore are matched correctly + + // *.log should be ignored + let log_file = PathBuf::from("packs/foo/debug.log"); + let result = matcher.matched(&log_file, false); + assert!(result.is_ignore(), "*.log files should be ignored"); + + // ignored_file.rb should be ignored (explicitly listed) + let ignored_rb = PathBuf::from("packs/foo/ignored_file.rb"); + let result = matcher.matched(&ignored_rb, false); + assert!(result.is_ignore(), "ignored_file.rb should be ignored"); + + // ignored_folder/ directory should be ignored + let ignored_folder = PathBuf::from("ignored_folder"); + let result = matcher.matched(&ignored_folder, true); + assert!( + result.is_ignore(), + "ignored_folder/ directory should be ignored" + ); + + // included_file.rb should NOT be ignored + let included_rb = PathBuf::from("packs/foo/included_file.rb"); + let result = matcher.matched(&included_rb, false); + assert!( + !result.is_ignore(), + "included_file.rb should not be ignored" + ); + + Ok(()) +} + +/// Test that the matcher can be built even without a .gitignore file. +#[test] +fn test_gitignore_matcher_without_gitignore() -> Result<(), Box> { + use packs::packs::walk_directory::build_gitignore_matcher; + use std::path::PathBuf; + + // Use simple_app which doesn't have a .gitignore + let absolute_path = PathBuf::from("tests/fixtures/simple_app") + .canonicalize() + .expect("Could not canonicalize path"); + + // Should succeed even without .gitignore + let result = build_gitignore_matcher(&absolute_path); + assert!( + result.is_ok(), + "Should build matcher even without .gitignore" + ); + + let matcher = result?; + + // Without a .gitignore, regular files should not be ignored + let test_file = PathBuf::from("test.rb"); + let result = matcher.matched(&test_file, false); + assert!( + !result.is_ignore(), + "Regular files should not be ignored without .gitignore" + ); + + Ok(()) +} + +/// CRITICAL: Test that respect_gitignore: false configuration disables gitignore support. +#[test] +fn test_respect_gitignore_can_be_disabled() -> Result<(), Box> { + // The fixture has: + // - .gitignore with ignored_folder/ pattern + // - respect_gitignore: false in packwerk.yml + // - ignored_folder/violating.rb with a violation + // + // With respect_gitignore: false, the violation SHOULD be detected. + + let result = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/app_with_gitignore_disabled") + .arg("check") + .assert() + .failure(); // Should fail due to violation in ignored_folder/ + + let output = result.get_output(); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + // With respect_gitignore: false, should detect violation in "ignored" folder + assert!( + stdout.contains("ignored_folder") || stdout.contains("violating.rb"), + "Should detect violations in gitignored files when respect_gitignore: false.\nstdout: {}\nstderr: {}", + stdout, stderr + ); + + common::teardown(); + Ok(()) +} + +/// Test gitignore negation patterns (!pattern). +/// Common use case: ignore all *.log except important.log +#[test] +fn test_gitignore_negation_patterns() -> Result<(), Box> { + use packs::packs::walk_directory::build_gitignore_matcher; + use std::path::PathBuf; + + let absolute_path = PathBuf::from("tests/fixtures/app_with_gitignore") + .canonicalize() + .expect("Could not canonicalize path"); + + let matcher = build_gitignore_matcher(&absolute_path)?; + + // .gitignore has: + // *.log + // !important.log + + // regular.log should be ignored by *.log + let regular_log = PathBuf::from("packs/foo/regular.log"); + let result = matcher.matched(®ular_log, false); + assert!( + result.is_ignore(), + "regular.log should be ignored by *.log pattern" + ); + + // important.log should NOT be ignored due to !important.log negation + let important_log = PathBuf::from("packs/foo/important.log"); + let result = matcher.matched(&important_log, false); + assert!( + !result.is_ignore(), + "important.log should NOT be ignored due to negation pattern" + ); + + Ok(()) +} + +/// Test that list-included-files respects negation patterns. +/// Note: We test this at the library level since .log files aren't Ruby files +/// and won't appear in list-included-files regardless of gitignore. +#[test] +fn test_list_included_files_respects_negation() -> Result<(), Box> { + // This is already tested by test_gitignore_negation_patterns at the library level. + // At the CLI level, .log files aren't included in list-included-files anyway + // since they don't match the Ruby file patterns. + + // Just verify the basic behavior still works + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/app_with_gitignore") + .arg("list-included-files") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8_lossy(&output); + + // Should still include Ruby files + assert!( + stdout.contains("included_file.rb"), + "Should include non-ignored Ruby files" + ); + + common::teardown(); + Ok(()) +} + +/// Test that global gitignore works end-to-end using core.excludesFile. +/// This requires temporarily setting up git config. +#[test] +#[serial] +fn test_respects_global_gitignore() -> Result<(), Box> { + use std::env; + use std::fs; + use std::path::PathBuf; + use std::process::Command as StdCommand; + + // Create a temporary global gitignore + let temp_dir = env::temp_dir(); + let global_gitignore = temp_dir.join("test_global_gitignore"); + + // Write a pattern that will affect our test + fs::write(&global_gitignore, "# Global test\n*.global_ignore\n")?; + + // Create a test file that should be ignored + let fixture_path = PathBuf::from("tests/fixtures/app_with_gitignore"); + let test_file = fixture_path.join("test.global_ignore"); + fs::write(&test_file, "// Should be ignored by global gitignore\n")?; + + // Save original core.excludesFile config + let original_excludes_file = StdCommand::new("git") + .args(["config", "--global", "core.excludesFile"]) + .output() + .ok() + .and_then(|o| { + if o.status.success() { + Some(String::from_utf8_lossy(&o.stdout).trim().to_string()) + } else { + None + } + }); + + // Set core.excludesFile to our test global gitignore + let set_result = StdCommand::new("git") + .args([ + "config", + "--global", + "core.excludesFile", + &global_gitignore.to_string_lossy(), + ]) + .status(); + + assert!( + set_result.is_ok() && set_result.unwrap().success(), + "Failed to set core.excludesFile" + ); + + // Test that list-included-files excludes the globally ignored file + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg("tests/fixtures/app_with_gitignore") + .arg("list-included-files") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8_lossy(&output); + + // Should NOT include globally ignored file + assert!( + !stdout.contains("test.global_ignore"), + "Should NOT include file matched by global gitignore.\nOutput: {}", + stdout + ); + + // Cleanup: restore original core.excludesFile + if let Some(orig) = original_excludes_file { + if !orig.is_empty() { + StdCommand::new("git") + .args(["config", "--global", "core.excludesFile", &orig]) + .status() + .ok(); + } + } else { + // Unset if it wasn't set before + StdCommand::new("git") + .args(["config", "--global", "--unset", "core.excludesFile"]) + .status() + .ok(); + } + fs::remove_file(&test_file).ok(); + fs::remove_file(&global_gitignore).ok(); + + common::teardown(); + Ok(()) +} + +/// Test that gitignore works with the update command. +/// Gitignored files should not cause package_todo.yml updates. +#[test] +fn test_update_respects_gitignore() -> Result<(), Box> { + // Create a temporary copy of the fixture + let temp_dir = TempDir::new()?; + let temp_fixture = temp_dir.path().join("app"); + + // Copy fixture to temp directory + let fixture_path = "tests/fixtures/app_with_gitignore"; + copy_dir_all(fixture_path, &temp_fixture)?; + + // Run update command + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) + .arg("--project-root") + .arg(&temp_fixture) + .arg("update") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let stdout = String::from_utf8_lossy(&output); + + // Should not mention files in ignored_folder + assert!( + !stdout.contains("ignored_folder"), + "Update should not process gitignored files.\nOutput: {}", + stdout + ); + + common::teardown(); + Ok(()) +} + +// Helper function to copy directories recursively +fn copy_dir_all( + src: impl AsRef, + dst: impl AsRef, +) -> std::io::Result<()> { + fs::create_dir_all(&dst)?; + for entry in fs::read_dir(src)? { + let entry = entry?; + let ty = entry.file_type()?; + if ty.is_dir() { + copy_dir_all(entry.path(), dst.as_ref().join(entry.file_name()))?; + } else { + fs::copy(entry.path(), dst.as_ref().join(entry.file_name()))?; + } + } + Ok(()) +} diff --git a/tests/layer_violations_test.rs b/tests/layer_violations_test.rs index 8795dec..aa8c070 100644 --- a/tests/layer_violations_test.rs +++ b/tests/layer_violations_test.rs @@ -5,7 +5,7 @@ mod common; #[test] fn test_check_with_error_template_overrides() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/layer_violations_with_overrides") .arg("--debug") @@ -27,7 +27,7 @@ fn test_check_with_error_template_overrides() -> Result<(), Box> { } #[test] fn test_check() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/layer_violations") .arg("--debug") @@ -50,7 +50,7 @@ fn test_check() -> Result<(), Box> { #[test] fn test_check_enforce_layers_disabled() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/layer_violations") .arg("--debug") diff --git a/tests/list_definitions_test.rs b/tests/list_definitions_test.rs index 8b8dc3c..41938a2 100644 --- a/tests/list_definitions_test.rs +++ b/tests/list_definitions_test.rs @@ -5,7 +5,7 @@ mod common; #[test] fn test_list_definitions_experimental() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_monkey_patches") .arg("--debug") @@ -38,7 +38,7 @@ fn test_list_definitions_experimental() -> Result<(), Box> { #[test] fn test_list_definitions_with_ambiguous_experimental( ) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_monkey_patches") .arg("--debug") diff --git a/tests/list_packs_test.rs b/tests/list_packs_test.rs index 2c50b43..f451994 100644 --- a/tests/list_packs_test.rs +++ b/tests/list_packs_test.rs @@ -4,7 +4,7 @@ use std::{error::Error, process::Command}; #[test] fn lint_packs() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") diff --git a/tests/update_test.rs b/tests/update_test.rs index 8b3595b..4adfd0f 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -19,7 +19,7 @@ fn update_todo() -> Result<(), Box> { } fn test_update(command: &str) -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -63,8 +63,7 @@ packs/bar: #[test] #[serial] fn test_update_with_experimental_parser() -> Result<(), Box> { - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_app") .arg("--debug") @@ -110,8 +109,7 @@ packs/bar: fn test_update_with_stale_violations() -> Result<(), Box> { common::set_up_fixtures(); - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_stale_violations") .arg("update") @@ -157,7 +155,7 @@ packs/bar: #[test] fn test_update_with_packs_first_app() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/simple_packs_first_app") .arg("update") @@ -205,7 +203,7 @@ fn test_update_with_strict_violations() -> anyhow::Result<()> { ); let _ignore = std::fs::remove_file(path); - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/contains_strict_violations") .arg("update") diff --git a/tests/validate_test.rs b/tests/validate_test.rs index 0820eb5..12d47d6 100644 --- a/tests/validate_test.rs +++ b/tests/validate_test.rs @@ -14,8 +14,7 @@ The following groups of packages form a cycle: packs/foo, packs/bar", ); - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_dependency_cycles") .arg("--debug") @@ -41,8 +40,7 @@ fn test_validate_layer() -> Result<(), Box> { "Invalid \'layer\' option in \'packs/foo/package.yml\'. `layer` must be one of the layers defined in `packwerk.yml`" ); - Command::cargo_bin("pks") - .unwrap() + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/app_with_layer_violations_in_yml") .arg("validate") @@ -58,7 +56,7 @@ fn test_validate_layer() -> Result<(), Box> { #[test] fn test_validate_with_referencing_unknown_pack() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/references_unknown_pack") .arg("--debug") diff --git a/tests/visibility_test.rs b/tests/visibility_test.rs index df43e8a..6c9577c 100644 --- a/tests/visibility_test.rs +++ b/tests/visibility_test.rs @@ -6,7 +6,7 @@ mod common; #[test] fn test_check_with_visibility_violation_template_overrides( ) -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/visibility_violations_with_overrides") .arg("--debug") @@ -30,7 +30,7 @@ fn test_check_with_visibility_violation_template_overrides( #[test] fn test_check() -> Result<(), Box> { - let output = Command::cargo_bin("pks")? + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/visibility_violations") .arg("--debug") @@ -54,7 +54,7 @@ fn test_check() -> Result<(), Box> { #[test] fn test_check_disabled_enforce_visibility() -> Result<(), Box> { - Command::cargo_bin("pks")? + Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") .arg("tests/fixtures/visibility_violations") .arg("--debug")