diff --git a/CHANGELOG.md b/CHANGELOG.md index cbed246..2ccec80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,27 +1,70 @@ # Changelog + + ## Unreleased ### Breaking Changes -#### `respect_gitignore` defaults to `true` +#### Strict mode tolerates violations already recorded in `package_todo.yml` -pks now respects `.gitignore` files by default. Files and directories matched by -`.gitignore`, `.git/info/exclude`, or your global gitignore (`core.excludesFile`) -are excluded from analysis. +Any checker set to `strict` now fails only on references that are **not** already +recorded in a `package_todo.yml`. This matches packwerk's +`unlisted_strict_mode_violations` +([Shopify/packwerk#368](https://github.com/Shopify/packwerk/pull/368)). -**Who is affected:** any project that previously relied on pks analyzing gitignored -paths — for example, vendored code checked into `.gitignore`-excluded directories, -or generated files that matter for boundary checking. +This is not limited to privacy and dependencies. The filter is checker-agnostic, +so `enforce_layers: strict`, `enforce_visibility: strict` and strict folder +privacy relax in exactly the same way. If you are using one of those to hold a +boundary hard, this affects you too. -**What changes:** pks silently produces different (smaller) results without any -configuration change. This is intentional: most projects want gitignored files -excluded, and the old behavior (analyze everything) was rarely desired. +**Who is affected:** any project with a strict checker whose existing violations +are recorded in todo files. Previously `pks check` failed on every recorded strict +violation, so a strict package could only be green with no strict entries +recorded against it. -**Opt out:** add the following to `packwerk.yml` to restore the previous behavior: +Entries always live in the **referencing** package's `package_todo.yml`, which is +not always the strict package. For `enforce_privacy`, `enforce_visibility` and +folder privacy the enforcing package is the one being referenced, so look in the +*other* package's file. For `enforce_dependencies` and `enforce_layers` the +enforcing package is the referencing package, so the entries are in the strict +package's own file. -```yaml -respect_gitignore: false +**What changes, in `check`:** pks silently produces different (smaller) results +with no configuration change. Strict packages that were red because of +grandfathered violations go green. New references still fail, and a reference to +a different constant from an already-recorded file still fails. + +**What changes, in `update`, and this is the half that touches committed files:** +previously `update` dropped every strict violation when regenerating todo files, +and a package left with no entries had its `package_todo.yml` deleted outright. +So `update` used to erase recorded strict entries, which silently un-did the +tolerance `check` now depends on. It preserves them now. + +To be precise about the direction, because it is easy to read this as the +opposite: `update` never *adds* a strict entry. An unrecorded strict violation is +still not written, so strict mode cannot be adopted by running `update`. What +changed is that it stops **deleting** the entries that are already committed. If +your workflow previously relied on `update` clearing them, expect those lines to +survive where they used to disappear. + +**Adopting strict mode:** run `update` while the checker is still `true`, commit +the todo files, then set it to `strict`. Flipping first does not work, because +`update` will not record violations for a package that is already strict. See +CHECKERS.md. + +**No opt out:** there is no config flag, matching packwerk. `--ignore-recorded-violations` +is *not* a drop-in replacement for the old behaviour, because it also disables +recorded-violation filtering everywhere else and will surface every recorded +violation of every type in every package. It is useful for seeing what the todo +files are grandfathering: + +```sh +pks check --ignore-recorded-violations ``` ### Internal @@ -37,3 +80,27 @@ maintained fork, which is backed by `libyaml-rs` from the same org. support and lint cleanups, so this is behavior-preserving: the bytes pks writes to `package.yml` and `package_todo.yml` are unchanged, as are its YAML parse error messages. No action is required. + +## 0.4.0 + +### Breaking Changes + +#### `respect_gitignore` defaults to `true` + +pks now respects `.gitignore` files by default. Files and directories matched by +`.gitignore`, `.git/info/exclude`, or your global gitignore (`core.excludesFile`) +are excluded from analysis. + +**Who is affected:** any project that previously relied on pks analyzing gitignored +paths — for example, vendored code checked into `.gitignore`-excluded directories, +or generated files that matter for boundary checking. + +**What changes:** pks silently produces different (smaller) results without any +configuration change. This is intentional: most projects want gitignored files +excluded, and the old behavior (analyze everything) was rarely desired. + +**Opt out:** add the following to `packwerk.yml` to restore the previous behavior: + +```yaml +respect_gitignore: false +``` diff --git a/CHECKERS.md b/CHECKERS.md index 6cb2d18..1333f39 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -13,9 +13,26 @@ enforce_privacy: true Setting `enforce_privacy` to `true` will make all references to private constants in your package a violation. -Setting `enforce_privacy` to `strict` will forbid all references to private constants in your package. **This includes violations that have been added to other packages' `package_todo.yml` files.** +Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in the referencing package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty. -Note: You will need to remove all existing privacy violations before setting `enforce_privacy` to `strict`. +### Adopting strict mode on a package that already has violations + +**Record the existing violations first, then flip to `strict`.** The order matters, because tolerance only ever matches entries that are *already* in a `package_todo.yml`, and `update` will not create them once the package is strict: + +```sh +# 1. while the package is still `enforce_privacy: true` +pks update + +# 2. commit the package_todo.yml files this wrote + +# 3. now set enforce_privacy: strict +``` + +Flipping to `strict` first leaves you stuck: `check` fails on the existing references, and `pks update` will not record them, so the only ways out are fixing every reference, hand-writing the todo entries, or reverting to `true`. pks matches packwerk here. + +To see everything the todo files are currently grandfathering, run `pks check --ignore-recorded-violations`. + +Once the package is strict, `pks update` will not add new entries for it: an unrecorded strict violation is never written to a `package_todo.yml`, so it keeps failing until the reference is dealt with. Note that this is a guarantee about `update`, not about the file. A hand-added entry does silence strict mode, and `update` preserves it rather than dropping it, so the boundary is only as strong as your review of `package_todo.yml` diffs. ### Using public folders You may enforce privacy either way mentioned above and still expose a public API for your package by placing constants in the public folder, which by default is `app/public`. The constants in the public folder will be made available for use by the rest of the application. @@ -98,17 +115,29 @@ end => Ideal solution. No exceptions from rubocop and very low risk of the magic ### Using specific private constants Sometimes it is desirable to only enforce privacy on a subset of constants in a package. You can do so by defining a `private_constants` list in your package.yml. Note that `enforce_privacy` must be set to `true` or `'strict'` for this to work. -### Ignore strict mode for violation coming from specific path patterns -If you want to activate `'strict'` mode on your package but have a few privacy violations you know you will deal with later, -you can set a list of patterns to exclude. +### Ignore strict mode for violations coming from specific path patterns +You do not need this to adopt `'strict'` mode on a package that already has violations you will deal with later: record them first and they are tolerated, as described above. Reach for a path exemption when you want to exempt a **path** rather than a recorded list. + +Use [`enforcement_globs_ignore`](#enforcement-globs-ignore) with `enforcements: [privacy]`: ```yaml enforce_privacy: strict -strict_privacy_ignored_patterns: -- engines/another_engine/test/**/* + +enforcement_globs_ignore: +- enforcements: + - privacy + ignores: + - engines/another_engine/test/** + reason: test files reach into engine internals ``` -In this example, violations on constants of your engine referenced in those files `engines/another_engine/test/**/*` will not fail Packwerk checks. +In this example, privacy violations on constants of your engine referenced from anywhere under `engines/another_engine/test/` will not fail pks checks. + +Note the trailing `**` rather than `**/*`. `**` matches the whole subtree including files directly inside `test/`, whereas `**/*` requires at least one intervening directory and so silently skips `test/a_test.rb`. Do not reason about these from gitignore: `git check-ignore` treats `test/**` and `test/**/*` identically, and pks does not, because matching goes through `fnmatch_regex2::glob_to_regex` (`src/packs/ignored.rs`) rather than gitignore semantics. A pattern that matches nothing looks identical to no exemption at all, so check a new pattern against a file you expect it to cover. + +> **Note:** packwerk spells this `strict_privacy_ignored_patterns`. **pks does not implement that key**, and because `Pack` collects unknown keys via `#[serde(flatten)]` it is accepted silently and has no effect, which leaves the pack unguarded. Use `enforcement_globs_ignore` instead. + +The two mechanisms differ in what they grandfather, so they are not interchangeable. A `package_todo.yml` entry covers one constant referenced from one file, for one violation type, so a reference to a *different* constant from that same file still fails. A path exemption covers the path outright, so anything those files reference later is ignored too. Prefer the todo file unless you genuinely want the whole path exempt. ### Package Privacy violation Packwerk thinks something is a privacy violation if you're referencing a constant, class, or module defined in the private implementation (i.e. not the public folder) of another package. We care about these because we want to make sure we only use parts of a package that have been exposed as public API. diff --git a/src/packs/checker.rs b/src/packs/checker.rs index cfd694b..7c7f419 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -39,6 +39,20 @@ pub struct ViolationIdentifier { pub referencing_pack_name: String, pub defining_pack_name: String, } + +impl ViolationIdentifier { + /// `strict` describes how a violation should be treated, not which violation + /// it is, and `package_todo.yml` has nowhere to record it, so recorded + /// violations are always rebuilt with `strict: false`. Compare through this + /// so a violation in a strict pack can still match its recorded entry. + pub(crate) fn recorded_key(&self) -> Self { + Self { + strict: false, + ..self.clone() + } + } +} + /// A violation combines an identifier with display metadata. /// /// `source_location` is intentionally separate from `ViolationIdentifier` because: @@ -124,7 +138,7 @@ impl<'a> CheckAllBuilder<'a> { .cloned() .collect(), strict_mode_violations: self - .build_strict_mode_violations() + .build_strict_mode_violations(recorded_violations) .into_iter() .collect(), }) @@ -142,7 +156,10 @@ impl<'a> CheckAllBuilder<'a> { self.found_violations .violations .iter() - .filter(|v| !recorded_violations.contains(&v.identifier)) + .filter(|v| { + !recorded_violations + .contains(&v.identifier.recorded_key()) + }) .collect() }; reportable_violations @@ -152,11 +169,11 @@ impl<'a> CheckAllBuilder<'a> { &mut self, recorded_violations: &'a HashSet, ) -> anyhow::Result> { - let found_violation_identifiers: HashSet<&ViolationIdentifier> = self + let found_violation_identifiers: HashSet = self .found_violations .violations .par_iter() - .map(|v| &v.identifier) + .map(|v| v.identifier.recorded_key()) .collect(); let relative_files = self .found_violations @@ -196,9 +213,13 @@ impl<'a> CheckAllBuilder<'a> { Ok(stale_violations) } + /// `found_violation_identifiers` is keyed by [`ViolationIdentifier::recorded_key`]. + /// `todo_violation_identifier` needs no such normalization: it comes from + /// `pack_set.all_violations`, which rebuilds every recorded violation with + /// `strict: false` already, so it is its own recorded key. fn is_stale_violation( relative_files: &HashSet<&str>, - found_violation_identifiers: &HashSet<&ViolationIdentifier>, + found_violation_identifiers: &HashSet, todo_violation_identifier: &ViolationIdentifier, ) -> bool { let violation_path_exists = @@ -210,11 +231,23 @@ impl<'a> CheckAllBuilder<'a> { } } - fn build_strict_mode_violations(&self) -> Vec { + /// Strict mode reports violations that are not already recorded in a + /// `package_todo.yml`, matching packwerk's `unlisted_strict_mode_violations` + /// (Shopify/packwerk#368). Turning strict on therefore blocks new violations + /// without also requiring every recorded one to be fixed first. + fn build_strict_mode_violations( + &self, + recorded_violations: &HashSet, + ) -> Vec { self.found_violations .violations .iter() .filter(|v| v.identifier.strict) + .filter(|v| { + self.configuration.ignore_recorded_violations + || !recorded_violations + .contains(&v.identifier.recorded_key()) + }) .cloned() .collect() } @@ -302,22 +335,35 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { &checkers, )?; - let strict_violations = &violations + let recorded_violations = &configuration.pack_set.all_violations; + + // Only *unlisted* strict violations make `check` fail, so only those are + // worth reporting here. Reporting recorded ones too claimed `check` would + // fail when it succeeds. Same filter as `build_strict_mode_violations`, and + // as packwerk's `unlisted_strict_mode_violations`. + let unlisted_strict_violations = &violations .iter() .filter(|v| v.identifier.strict) + .filter(|v| !recorded_violations.contains(&v.identifier.recorded_key())) .collect::>(); - if !strict_violations.is_empty() { - for violation in strict_violations { + if !unlisted_strict_violations.is_empty() { + for violation in unlisted_strict_violations { let strict_message = build_strict_violation_message(&violation.identifier); println!("{}", strict_message); } println!( "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.", - &strict_violations.len() + &unlisted_strict_violations.len() ); + // TODO: packwerk's `update-todo` exits non-zero here; `update` returns + // Ok and prints a success line. Pre-existing, separate breaking change. } - package_todo::write_violations_to_disk(configuration, violations); + package_todo::write_violations_to_disk( + configuration, + violations, + recorded_violations, + ); println!("Successfully updated package_todo.yml files!"); Ok(()) diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 954a1e9..79dfce1 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -3,6 +3,7 @@ use serde::{ser::SerializeMap, Deserialize, Serialize, Serializer}; use std::collections::{BTreeMap, HashMap, HashSet}; use tracing::debug; +use super::checker::ViolationIdentifier; use super::{pack::Pack, Configuration, Violation}; #[derive(PartialEq, Debug, Eq, Deserialize, Serialize, Default, Clone)] @@ -133,6 +134,7 @@ pub fn package_todos_for_pack_name( pub fn write_violations_to_disk( configuration: &Configuration, violations: HashSet, + recorded_violations: &HashSet, ) { debug!("Starting writing violations to disk"); // First we need to group the violations by the responsible pack, which today is always the referencing pack @@ -141,7 +143,18 @@ pub fn write_violations_to_disk( let mut violations_by_responsible_pack: HashMap> = HashMap::new(); for violation in violations { - if violation.identifier.strict { + // An *unlisted* strict violation is never recorded, so `update` cannot + // be used to silence strict mode. An already-recorded one has to be + // re-written, because `check` now tolerates recorded violations in + // strict packs and `PackageTodo` is dumped wholesale from these + // entries — dropping it here would delete the record that made the + // build green and fail the next `check` with no source change in + // between. packwerk keeps the entry for the same reason, in + // `OffenseCollection#add_offense`. + if violation.identifier.strict + && !recorded_violations + .contains(&violation.identifier.recorded_key()) + { continue; } let referencing_pack_name = diff --git a/tests/check_test.rs b/tests/check_test.rs index 4f0d043..ca3c02c 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -320,12 +320,34 @@ fn test_check_without_stale_violations() -> Result<(), Box> { } #[test] -fn test_check_with_strict_mode() -> Result<(), Box> { +fn test_check_with_recorded_strict_mode_violation() -> Result<(), Box> +{ + // The violation is already recorded in packs/foo/package_todo.yml, so strict + // mode tolerates it and only blocks new ones. Matches packwerk's + // `unlisted_strict_mode_violations` (Shopify/packwerk#368). cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") .assert() + .code(0) + .stdout(predicate::str::contains("No violations detected!")); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_recorded_strict_mode_violation_ignoring_todo( +) -> Result<(), Box> { + // `--ignore-recorded-violations` is the escape hatch: it still surfaces + // everything the todo file is grandfathering. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("--ignore-recorded-violations") + .assert() .code(1) .stdout(predicate::str::contains( "packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", @@ -338,18 +360,119 @@ fn test_check_with_strict_mode() -> Result<(), Box> { Ok(()) } +#[test] +fn test_check_with_unrecorded_strict_mode_violation( +) -> Result<(), Box> { + // No package_todo.yml entry for this one, so strict mode must still fail. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_strict_violations") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + )); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_partially_recorded_strict_mode_violations( +) -> Result<(), Box> { + // The case that makes strict mode adoptable: a recorded violation (::Bar) + // and an unrecorded one (::Baz) in the same strict pack, in the same run. + // Only the unrecorded one is reported, and the run still fails because of + // it. This fixture also carries ::Qux with a single-type entry, read by + // `test_check_with_single_recorded_violation_type_in_strict_pack`, so + // ::Qux's dependency message is expected in this output too. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_partially_recorded") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have privacy violations on packs/baz because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + )) + .stdout(predicate::str::contains( + "packs/foo cannot have dependency violations on packs/baz because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", + )) + .stdout(predicate::str::contains("::Baz")) + // The recorded one stays silent: no strict message, no new-violation + // report, no stale-todo line. + .stdout(predicate::str::contains("packs/bar").not()) + .stdout(predicate::str::contains("::Bar").not()) + .stdout( + predicate::str::contains( + "There were stale violations found, please run `packs update`", + ) + .not(), + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_single_recorded_violation_type_in_strict_pack( +) -> Result<(), Box> { + // `::Qux` is recorded for `privacy` only, while `packs/qux` is strict on both + // privacy and dependencies. Recording one type must tolerate only that type, + // so the dependency violation still fails the run. + // + // This pins `violation_type` inside the recorded-comparison key. A refactor + // that normalized it away, the way `strict` is normalized by + // `ViolationIdentifier::recorded_key`, would silence both types from a + // single-type entry. + // + // Measured, because the obvious way to say this overstates it: collapsing + // `violation_type` in the strict filter *specifically* fails exactly this + // test and nothing else in the suite. Collapsing it everywhere in + // `recorded_key` fails eight tests, since it also breaks reportable and + // stale comparisons, so that coarser mutation proves nothing about this + // one. Every other strict fixture records privacy and dependency together, + // which is why the narrow case needs its own coverage. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_partially_recorded") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have dependency violations on packs/qux because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", + )) + .stdout( + predicate::str::contains( + "packs/foo cannot have privacy violations on packs/qux because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + ) + .not(), + ); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { + // A CSV format test, and only that. It uses `contains_strict_violations` + // rather than `uses_strict_mode` because the latter's violation is recorded, + // so there is nothing left to assert against in the CSV. Note that this + // fixture ships no `package_todo.yml`, so the violation is an ordinary + // unrecorded one: this test does not exercise strict tolerance and passes + // with the recorded filter disabled. The duplicate assertion it used to + // carry was byte-identical to the one below it, so dropping it costs no + // coverage. cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/uses_strict_mode") + .arg("tests/fixtures/contains_strict_violations") .arg("check") .arg("-o") .arg("csv") .assert() .code(1) .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message")) - .stdout(predicate::str::contains("privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file")) .stdout(predicate::str::contains( "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file", )); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index eb09c23..fef07fa 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -55,6 +55,89 @@ pub fn delete_foobar_app_with_custom_readme() { } } +// The round-trip strict-mode fixture. Its todo file records a strict violation, +// which is the state `check` tolerance depends on, and its source file provides +// the reference that entry points at. Tests here mutate both, so they restore +// via the guard below rather than a trailing call: a panicking test would skip +// a trailing restore and leave a deleted fixture in the tree, which the +// pre-commit hook makes easy to commit by accident. +#[allow(dead_code)] +pub const ROUND_TRIP_TODO_PATH: &str = + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml"; + +#[allow(dead_code)] +pub const ROUND_TRIP_SOURCE_PATH: &str = + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb"; + +// Violation types are in sorted order, matching what `update` writes, so tests +// can assert byte equality against this rather than only grepping for a key. +#[allow(dead_code)] +pub const ROUND_TRIP_TODO: &str = "\ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + \"::Bar\": + violations: + - dependency + - privacy + files: + - packs/foo/app/services/foo.rb +"; + +#[allow(dead_code)] +pub const ROUND_TRIP_SOURCE: &str = "\ +module Foo + def calls_bar_without_stated_dependency + Bar + end +end +"; + +/// Restores the round-trip fixture when it goes out of scope, panic or not. +/// +/// Bind it to a named variable, `let _fixture = RoundTripFixture::set_up();`. +/// Binding to `let _` drops it immediately and silently removes the protection, +/// which `#[must_use]` cannot catch. +#[allow(dead_code)] +#[must_use = "bind this to a named variable; `let _` drops the guard immediately"] +pub struct RoundTripFixture; + +#[allow(dead_code)] +impl RoundTripFixture { + pub fn set_up() -> Self { + // Panicking here is fine and informative: the test has not run yet. + fs::write(ROUND_TRIP_TODO_PATH, ROUND_TRIP_TODO).unwrap(); + fs::write(ROUND_TRIP_SOURCE_PATH, ROUND_TRIP_SOURCE).unwrap(); + Self + } +} + +impl Drop for RoundTripFixture { + fn drop(&mut self) { + // Deliberately does not unwrap. This runs while a failing test is + // unwinding, and a panic here would be a panic-during-panic, which + // aborts the whole test binary and replaces the real failure with an + // abort. Report and carry on, as `teardown()` does. + for (path, contents) in [ + (ROUND_TRIP_TODO_PATH, ROUND_TRIP_TODO), + (ROUND_TRIP_SOURCE_PATH, ROUND_TRIP_SOURCE), + ] { + if let Err(err) = fs::write(path, contents) { + eprintln!( + "Failed to restore {} during teardown: {}", + path, err + ); + } + } + } +} + // In case we want our tests to call `update` or otherwise mutate the file system #[allow(dead_code)] pub fn set_up_fixtures() { diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5003150 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb new file mode 100644 index 0000000..dbe89a2 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb @@ -0,0 +1,2 @@ +module Baz +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..7435e66 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb @@ -0,0 +1,13 @@ +module Foo + def calls_bar_without_stated_dependency + Bar + end + + def calls_baz_without_stated_dependency + Baz + end + + def calls_qux_without_stated_dependency + Qux + end +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml new file mode 100644 index 0000000..8afa101 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml @@ -0,0 +1,21 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + "::Bar": + violations: + - dependency + - privacy + files: + - packs/foo/app/services/foo.rb +packs/qux: + "::Qux": + violations: + - privacy + files: + - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb new file mode 100644 index 0000000..c528918 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb @@ -0,0 +1,2 @@ +module Qux +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml new file mode 100644 index 0000000..51f2f3b --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/fixtures/uses_strict_mode_round_trip/package.yml b/tests/fixtures/uses_strict_mode_round_trip/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5003150 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..0884fdb --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb @@ -0,0 +1,5 @@ +module Foo + def calls_bar_without_stated_dependency + Bar + end +end diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml new file mode 100644 index 0000000..fd1f18f --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml @@ -0,0 +1,15 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + "::Bar": + violations: + - dependency + - privacy + files: + - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml b/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml new file mode 100644 index 0000000..51f2f3b --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/update_test.rs b/tests/update_test.rs index e8d76ff..5ec0ecf 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -197,6 +197,126 @@ packs/bar: } #[test] +#[serial] +// These three all mutate tests/fixtures/uses_strict_mode_round_trip, so they run +// in serial and restore through `RoundTripFixture`'s Drop rather than a trailing +// call, which a panicking test would skip. +fn test_update_preserves_recorded_strict_violations() -> anyhow::Result<()> { + let _fixture = common::RoundTripFixture::set_up(); + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success() + .stdout(predicate::str::contains( + "Successfully updated package_todo.yml files!", + )) + // The violation is recorded, so `check` tolerates it. Claiming it must + // be fixed for `check` to succeed would be false. + .stdout( + predicate::str::contains( + "These violations must be fixed for `check` to succeed.", + ) + .not(), + ); + + // Byte equality, not a substring: this pins the whole file `update` writes, + // so a change that preserved the entry but mangled the rest is caught too. + let actual = std::fs::read_to_string(common::ROUND_TRIP_TODO_PATH)?; + assert_eq!(common::ROUND_TRIP_TODO, actual); + + Ok(()) +} + +#[test] +#[serial] +// The counterpart to the test above, and the one that stops the obvious +// over-correction. Preserving recorded strict violations must not make them +// immortal: once the reference is gone the entry still has to be pruned. Union +// `recorded_violations` into the write set instead of intersecting it with the +// found violations and this test fails, as does the pre-existing +// `test_update_with_stale_violations`. That one uses a non-strict fixture, so +// this is the only coverage of the strict path. +fn test_update_prunes_recorded_strict_violation_once_reference_is_gone( +) -> anyhow::Result<()> { + let _fixture = common::RoundTripFixture::set_up(); + + std::fs::write( + common::ROUND_TRIP_SOURCE_PATH, + "module Foo\n def no_longer_references_bar\n :nothing\n end\nend\n", + )?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "There were stale violations found, please run `packs update`", + )); + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success(); + + assert!( + !Path::new(common::ROUND_TRIP_TODO_PATH).exists(), + "update must prune a recorded strict violation whose reference is gone, \ + otherwise `check` stays green forever for code that no longer exists" + ); + + Ok(()) +} + +#[test] +#[serial] +fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { + let _fixture = common::RoundTripFixture::set_up(); + + let assert_check_is_clean = || { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("check") + .assert() + .code(0) + .stdout(predicate::str::contains("No violations detected!")); + }; + + // A routine `update` between two checks must not turn a green build red. + assert_check_is_clean(); + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success(); + assert_check_is_clean(); + + // And it must leave the file exactly as it found it. + let actual = std::fs::read_to_string(common::ROUND_TRIP_TODO_PATH)?; + assert_eq!(common::ROUND_TRIP_TODO, actual); + + Ok(()) +} + +#[test] +// Shares `contains_strict_violations` with `check_test.rs`, which reads it. That +// is safe only because of what this test asserts: the committed fixture has no +// `package_todo.yml`, the `remove_file` below is defensive, and the assertion is +// that `update` does not create one. So the fixture is invariant across this +// test. If that assertion ever inverts, give this test its own fixture copy, +// because `serial_test` here has no `file_locks` feature and so cannot serialise +// across test binaries. Cargo runs test binaries sequentially, so the coupling is +// latent rather than live, but that is cargo's behaviour and not a property of +// this design: `cargo-nextest` runs tests from different binaries concurrently, +// so adopting it would make this live without anyone touching this test. fn test_update_with_strict_violations() -> anyhow::Result<()> { let path = Path::new( "tests/fixtures/contains_strict_violations/packs/foo/package_todo.yml",