From fcaa3952ed7f064bdca80021083a40535b1b3f38 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Tue, 9 Aug 2022 23:41:17 -0400 Subject: [PATCH 1/8] Refactor out package scanning into PackagePaths Moves arguments passed into the self methods on `PackageSet` into a new `PackagePaths` object. This makes it easier to add new arguments without alot of parameter drilling. This will support the next commit where we add the ability to conditionally find packages outside of the app dir --- lib/packwerk.rb | 2 + lib/packwerk/configuration.rb | 4 + lib/packwerk/package_paths.rb | 53 +++++++ lib/packwerk/package_set.rb | 38 +---- lib/packwerk/run_context.rb | 12 +- lib/packwerk/validator.rb | 9 +- test/unit/application_validator_test.rb | 201 ++++++++++++++++++++++++ test/unit/package_paths_test.rb | 93 +++++++++++ test/unit/packwerk/package_set_test.rb | 66 -------- 9 files changed, 375 insertions(+), 103 deletions(-) create mode 100644 lib/packwerk/package_paths.rb create mode 100644 test/unit/application_validator_test.rb create mode 100644 test/unit/package_paths_test.rb diff --git a/lib/packwerk.rb b/lib/packwerk.rb index 39023d1ca..a7b11b9e1 100644 --- a/lib/packwerk.rb +++ b/lib/packwerk.rb @@ -27,6 +27,8 @@ module Packwerk autoload :Package autoload :PackageSet autoload :PackageTodo + autoload :PackagePaths + autoload :ParsedConstantDefinitions autoload :Parsers autoload :RailsLoadPaths autoload :Reference diff --git a/lib/packwerk/configuration.rb b/lib/packwerk/configuration.rb index 22a7dee20..11302f3f7 100644 --- a/lib/packwerk/configuration.rb +++ b/lib/packwerk/configuration.rb @@ -60,6 +60,9 @@ def from_packwerk_config(path) sig { returns(Pathname) } attr_reader(:cache_directory) + sig { returns(T::Boolean) } + attr_reader(:packages_outside_of_app_dir_enabled) + sig { params(parallel: T::Boolean).returns(T::Boolean) } attr_writer(:parallel) @@ -80,6 +83,7 @@ def initialize(configs = {}, config_path: nil) @cache_enabled = T.let(configs.key?("cache") ? configs["cache"] : false, T::Boolean) @cache_directory = T.let(Pathname.new(configs["cache_directory"] || "tmp/cache/packwerk"), Pathname) @config_path = config_path + @packages_outside_of_app_dir_enabled = configs["packages_outside_of_app_dir_enabled"] || false @offenses_formatter_identifier = T.let( configs["offenses_formatter"] || Formatters::DefaultOffensesFormatter::IDENTIFIER, String diff --git a/lib/packwerk/package_paths.rb b/lib/packwerk/package_paths.rb new file mode 100644 index 000000000..791125db6 --- /dev/null +++ b/lib/packwerk/package_paths.rb @@ -0,0 +1,53 @@ +# typed: strict +# frozen_string_literal: true + +require "pathname" +require "bundler" + +module Packwerk + class PackagePaths + PACKAGE_CONFIG_FILENAME = "package.yml" + PathSpec = T.type_alias { T.any(String, T::Array[String]) } + + extend T::Sig + extend T::Generic + + sig do + params( + root_path: String, + package_pathspec: T.nilable(PathSpec), + exclude_pathspec: T.nilable(PathSpec), + ).void + end + def initialize(root_path, package_pathspec, exclude_pathspec = nil) + @root_path = root_path + @package_pathspec = package_pathspec + @exclude_pathspec = exclude_pathspec + end + + def all_paths + exclude_pathspec = Array(@exclude_pathspec).dup + .push(Bundler.bundle_path.join("**").to_s) + .map { |glob| File.expand_path(glob) } + + paths_to_scan = [@root_path] + + glob_patterns = paths_to_scan.product(Array(@package_pathspec)).map do |path, pathspec| + File.join(path, pathspec, PACKAGE_CONFIG_FILENAME) + end + + Dir.glob(glob_patterns) + .map { |path| Pathname.new(path).cleanpath } + .reject { |path| exclude_path?(exclude_pathspec, path) } + end + + private + + sig { params(globs: T::Array[String], path: Pathname).returns(T::Boolean) } + def exclude_path?(globs, path) + globs.any? do |glob| + path.realpath.fnmatch(glob, File::FNM_EXTGLOB) + end + end + end +end diff --git a/lib/packwerk/package_set.rb b/lib/packwerk/package_set.rb index 251c36f59..a64386652 100644 --- a/lib/packwerk/package_set.rb +++ b/lib/packwerk/package_set.rb @@ -5,7 +5,6 @@ require "bundler" module Packwerk - PathSpec = T.type_alias { T.any(String, T::Array[String]) } # A set of {Packwerk::Package}s as well as methods to parse packages from the filesystem. class PackageSet @@ -15,16 +14,17 @@ class PackageSet Elem = type_member { { fixed: Package } } - PACKAGE_CONFIG_FILENAME = "package.yml" class << self extend T::Sig - sig { params(root_path: String, package_pathspec: T.nilable(PathSpec)).returns(PackageSet) } + sig do + params(root_path: String, package_pathspec: T.nilable(PackagePaths::PathSpec)).returns(PackageSet) + end def load_all_from(root_path, package_pathspec: nil) - package_paths = package_paths(root_path, package_pathspec || "**") + package_paths = PackagePaths.new(root_path, package_pathspec || "**", nil) - packages = package_paths.map do |path| + packages = package_paths.all_paths.map do |path| root_relative = path.dirname.relative_path_from(root_path) Package.new(name: root_relative.to_s, config: YAML.load_file(path, fallback: nil)) end @@ -34,27 +34,6 @@ def load_all_from(root_path, package_pathspec: nil) new(packages) end - sig do - params( - root_path: String, - package_pathspec: PathSpec, - exclude_pathspec: T.nilable(PathSpec) - ).returns(T::Array[Pathname]) - end - def package_paths(root_path, package_pathspec, exclude_pathspec = []) - exclude_pathspec = Array(exclude_pathspec).dup - .push(Bundler.bundle_path.join("**").to_s) - .map { |glob| File.expand_path(glob) } - - glob_patterns = Array(package_pathspec).map do |pathspec| - File.join(root_path, pathspec, PACKAGE_CONFIG_FILENAME) - end - - Dir.glob(glob_patterns) - .map { |path| Pathname.new(path).cleanpath } - .reject { |path| exclude_path?(exclude_pathspec, path) } - end - private sig { params(packages: T::Array[Package]).void } @@ -64,12 +43,7 @@ def create_root_package_if_none_in(packages) packages << Package.new(name: Package::ROOT_PACKAGE_NAME, config: nil) end - sig { params(globs: T::Array[String], path: Pathname).returns(T::Boolean) } - def exclude_path?(globs, path) - globs.any? do |glob| - path.realpath.fnmatch(glob, File::FNM_EXTGLOB) - end - end + end sig { returns(T::Hash[String, Package]) } diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 8dae554a1..4549b4241 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -26,6 +26,7 @@ def from_configuration(configuration) cache_enabled: configuration.cache_enabled?, cache_directory: configuration.cache_directory, config_path: configuration.config_path, + packages_outside_of_app_dir_enabled: configuration.packages_outside_of_app_dir_enabled, ) end end @@ -52,7 +53,8 @@ def initialize( package_paths: nil, custom_associations: [], checkers: Checker.all, - cache_enabled: false + cache_enabled: false, + packages_outside_of_app_dir_enabled: false ) @root_path = root_path @load_paths = load_paths @@ -63,7 +65,7 @@ def initialize( @cache_enabled = cache_enabled @cache_directory = cache_directory @config_path = config_path - + @packages_outside_of_app_dir_enabled = packages_outside_of_app_dir_enabled @file_processor = T.let(nil, T.nilable(FileProcessor)) @context_provider = T.let(nil, T.nilable(ConstantDiscovery)) @package_set = T.let(nil, T.nilable(PackageSet)) @@ -88,7 +90,11 @@ def process_file(relative_file:) sig { returns(PackageSet) } def package_set - @package_set ||= ::Packwerk::PackageSet.load_all_from(@root_path, package_pathspec: @package_paths) + @package_set ||= ::Packwerk::PackageSet.load_all_from( + @root_path, + package_pathspec: @package_paths, + scan_for_packages_outside_of_app_dir: @packages_outside_of_app_dir_enabled, + ) end private diff --git a/lib/packwerk/validator.rb b/lib/packwerk/validator.rb index a79de1eea..8edcd27fc 100644 --- a/lib/packwerk/validator.rb +++ b/lib/packwerk/validator.rb @@ -61,8 +61,13 @@ def package_manifests_settings_for(configuration, setting) end def package_manifests(configuration, glob_pattern = nil) glob_pattern ||= package_glob(configuration) - PackageSet.package_paths(configuration.root_path, glob_pattern, configuration.exclude) - .map { |f| File.realpath(f) } + package_paths = PackagePaths.new( + @configuration.root_path, + glob_pattern, + @configuration.exclude, + @configuration.packages_outside_of_app_dir_enabled + ) + package_paths.all_paths.map { |f| File.realpath(f) } end sig { params(configuration: Configuration).returns(T.any(T::Array[String], String)) } diff --git a/test/unit/application_validator_test.rb b/test/unit/application_validator_test.rb new file mode 100644 index 000000000..7db5bac5a --- /dev/null +++ b/test/unit/application_validator_test.rb @@ -0,0 +1,201 @@ +# typed: true +# frozen_string_literal: true + +require "test_helper" + +# make sure PrivateThing.constantize succeeds to pass the privacy validity check +require "fixtures/skeleton/components/timeline/app/models/private_thing.rb" + +module Packwerk + class ApplicationValidatorTest < Minitest::Test + include RailsApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end + + test "validity" do + use_template(:skeleton) + + result = validator.check_all + + assert result.ok?, result.error_value + end + + test "check_package_manifest_syntax returns an error for unknown package keys" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_correctness" => false }) + + result = validator.check_package_manifest_syntax + + refute result.ok? + assert_match(/Unknown keys/, result.error_value) + end + + test "check_package_manifest_syntax returns an error for invalid enforce_privacy value" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_privacy" => "yes, please." }) + + result = validator.check_package_manifest_syntax + + refute result.ok? + assert_match(/Invalid 'enforce_privacy' option/, result.error_value) + end + + test "check_package_manifest_syntax returns an error for invalid enforce_dependencies value" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_dependencies" => "components/sales" }) + + result = validator.check_package_manifest_syntax + + refute result.ok? + assert_match(/Invalid 'enforce_dependencies' option/, result.error_value) + end + + test "check_package_manifest_syntax returns an error for invalid public_path value" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "public_path" => [] }) + + result = validator.check_package_manifest_syntax + + refute result.ok? + assert_match(/'public_path' option must be a string/, result.error_value) + end + + test "check_package_manifest_syntax returns error for invalid dependencies value" do + use_template(:minimal) + merge_into_app_yaml_file("components/timeline/package.yml", {}) + merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => "components/timeline" }) + + result = validator.check_package_manifest_syntax + + refute result.ok? + assert_match(/Invalid 'dependencies' option/, result.error_value) + end + + test "check_package_manifests_for_privacy returns an error for unresolvable privatized constants" do + use_template(:skeleton) + ConstantResolver.expects(:new).returns(stub("resolver", resolve: nil)) + + result = validator.check_package_manifests_for_privacy + + refute result.ok?, result.error_value + assert_match( + /'::PrivateThing', listed in #{to_app_path('components\/timeline\/package.yml')}, could not be resolved/, + result.error_value + ) + assert_match( + /Add a private_thing.rb file/, + result.error_value + ) + end + + test "check_package_manifests_for_privacy returns an error for privatized constants in other packages" do + use_template(:skeleton) + context = ConstantResolver::ConstantContext.new("::PrivateThing", "private_thing.rb") + + ConstantResolver.expects(:new).returns(stub("resolver", resolve: context)) + + result = validator.check_package_manifests_for_privacy + + refute result.ok?, result.error_value + assert_match( + %r{'::PrivateThing' is declared as private in the 'components/timeline' package}, + result.error_value + ) + assert_match( + /but appears to be defined\sin the '.' package/, + result.error_value + ) + end + + test "check_package_manifests_for_privacy returns an error for constants without `::` prefix" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_privacy" => ["::PrivateThing", "OtherThing"] }) + + result = validator.check_package_manifests_for_privacy + + refute result.ok?, result.error_value + assert_match( + /'OtherThing', listed in the 'enforce_privacy' option in .*package.yml, is invalid./, + result.error_value + ) + assert_match( + /Private constants need to be prefixed with the top-level namespace operator `::`/, + result.error_value + ) + end + + test "check_acyclic_graph returns error when package set contains circular dependencies" do + use_template(:minimal) + merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => ["components/timeline"] }) + merge_into_app_yaml_file("components/timeline/package.yml", { "dependencies" => ["components/sales"] }) + + result = validator.check_acyclic_graph + + refute result.ok? + assert_match(/Expected the package dependency graph to be acyclic/, result.error_value) + assert_match %r{components/sales → components/timeline → components/sales}, result.error_value + end + + test "check_package_manifest_paths returns error when config only declares partial list of packages" do + use_template(:minimal) + merge_into_app_yaml_file("components/timeline/package.yml", {}) + merge_into_app_yaml_file("packwerk.yml", { "package_paths" => ["components/sales", "."] }) + + result = validator.check_package_manifest_paths + + refute result.ok? + assert_match(/Expected package paths for all package.ymls to be specified/, result.error_value) + assert_match %r{manifests:\n\ncomponents/timeline/package.yml$}m, result.error_value + end + + test "check_package_manifest_paths returns no error when vendor/**/* is excluded" do + use_template(:skeleton) + merge_into_app_yaml_file("components/timeline/package.yml", {}) + merge_into_app_yaml_file("packwerk.yml", { "package_paths" => ["components/**/*", "."] }) + merge_into_app_yaml_file("packwerk.yml", { "exclude" => ["vendor/**/*"] }) + + package_paths = PackagePaths.new(".", "**").all_paths + vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") + assert_includes(package_paths, vendor_package_path) + + result = validator.check_package_manifest_paths + + assert result.ok? + refute result.error_value + end + + test "check_valid_package_dependencies returns error when config contains invalid package dependency" do + use_template(:minimal) + merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => ["components/timeline"] }) + + result = validator.check_valid_package_dependencies + + refute result.ok? + assert_match(/These dependencies do not point to valid packages:/, result.error_value) + assert_match %r{\n\ncomponents/sales/package.yml:\n - components/timeline\n\n$}m, result.error_value + end + + test "check_root_package_exists returns error when root directory is missing a package.yml file" do + use_template(:minimal) + remove_app_entry("package.yml") + + result = validator.check_root_package_exists + refute result.ok? + assert_match(/A root package does not exist./, result.error_value) + end + + def validator + @application_validator ||= Packwerk::ApplicationValidator.new( + config_file_path: config.config_path, + configuration: config, + environment: "test" + ) + end + end +end diff --git a/test/unit/package_paths_test.rb b/test/unit/package_paths_test.rb new file mode 100644 index 000000000..4a2f427a8 --- /dev/null +++ b/test/unit/package_paths_test.rb @@ -0,0 +1,93 @@ +# typed: true +# frozen_string_literal: true + +require "test_helper" + +module Packwerk + class PackagePathsTest < Minitest::Test + include RailsApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end + + test "#all_paths supports a path wildcard" do + use_template(:skeleton) + package_paths = PackagePaths.new(".", "**") + + assert_includes(package_paths.all_paths, Pathname.new("components/sales/package.yml")) + assert_includes(package_paths.all_paths, Pathname.new("package.yml")) + end + + test "#all_paths supports a single path as a string" do + use_template(:skeleton) + package_paths = PackagePaths.new(".", "components/sales") + + assert_equal(package_paths.all_paths, [Pathname.new("components/sales/package.yml")]) + end + + test "#all_paths supports many paths as an array" do + use_template(:skeleton) + package_paths = PackagePaths.new(".", ["components/sales", "."]) + + assert_equal( + package_paths.all_paths, + [ + Pathname.new("components/sales/package.yml"), + Pathname.new("package.yml"), + ] + ) + end + + test "#all_paths excludes paths inside the gem directory" do + use_template(:skeleton) + vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") + + package_paths = PackagePaths.new(".", "**") + assert_includes(package_paths.all_paths, vendor_package_path) + + Bundler.expects(:bundle_path).once.returns(Rails.root.join("vendor/cache/gems")) + package_paths = PackagePaths.new(".", "**") + refute_includes(package_paths.all_paths, vendor_package_path) + end + + test "#all_paths excludes paths in exclude pathspec" do + use_template(:skeleton) + vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") + + package_paths = PackagePaths.new(".", "**") + assert_includes(package_paths.all_paths, vendor_package_path) + + package_paths = PackagePaths.new(".", "**", "vendor/*") + refute_includes(package_paths.all_paths, vendor_package_path) + end + + test "#all_paths excludes paths in multiple exclude pathspecs" do + use_template(:skeleton) + + vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") + sales_package_path = Pathname.new("components/sales/package.yml") + + package_paths = PackagePaths.new(".", "**") + assert_includes(package_paths.all_paths, vendor_package_path) + assert_includes(package_paths.all_paths, sales_package_path) + + package_paths = PackagePaths.new(".", "**", ["vendor/*", "components/sales/*"]) + refute_includes(package_paths.all_paths, vendor_package_path) + refute_includes(package_paths.all_paths, sales_package_path) + end + + test "#all_paths ignores empty excludes" do + use_template(:skeleton) + + assert_equal( + PackagePaths.new(".", "**").all_paths, + PackagePaths.new(".", "**", []).all_paths + ) + end + end +end diff --git a/test/unit/packwerk/package_set_test.rb b/test/unit/packwerk/package_set_test.rb index 18ff2bc93..386690a86 100644 --- a/test/unit/packwerk/package_set_test.rb +++ b/test/unit/packwerk/package_set_test.rb @@ -39,71 +39,5 @@ class PackageSetTest < Minitest::Test test "#fetch returns nil for unknown package name" do assert_nil(@package_set.fetch("components/unknown")) end - - test ".package_paths supports a path wildcard" do - package_paths = PackageSet.package_paths(".", "**") - - assert_includes(package_paths, Pathname.new("components/sales/package.yml")) - assert_includes(package_paths, Pathname.new("package.yml")) - end - - test ".package_paths supports a single path as a string" do - package_paths = PackageSet.package_paths(".", "components/sales") - - assert_equal(package_paths, [Pathname.new("components/sales/package.yml")]) - end - - test ".package_paths supports many paths as an array" do - package_paths = PackageSet.package_paths(".", ["components/sales", "."]) - - assert_equal( - package_paths, - [ - Pathname.new("components/sales/package.yml"), - Pathname.new("package.yml"), - ] - ) - end - - test ".package_paths excludes paths inside the gem directory" do - vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") - - package_paths = PackageSet.package_paths(".", "**") - assert_includes(package_paths, vendor_package_path) - - Bundler.expects(:bundle_path).returns(Rails.root.join("vendor/cache/gems")) - package_paths = PackageSet.package_paths(".", "**") - refute_includes(package_paths, vendor_package_path) - end - - test ".package_paths excludes paths in exclude pathspec" do - vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") - - package_paths = PackageSet.package_paths(".", "**") - assert_includes(package_paths, vendor_package_path) - - package_paths = PackageSet.package_paths(".", "**", "vendor/*") - refute_includes(package_paths, vendor_package_path) - end - - test ".package_paths excludes paths in multiple exclude pathspecs" do - vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") - sales_package_path = Pathname.new("components/sales/package.yml") - - package_paths = PackageSet.package_paths(".", "**") - assert_includes(package_paths, vendor_package_path) - assert_includes(package_paths, sales_package_path) - - package_paths = PackageSet.package_paths(".", "**", ["vendor/*", "components/sales/*"]) - refute_includes(package_paths, vendor_package_path) - refute_includes(package_paths, sales_package_path) - end - - test ".package_paths ignores empty excludes" do - assert_equal( - PackageSet.package_paths(".", "**"), - PackageSet.package_paths(".", "**", []) - ) - end end end From e89cc4dba79c4591774137c871737db0e9180741 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Wed, 27 Jul 2022 22:44:31 -0400 Subject: [PATCH 2/8] Allow packwerk to scan for packages inside of engines This commit modifies packwerk so that it can scan for packages defined in any engines loaded in a given rails application. This allows support for more complex repository structures where the rails app is not at the root of the repository, and where that app depends on engines defined in sibiling directories. To support this, I modified `PackageSet` so it also looks at `Rails.application.railties` to pull a list of engines, and their root directories, and include those in the paths scanned for packages. I also dropped the restrictions on load paths for constant resolution in `ApplicationLoadPaths`. The result is that packwerk can now find packages defined in these engines, and resolve constants in these engines. With this setup, packwerk still only scans files in the actual app and it's subdirectories for violations, but now it can resolve constants in the engines, and attach them to packages in the engine. For testing, I created a new fixture which models this sort of directory structure, and mocks an app with a rails engine. --- lib/packwerk/package_paths.rb | 21 ++++++++++- lib/packwerk/package_set.rb | 7 ++-- lib/packwerk/rails_load_paths.rb | 2 - lib/packwerk/run_context.rb | 4 +- .../app/models/concerns/has_timeline.rb | 5 +++ .../timeline/app/models/imaginary/.gitkeep | 0 .../timeline/app/models/private_thing.rb | 2 + .../timeline/app/models/sales/.gitkeep | 0 .../components/timeline/nested/package.yml | 0 .../app/components/timeline/package.yml | 2 + .../app/config/environment.rb | 0 .../external_packages/app/package.yml | 0 .../external_packages/app/packwerk.yml | 1 + .../components/sales/app/models/order.rb | 5 +++ .../sales/app/models/sales/order.rb | 7 ++++ .../app/public/sales/record_new_order.rb | 7 ++++ .../sales/components/sales/package.yml | 8 ++++ .../custom_executable_integration_test.rb | 20 +++++++++- .../packwerk/application_fixture_helper.rb | 4 ++ .../rails_application_fixture_helper.rb | 23 +++++++++++- test/unit/packwerk/package_set_test.rb | 37 +++++++++++++++---- test/unit/packwerk/rails_load_paths_test.rb | 15 -------- 22 files changed, 137 insertions(+), 33 deletions(-) create mode 100644 test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb create mode 100644 test/fixtures/external_packages/app/components/timeline/app/models/imaginary/.gitkeep create mode 100644 test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb create mode 100644 test/fixtures/external_packages/app/components/timeline/app/models/sales/.gitkeep create mode 100644 test/fixtures/external_packages/app/components/timeline/nested/package.yml create mode 100644 test/fixtures/external_packages/app/components/timeline/package.yml create mode 100644 test/fixtures/external_packages/app/config/environment.rb create mode 100644 test/fixtures/external_packages/app/package.yml create mode 100644 test/fixtures/external_packages/app/packwerk.yml create mode 100644 test/fixtures/external_packages/sales/components/sales/app/models/order.rb create mode 100644 test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb create mode 100644 test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb create mode 100644 test/fixtures/external_packages/sales/components/sales/package.yml diff --git a/lib/packwerk/package_paths.rb b/lib/packwerk/package_paths.rb index 791125db6..e48703d92 100644 --- a/lib/packwerk/package_paths.rb +++ b/lib/packwerk/package_paths.rb @@ -17,12 +17,14 @@ class PackagePaths root_path: String, package_pathspec: T.nilable(PathSpec), exclude_pathspec: T.nilable(PathSpec), + scan_for_packages_outside_of_app_dir: T.nilable(T::Boolean) ).void end - def initialize(root_path, package_pathspec, exclude_pathspec = nil) + def initialize(root_path, package_pathspec, exclude_pathspec = nil, scan_for_packages_outside_of_app_dir = false) @root_path = root_path @package_pathspec = package_pathspec @exclude_pathspec = exclude_pathspec + @scan_for_packages_outside_of_app_dir = scan_for_packages_outside_of_app_dir end def all_paths @@ -30,7 +32,11 @@ def all_paths .push(Bundler.bundle_path.join("**").to_s) .map { |glob| File.expand_path(glob) } - paths_to_scan = [@root_path] + paths_to_scan = if @scan_for_packages_outside_of_app_dir + engine_paths_to_scan.push(@root_path) + else + [@root_path] + end glob_patterns = paths_to_scan.product(Array(@package_pathspec)).map do |path, pathspec| File.join(path, pathspec, PACKAGE_CONFIG_FILENAME) @@ -49,5 +55,16 @@ def exclude_path?(globs, path) path.realpath.fnmatch(glob, File::FNM_EXTGLOB) end end + + sig { returns(T::Array[String]) } + def engine_paths_to_scan + bundle_path_match = Bundler.bundle_path.join("**") + + Rails.application.railties + .select { |r| r.is_a?(Rails::Engine) } + .map { |r| Pathname.new(r.root).expand_path } + .reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems + .map(&:to_s) + end end end diff --git a/lib/packwerk/package_set.rb b/lib/packwerk/package_set.rb index a64386652..1a3a822a4 100644 --- a/lib/packwerk/package_set.rb +++ b/lib/packwerk/package_set.rb @@ -19,10 +19,11 @@ class << self extend T::Sig sig do - params(root_path: String, package_pathspec: T.nilable(PackagePaths::PathSpec)).returns(PackageSet) + params(root_path: String, package_pathspec: T.nilable(PackagePaths::PathSpec), + scan_for_packages_outside_of_app_dir: T.nilable(T::Boolean)).returns(PackageSet) end - def load_all_from(root_path, package_pathspec: nil) - package_paths = PackagePaths.new(root_path, package_pathspec || "**", nil) + def load_all_from(root_path, package_pathspec: nil, scan_for_packages_outside_of_app_dir: false) + package_paths = PackagePaths.new(root_path, package_pathspec || "**", nil, scan_for_packages_outside_of_app_dir) packages = package_paths.all_paths.map do |path| root_relative = path.dirname.relative_path_from(root_path) diff --git a/lib/packwerk/rails_load_paths.rb b/lib/packwerk/rails_load_paths.rb index c2b2336e3..b7189c248 100644 --- a/lib/packwerk/rails_load_paths.rb +++ b/lib/packwerk/rails_load_paths.rb @@ -35,11 +35,9 @@ def extract_application_autoload_paths end def filter_relevant_paths(all_paths, bundle_path: Bundler.bundle_path, rails_root: Rails.root) bundle_path_match = bundle_path.join("**") - rails_root_match = rails_root.join("**") all_paths .transform_keys { |path| Pathname.new(path).expand_path } - .select { |path| path.fnmatch(rails_root_match.to_s) } # path needs to be in application directory .reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems end diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 4549b4241..0d4ffd9f6 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -21,6 +21,7 @@ def from_configuration(configuration) root_path: configuration.root_path, load_paths: configuration.load_paths, package_paths: configuration.package_paths, + packages_outside_of_app_dir_enabled: configuration.packages_outside_of_app_dir_enabled, inflector: inflector, custom_associations: configuration.custom_associations, cache_enabled: configuration.cache_enabled?, @@ -42,6 +43,7 @@ def from_configuration(configuration) custom_associations: AssociationInspector::CustomAssociations, checkers: T::Array[Checker], cache_enabled: T::Boolean, + packages_outside_of_app_dir_enabled: T.nilable(T::Boolean), ).void end def initialize( @@ -129,7 +131,7 @@ def resolver inflector: @inflector, ) end - + sig { returns(T::Array[ConstantNameInspector]) } def constant_name_inspectors [ diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb b/test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb new file mode 100644 index 000000000..92461a137 --- /dev/null +++ b/test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb @@ -0,0 +1,5 @@ +# typed: ignore +# frozen_string_literal: true + +module HasTimeline +end diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/imaginary/.gitkeep b/test/fixtures/external_packages/app/components/timeline/app/models/imaginary/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb b/test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb new file mode 100644 index 000000000..6ae09c969 --- /dev/null +++ b/test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb @@ -0,0 +1,2 @@ +class PrivateThing +end diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/sales/.gitkeep b/test/fixtures/external_packages/app/components/timeline/app/models/sales/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/external_packages/app/components/timeline/nested/package.yml b/test/fixtures/external_packages/app/components/timeline/nested/package.yml new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/external_packages/app/components/timeline/package.yml b/test/fixtures/external_packages/app/components/timeline/package.yml new file mode 100644 index 000000000..46f5ff725 --- /dev/null +++ b/test/fixtures/external_packages/app/components/timeline/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: + - "::PrivateThing" diff --git a/test/fixtures/external_packages/app/config/environment.rb b/test/fixtures/external_packages/app/config/environment.rb new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/external_packages/app/package.yml b/test/fixtures/external_packages/app/package.yml new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/external_packages/app/packwerk.yml b/test/fixtures/external_packages/app/packwerk.yml new file mode 100644 index 000000000..dca3889ac --- /dev/null +++ b/test/fixtures/external_packages/app/packwerk.yml @@ -0,0 +1 @@ +packages_outside_of_app_dir_enabled: true \ No newline at end of file diff --git a/test/fixtures/external_packages/sales/components/sales/app/models/order.rb b/test/fixtures/external_packages/sales/components/sales/app/models/order.rb new file mode 100644 index 000000000..627a03838 --- /dev/null +++ b/test/fixtures/external_packages/sales/components/sales/app/models/order.rb @@ -0,0 +1,5 @@ +# typed: ignore +# frozen_string_literal: true + +class Order +end diff --git a/test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb b/test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb new file mode 100644 index 000000000..125b83364 --- /dev/null +++ b/test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb @@ -0,0 +1,7 @@ +# typed: ignore +# frozen_string_literal: true + +module Sales + class Order + end +end diff --git a/test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb b/test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb new file mode 100644 index 000000000..ed72bc35b --- /dev/null +++ b/test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb @@ -0,0 +1,7 @@ +# typed: ignore +# frozen_string_literal: true + +module Sales + module RecordNewOrder + end +end diff --git a/test/fixtures/external_packages/sales/components/sales/package.yml b/test/fixtures/external_packages/sales/components/sales/package.yml new file mode 100644 index 000000000..6c9ff1ee9 --- /dev/null +++ b/test/fixtures/external_packages/sales/components/sales/package.yml @@ -0,0 +1,8 @@ +--- +enforce_privacy: true + +metadata: + stewards: + - "@Shopify/sales" + slack_channels: + - "#sales" diff --git a/test/integration/packwerk/custom_executable_integration_test.rb b/test/integration/packwerk/custom_executable_integration_test.rb index 2289a46fd..efb626648 100644 --- a/test/integration/packwerk/custom_executable_integration_test.rb +++ b/test/integration/packwerk/custom_executable_integration_test.rb @@ -13,7 +13,6 @@ class CustomExecutableIntegrationTest < Minitest::Test setup do reset_output setup_application_fixture - use_template(:skeleton) end teardown do @@ -21,6 +20,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk check' with no violations succeeds in all variants" do + use_template(:skeleton) assert_successful_run("check") assert_match(/No offenses detected/, captured_output) @@ -34,6 +34,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk check' with violations only in nested packages has different outcomes per variant" do + use_template(:skeleton) open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") file.flush @@ -54,6 +55,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do + use_template(:skeleton) open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") file.flush @@ -73,6 +75,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk update-todo' with no violations succeeds and updates no files" do + use_template(:skeleton) package_todo_content = read_package_todo assert_successful_run("update-todo") @@ -93,6 +96,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk update-todo' with violations succeeds and updates relevant package_todo" do + use_template(:skeleton) package_todo_content = read_package_todo timeline_package_todo_path = to_app_path(File.join(TIMELINE_PATH, "package_todo.yml")) @@ -129,6 +133,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end test "'packwerk check' does not blow up when parsing files with syntax issues from a false positive association" do + use_template(:skeleton) open_app_file(TIMELINE_PATH.join("app", "models", "bad_file.rb")) do |file| # This is an example of a file that has an object called `belongs_to` that accepts methods content = <<~CONTENT @@ -150,6 +155,19 @@ class CustomExecutableIntegrationTest < Minitest::Test assert_match(/No stale violations detected/, captured_output) end end + + test "'packwerk check' for an app with external packages it detects a violation referencing a constant in the external package" do + use_template(:external_packages) + + open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| + file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") + file.flush + end + + refute_successful_run("check") + assert_match(/Privacy violation: '::Order'/, captured_output) + assert_match(/1 offense detected/, captured_output) + end private diff --git a/test/support/packwerk/application_fixture_helper.rb b/test/support/packwerk/application_fixture_helper.rb index bc0f6b96c..3db777a95 100644 --- a/test/support/packwerk/application_fixture_helper.rb +++ b/test/support/packwerk/application_fixture_helper.rb @@ -35,6 +35,10 @@ def use_template(template) raise "use_template may only be called once per test" if using_template? copy_dir("test/fixtures/#{template}") + if template == :external_packages + @app_dir = File.join(app_dir, "./app") + end + Dir.chdir(app_dir) end diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb index 8e5e13857..764b9947a 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -1,4 +1,4 @@ -# typed: true +# typed: false # frozen_string_literal: true require "support/rails_test_helper" @@ -32,16 +32,21 @@ def use_template(template) Rails.stubs(:autoloaders).returns(Autoloaders.new) + root = Pathname.new(app_dir) + case template when :minimal set_load_paths_for_minimal_template when :skeleton set_load_paths_for_skeleton_template + when :external_packages + set_load_paths_for_external_packages_template + create_new_engine_at_path(*to_app_paths("../sales/components/sales/")) + Rails.application.stubs(:railties).returns(Rails::Engine::Railties.new) else raise "Unknown fixture template #{template}" end - root = Pathname.new(app_dir) Rails.application.config.stubs(:root).returns(root) end @@ -59,4 +64,18 @@ def set_load_paths_for_skeleton_template Rails.autoloaders.once.push_dir(*to_app_paths("components/timeline/app/models/concerns")) Rails.autoloaders.once.push_dir(*to_app_paths("vendor/cache/gems/example/models")) end + + def set_load_paths_for_external_packages_template + Rails.autoloaders.main.push_dir(*to_app_paths("/components/timeline/app/models")) + + Rails.autoloaders.once.push_dir(*to_app_paths("../sales/components/sales/app/models")) + end + + def create_new_engine_at_path(path) + Class.new(Rails::Engine) do + define_method(:root) do + path + end + end + end end diff --git a/test/unit/packwerk/package_set_test.rb b/test/unit/packwerk/package_set_test.rb index 386690a86..da5d8ef50 100644 --- a/test/unit/packwerk/package_set_test.rb +++ b/test/unit/packwerk/package_set_test.rb @@ -9,8 +9,6 @@ class PackageSetTest < Minitest::Test setup do setup_application_fixture - use_template(:skeleton) - @package_set = PackageSet.load_all_from(app_dir) end teardown do @@ -18,26 +16,51 @@ class PackageSetTest < Minitest::Test end test "#package_from_path returns package instance for a known path" do - assert_equal("components/timeline", @package_set.package_from_path("components/timeline/something.rb").name) + use_template(:skeleton) + package_set = PackageSet.load_all_from(app_dir) + + assert_equal("components/timeline", package_set.package_from_path("components/timeline/something.rb").name) end test "#package_from_path returns root package for an unpackaged path" do - assert_equal(".", @package_set.package_from_path("components/unknown/something.rb").name) + use_template(:skeleton) + package_set = PackageSet.load_all_from(app_dir) + + assert_equal(".", package_set.package_from_path("components/unknown/something.rb").name) end test "#package_from_path returns nested packages" do + use_template(:skeleton) + package_set = PackageSet.load_all_from(app_dir) + assert_equal( "components/timeline/nested", - @package_set.package_from_path("components/timeline/nested/something.rb").name + package_set.package_from_path("components/timeline/nested/something.rb").name + ) + end + + test "#package_from_path returns a package from an external package in autoload paths" do + use_template(:external_packages) + + package_set = PackageSet.load_all_from(app_dir, scan_for_packages_outside_of_app_dir: true) + assert_equal( + package_set.package_from_path("../sales/components/sales/app/models/order.rb").name, + "../sales/components/sales" ) end test "#fetch returns a package instance for known package name" do - assert_equal("components/timeline", @package_set.fetch("components/timeline").name) + use_template(:skeleton) + package_set = PackageSet.load_all_from(app_dir) + + assert_equal("components/timeline", T.must(package_set.fetch("components/timeline")).name) end test "#fetch returns nil for unknown package name" do - assert_nil(@package_set.fetch("components/unknown")) + use_template(:skeleton) + package_set = PackageSet.load_all_from(app_dir) + + assert_nil(package_set.fetch("components/unknown")) end end end diff --git a/test/unit/packwerk/rails_load_paths_test.rb b/test/unit/packwerk/rails_load_paths_test.rb index c3e5841b3..e781e22ba 100644 --- a/test/unit/packwerk/rails_load_paths_test.rb +++ b/test/unit/packwerk/rails_load_paths_test.rb @@ -17,21 +17,6 @@ class RailsLoadPathsTest < Minitest::Test assert_equal({ relative_path => Object }, relative_path_strings) end - test ".for excludes paths outside of the application root" do - RailsLoadPaths.expects(:require_application).with("/application/", "test").once.returns(true) - rails_root = Pathname.new("/application/") - Rails.expects(:root).twice.returns(rails_root) - Bundler.expects(:bundle_path).once.returns(Pathname.new("/application/vendor/")) - - valid_paths = ["/application/app/models"] - paths = valid_paths + ["/users/tobi/.gems/something/app/models", "/application/../something/"] - paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object } - RailsLoadPaths.expects(:extract_application_autoload_paths).once.returns(paths) - filtered_path_strings = RailsLoadPaths.for(rails_root.to_s, environment: "test") - - assert_equal({ "app/models" => Object }, filtered_path_strings.transform_keys(&:to_s)) - end - test ".for excludes paths from vendored gems" do RailsLoadPaths.expects(:require_application).with("/application/", "test").once.returns(true) rails_root = Pathname.new("/application/") From e64ce0e0b31d80dc202eadea4b1081eed975399c Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Tue, 16 Aug 2022 20:14:57 -0400 Subject: [PATCH 3/8] Address code review feedback Fixed some typing issues by using unsafe where necessary --- lib/packwerk/package_paths.rb | 1 + test/support/rails_application_fixture_helper.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/packwerk/package_paths.rb b/lib/packwerk/package_paths.rb index e48703d92..59cb80d8f 100644 --- a/lib/packwerk/package_paths.rb +++ b/lib/packwerk/package_paths.rb @@ -27,6 +27,7 @@ def initialize(root_path, package_pathspec, exclude_pathspec = nil, scan_for_pac @scan_for_packages_outside_of_app_dir = scan_for_packages_outside_of_app_dir end + sig {returns(T::Array[Pathname])} def all_paths exclude_pathspec = Array(@exclude_pathspec).dup .push(Bundler.bundle_path.join("**").to_s) diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb index 764b9947a..a92322165 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -1,4 +1,4 @@ -# typed: false +# typed: true # frozen_string_literal: true require "support/rails_test_helper" @@ -73,7 +73,7 @@ def set_load_paths_for_external_packages_template def create_new_engine_at_path(path) Class.new(Rails::Engine) do - define_method(:root) do + T.unsafe(self).define_method(:root) do path end end From ca16a5fce606494d466a699e9488cee6141f3130 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Tue, 16 Aug 2022 20:29:51 -0400 Subject: [PATCH 4/8] Updating docs --- USAGE.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/USAGE.md b/USAGE.md index 4d77b052d..6c26de3a6 100644 --- a/USAGE.md +++ b/USAGE.md @@ -72,15 +72,16 @@ Secondly, to enable Spring, first run `bin/spring binstub packwerk` which will " Packwerk reads from the `packwerk.yml` configuration file in the root directory. Packwerk will run with the default configuration if any of these settings are not specified. -| Key | Default value | Description | -|----------------------|-------------------------------------------|--------------| -| include | **/*.{rb,rake,erb} | list of patterns for folder paths to include | -| exclude | {bin,node_modules,script,tmp,vendor}/**/* | list of patterns for folder paths to exclude | -| package_paths | **/ | a single pattern or a list of patterns to find package configuration files, see: [Defining packages](#Defining-packages) | -| custom_associations | N/A | list of custom associations, if any | -| parallel | true | when true, fork code parsing out to subprocesses | -| cache | false | when true, caches the results of parsing files | -| cache_directory | tmp/cache/packwerk | the directory that will hold the packwerk cache | +| Key | Default value | Description | +|-------------------------------------|-------------------------------------------|--------------------------------------------------------------------------------------------------------------------------| +| include | **/*.{rb,rake,erb} | list of patterns for folder paths to include | +| exclude | {bin,node_modules,script,tmp,vendor}/**/* | list of patterns for folder paths to exclude | +| package_paths | **/ | a single pattern or a list of patterns to find package configuration files, see: [Defining packages](#Defining-packages) | +| custom_associations | N/A | list of custom associations, if any | +| parallel | true | when true, fork code parsing out to subprocesses | +| cache | false | when true, caches the results of parsing files | +| cache_directory | tmp/cache/packwerk | the directory that will hold the packwerk cache | +| packages_outside_of_app_dir_enabled | false | when true, packwerk will scan for packages in folders for locally published rails engines | ### Using a custom ERB parser From c3eae4ade64a3732e0f43b7439438490864e8b2f Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Wed, 19 Oct 2022 20:57:07 -0400 Subject: [PATCH 5/8] Text change to make CI do it's thing --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 8cc93fea1..50491828b 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,8 @@ To reduce the impact of those limitations, Packwerk is designed to avoid false p - Method calls and objects passed around the application are completely ignored. Packwerk only cares about static constant references. That said, if you want Packwerk to analyze parameters of a method, you can use [Sorbet](https://sorbet.org/) to define a type signature. Sorbet signatures are pure Ruby code and use constants to express types, and Packwerk understands that. - Support for custom Zeitwerk configuration is limited. If [custom ActiveSupport inflections](https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#customizing-inflections) are used, Packwerk will understand that and everything is fine. However, if Zeitwerk is directly configured with [custom Zeitwerk inflections](https://github.com/fxn/zeitwerk#inflection) or to [collapse directories](https://github.com/fxn/zeitwerk#collapsing-directories), _Packwerk will get confused and produce false positives_. +TODO: Remove this + ## Contributing Bug reports and pull requests are welcome on GitHub at https://github.com/Shopify/packwerk. From 9baa416ae9591ecf61afb6134f83e79cd0558f39 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Wed, 19 Oct 2022 21:24:29 -0400 Subject: [PATCH 6/8] Small sorbet fix --- README.md | 2 -- test/support/rails_application_fixture_helper.rb | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 50491828b..8cc93fea1 100644 --- a/README.md +++ b/README.md @@ -79,8 +79,6 @@ To reduce the impact of those limitations, Packwerk is designed to avoid false p - Method calls and objects passed around the application are completely ignored. Packwerk only cares about static constant references. That said, if you want Packwerk to analyze parameters of a method, you can use [Sorbet](https://sorbet.org/) to define a type signature. Sorbet signatures are pure Ruby code and use constants to express types, and Packwerk understands that. - Support for custom Zeitwerk configuration is limited. If [custom ActiveSupport inflections](https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#customizing-inflections) are used, Packwerk will understand that and everything is fine. However, if Zeitwerk is directly configured with [custom Zeitwerk inflections](https://github.com/fxn/zeitwerk#inflection) or to [collapse directories](https://github.com/fxn/zeitwerk#collapsing-directories), _Packwerk will get confused and produce false positives_. -TODO: Remove this - ## Contributing Bug reports and pull requests are welcome on GitHub at https://github.com/Shopify/packwerk. diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb index a92322165..671f7f1e5 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -73,7 +73,8 @@ def set_load_paths_for_external_packages_template def create_new_engine_at_path(path) Class.new(Rails::Engine) do - T.unsafe(self).define_method(:root) do + T.bind(self, Class) + define_method(:root) do path end end From ab1a847c7918450a716b56e922bcefe7b2a33495 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Wed, 8 Nov 2023 11:29:11 -0500 Subject: [PATCH 7/8] Fix stuff after rebasing --- lib/packwerk/package_set.rb | 1 + lib/packwerk/run_context.rb | 8 +- lib/packwerk/validator.rb | 6 +- .../components/timeline/nested/package.yml | 1 + .../custom_executable_integration_test.rb | 276 +++++++++--------- test/unit/application_validator_test.rb | 201 ------------- .../packwerk/application_validator_test.rb | 2 +- 7 files changed, 150 insertions(+), 345 deletions(-) delete mode 100644 test/unit/application_validator_test.rb diff --git a/lib/packwerk/package_set.rb b/lib/packwerk/package_set.rb index 1a3a822a4..930e196c1 100644 --- a/lib/packwerk/package_set.rb +++ b/lib/packwerk/package_set.rb @@ -14,6 +14,7 @@ class PackageSet Elem = type_member { { fixed: Package } } + PACKAGE_CONFIG_FILENAME = "package.yml" class << self extend T::Sig diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 0d4ffd9f6..1d4a7b7dd 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -27,7 +27,6 @@ def from_configuration(configuration) cache_enabled: configuration.cache_enabled?, cache_directory: configuration.cache_directory, config_path: configuration.config_path, - packages_outside_of_app_dir_enabled: configuration.packages_outside_of_app_dir_enabled, ) end end @@ -81,10 +80,13 @@ def initialize( def process_file(relative_file:) processed_file = file_processor.call(relative_file) + puts relative_file + references = ReferenceExtractor.get_fully_qualified_references_from( processed_file.unresolved_references, context_provider ) + puts references reference_checker = ReferenceChecking::ReferenceChecker.new(@checkers) processed_file.offenses + references.flat_map { |reference| reference_checker.call(reference) } @@ -96,7 +98,9 @@ def package_set @root_path, package_pathspec: @package_paths, scan_for_packages_outside_of_app_dir: @packages_outside_of_app_dir_enabled, - ) + ) + puts @package_set.packages + @package_set end private diff --git a/lib/packwerk/validator.rb b/lib/packwerk/validator.rb index 8edcd27fc..8f08ac05f 100644 --- a/lib/packwerk/validator.rb +++ b/lib/packwerk/validator.rb @@ -62,10 +62,10 @@ def package_manifests_settings_for(configuration, setting) def package_manifests(configuration, glob_pattern = nil) glob_pattern ||= package_glob(configuration) package_paths = PackagePaths.new( - @configuration.root_path, + configuration.root_path, glob_pattern, - @configuration.exclude, - @configuration.packages_outside_of_app_dir_enabled + configuration.exclude, + configuration.packages_outside_of_app_dir_enabled ) package_paths.all_paths.map { |f| File.realpath(f) } end diff --git a/test/fixtures/external_packages/app/components/timeline/nested/package.yml b/test/fixtures/external_packages/app/components/timeline/nested/package.yml index e69de29bb..c0ce986be 100644 --- a/test/fixtures/external_packages/app/components/timeline/nested/package.yml +++ b/test/fixtures/external_packages/app/components/timeline/nested/package.yml @@ -0,0 +1 @@ +enforce_dependencies: true \ No newline at end of file diff --git a/test/integration/packwerk/custom_executable_integration_test.rb b/test/integration/packwerk/custom_executable_integration_test.rb index efb626648..ce7f27bb5 100644 --- a/test/integration/packwerk/custom_executable_integration_test.rb +++ b/test/integration/packwerk/custom_executable_integration_test.rb @@ -6,7 +6,7 @@ module Packwerk class CustomExecutableIntegrationTest < Minitest::Test - include ApplicationFixtureHelper + include RailsApplicationFixtureHelper TIMELINE_PATH = Pathname.new("components/timeline") @@ -19,143 +19,143 @@ class CustomExecutableIntegrationTest < Minitest::Test teardown_application_fixture end - test "'packwerk check' with no violations succeeds in all variants" do - use_template(:skeleton) - assert_successful_run("check") - assert_match(/No offenses detected/, captured_output) + # test "'packwerk check' with no violations succeeds in all variants" do + # use_template(:skeleton) + # assert_successful_run("check") + # assert_match(/No offenses detected/, captured_output) + + # reset_output + # assert_successful_run(["check", "components/timeline"]) + # assert_match(/No offenses detected/, captured_output) + + # reset_output + # assert_successful_run(["check", "--packages=components/timeline"]) + # assert_match(/No offenses detected/, captured_output) + # end + + # test "'packwerk check' with violations only in nested packages has different outcomes per variant" do + # use_template(:skeleton) + # open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| + # file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") + # file.flush + # end + + # refute_successful_run("check") + # assert_match(/Dependency violation: ::Order/, captured_output) + # assert_match(/1 offense detected/, captured_output) + + # reset_output + # refute_successful_run(["check", "components/timeline"]) + # assert_match(/Dependency violation: ::Order/, captured_output) + # assert_match(/1 offense detected/, captured_output) + + # reset_output + # assert_successful_run(["check", "--packages=components/timeline"]) + # assert_match(/No offenses detected/, captured_output) + # end + + # test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do + # use_template(:skeleton) + # open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| + # file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") + # file.flush + # end + + # refute_successful_run("check") + # assert_match(/Dependency violation: ::Order/, captured_output) + # assert_match(/1 offense detected/, captured_output) + + # reset_output + # assert_successful_run(["check", "components/sales"]) + # assert_match(/No offenses detected/, captured_output) + + # reset_output + # assert_successful_run(["check", "--packages=components/sales"]) + # assert_match(/No offenses detected/, captured_output) + # end + + # test "'packwerk update-todo' with no violations succeeds and updates no files" do + # use_template(:skeleton) + # package_todo_content = read_package_todo + + # assert_successful_run("update-todo") + + # package_todo_content_after_update = read_package_todo + # expected_output = <<~EOS + # 📦 Packwerk is inspecting 12 files + # \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. + # 📦 Finished in \\d+\\.\\d+ seconds + + # No offenses detected + # ✅ `package_todo.yml` has been updated. + # EOS + + # assert_equal(package_todo_content, package_todo_content_after_update, + # "expected no updates to any package todo file") + # assert_match(/#{expected_output}/, captured_output) + # end + + # test "'packwerk update-todo' with violations succeeds and updates relevant package_todo" do + # use_template(:skeleton) + # package_todo_content = read_package_todo + # timeline_package_todo_path = to_app_path(File.join(TIMELINE_PATH, "package_todo.yml")) + + # open_app_file(TIMELINE_PATH.join("app", "models", "timeline_comment.rb")) do |file| + # file.write("class TimelineComment; belongs_to :order; end") + # file.flush + + # assert_successful_run("update-todo") + + # assert(File.exist?(timeline_package_todo_path), + # "expected new package_todo for timeline package to be created") + + # timeline_package_todo_content = File.read(timeline_package_todo_path) + # assert_match( + # "components/sales:\n \"::Order\":\n violations:\n - dependency", + # timeline_package_todo_content + # ) + + # package_todo_content_after_update = + # read_package_todo.reject { |k, _v| k.match?(timeline_package_todo_path) } + # expected_output = <<~EOS + # 📦 Packwerk is inspecting 13 files + # \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. + # 📦 Finished in \\d+\\.\\d+ seconds + + # No offenses detected + # ✅ `package_todo.yml` has been updated. + # EOS + + # assert_equal(package_todo_content, package_todo_content_after_update, + # "expected no updates to any package todo files besides timeline/package_todo.yml") + # assert_match(/#{expected_output}/, captured_output) + # end + # end + + # test "'packwerk check' does not blow up when parsing files with syntax issues from a false positive association" do + # use_template(:skeleton) + # open_app_file(TIMELINE_PATH.join("app", "models", "bad_file.rb")) do |file| + # # This is an example of a file that has an object called `belongs_to` that accepts methods + # content = <<~CONTENT + # belongs_to.some_method + # CONTENT + + # file.write(content) + # file.flush + + # refute_successful_run("check") + + # assert_match(/Packwerk is inspecting 13 files/, captured_output) + # assert_match(%r{components/timeline/app/models/bad_file.rb}, captured_output) + # assert_match(/Packwerk encountered an internal error/, captured_output) + # assert_match(/For now, you can add this file to `packwerk.yml` `exclude` list./, captured_output) + # assert_match(/Please file an issue and include this error message and stacktrace:/, captured_output) + # assert_match(/Passed `nil` into T.must/, captured_output) + # assert_match(/1 offense detected/, captured_output) + # assert_match(/No stale violations detected/, captured_output) + # end + # end - reset_output - assert_successful_run(["check", "components/timeline"]) - assert_match(/No offenses detected/, captured_output) - - reset_output - assert_successful_run(["check", "--packages=components/timeline"]) - assert_match(/No offenses detected/, captured_output) - end - - test "'packwerk check' with violations only in nested packages has different outcomes per variant" do - use_template(:skeleton) - open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| - file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") - file.flush - end - - refute_successful_run("check") - assert_match(/Dependency violation: ::Order/, captured_output) - assert_match(/1 offense detected/, captured_output) - - reset_output - refute_successful_run(["check", "components/timeline"]) - assert_match(/Dependency violation: ::Order/, captured_output) - assert_match(/1 offense detected/, captured_output) - - reset_output - assert_successful_run(["check", "--packages=components/timeline"]) - assert_match(/No offenses detected/, captured_output) - end - - test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do - use_template(:skeleton) - open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| - file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") - file.flush - end - - refute_successful_run("check") - assert_match(/Dependency violation: ::Order/, captured_output) - assert_match(/1 offense detected/, captured_output) - - reset_output - assert_successful_run(["check", "components/sales"]) - assert_match(/No offenses detected/, captured_output) - - reset_output - assert_successful_run(["check", "--packages=components/sales"]) - assert_match(/No offenses detected/, captured_output) - end - - test "'packwerk update-todo' with no violations succeeds and updates no files" do - use_template(:skeleton) - package_todo_content = read_package_todo - - assert_successful_run("update-todo") - - package_todo_content_after_update = read_package_todo - expected_output = <<~EOS - 📦 Packwerk is inspecting 12 files - \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. - 📦 Finished in \\d+\\.\\d+ seconds - - No offenses detected - ✅ `package_todo.yml` has been updated. - EOS - - assert_equal(package_todo_content, package_todo_content_after_update, - "expected no updates to any package todo file") - assert_match(/#{expected_output}/, captured_output) - end - - test "'packwerk update-todo' with violations succeeds and updates relevant package_todo" do - use_template(:skeleton) - package_todo_content = read_package_todo - timeline_package_todo_path = to_app_path(File.join(TIMELINE_PATH, "package_todo.yml")) - - open_app_file(TIMELINE_PATH.join("app", "models", "timeline_comment.rb")) do |file| - file.write("class TimelineComment; belongs_to :order; end") - file.flush - - assert_successful_run("update-todo") - - assert(File.exist?(timeline_package_todo_path), - "expected new package_todo for timeline package to be created") - - timeline_package_todo_content = File.read(timeline_package_todo_path) - assert_match( - "components/sales:\n \"::Order\":\n violations:\n - dependency", - timeline_package_todo_content - ) - - package_todo_content_after_update = - read_package_todo.reject { |k, _v| k.match?(timeline_package_todo_path) } - expected_output = <<~EOS - 📦 Packwerk is inspecting 13 files - \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. - 📦 Finished in \\d+\\.\\d+ seconds - - No offenses detected - ✅ `package_todo.yml` has been updated. - EOS - - assert_equal(package_todo_content, package_todo_content_after_update, - "expected no updates to any package todo files besides timeline/package_todo.yml") - assert_match(/#{expected_output}/, captured_output) - end - end - - test "'packwerk check' does not blow up when parsing files with syntax issues from a false positive association" do - use_template(:skeleton) - open_app_file(TIMELINE_PATH.join("app", "models", "bad_file.rb")) do |file| - # This is an example of a file that has an object called `belongs_to` that accepts methods - content = <<~CONTENT - belongs_to.some_method - CONTENT - - file.write(content) - file.flush - - refute_successful_run("check") - - assert_match(/Packwerk is inspecting 13 files/, captured_output) - assert_match(%r{components/timeline/app/models/bad_file.rb}, captured_output) - assert_match(/Packwerk encountered an internal error/, captured_output) - assert_match(/For now, you can add this file to `packwerk.yml` `exclude` list./, captured_output) - assert_match(/Please file an issue and include this error message and stacktrace:/, captured_output) - assert_match(/Passed `nil` into T.must/, captured_output) - assert_match(/1 offense detected/, captured_output) - assert_match(/No stale violations detected/, captured_output) - end - end - test "'packwerk check' for an app with external packages it detects a violation referencing a constant in the external package" do use_template(:external_packages) @@ -165,7 +165,7 @@ class CustomExecutableIntegrationTest < Minitest::Test end refute_successful_run("check") - assert_match(/Privacy violation: '::Order'/, captured_output) + assert_match(/Dependency violation: ::Order/, captured_output) assert_match(/1 offense detected/, captured_output) end diff --git a/test/unit/application_validator_test.rb b/test/unit/application_validator_test.rb deleted file mode 100644 index 7db5bac5a..000000000 --- a/test/unit/application_validator_test.rb +++ /dev/null @@ -1,201 +0,0 @@ -# typed: true -# frozen_string_literal: true - -require "test_helper" - -# make sure PrivateThing.constantize succeeds to pass the privacy validity check -require "fixtures/skeleton/components/timeline/app/models/private_thing.rb" - -module Packwerk - class ApplicationValidatorTest < Minitest::Test - include RailsApplicationFixtureHelper - - setup do - setup_application_fixture - end - - teardown do - teardown_application_fixture - end - - test "validity" do - use_template(:skeleton) - - result = validator.check_all - - assert result.ok?, result.error_value - end - - test "check_package_manifest_syntax returns an error for unknown package keys" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_correctness" => false }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/Unknown keys/, result.error_value) - end - - test "check_package_manifest_syntax returns an error for invalid enforce_privacy value" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_privacy" => "yes, please." }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/Invalid 'enforce_privacy' option/, result.error_value) - end - - test "check_package_manifest_syntax returns an error for invalid enforce_dependencies value" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_dependencies" => "components/sales" }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/Invalid 'enforce_dependencies' option/, result.error_value) - end - - test "check_package_manifest_syntax returns an error for invalid public_path value" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "public_path" => [] }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/'public_path' option must be a string/, result.error_value) - end - - test "check_package_manifest_syntax returns error for invalid dependencies value" do - use_template(:minimal) - merge_into_app_yaml_file("components/timeline/package.yml", {}) - merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => "components/timeline" }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/Invalid 'dependencies' option/, result.error_value) - end - - test "check_package_manifests_for_privacy returns an error for unresolvable privatized constants" do - use_template(:skeleton) - ConstantResolver.expects(:new).returns(stub("resolver", resolve: nil)) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - /'::PrivateThing', listed in #{to_app_path('components\/timeline\/package.yml')}, could not be resolved/, - result.error_value - ) - assert_match( - /Add a private_thing.rb file/, - result.error_value - ) - end - - test "check_package_manifests_for_privacy returns an error for privatized constants in other packages" do - use_template(:skeleton) - context = ConstantResolver::ConstantContext.new("::PrivateThing", "private_thing.rb") - - ConstantResolver.expects(:new).returns(stub("resolver", resolve: context)) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - %r{'::PrivateThing' is declared as private in the 'components/timeline' package}, - result.error_value - ) - assert_match( - /but appears to be defined\sin the '.' package/, - result.error_value - ) - end - - test "check_package_manifests_for_privacy returns an error for constants without `::` prefix" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_privacy" => ["::PrivateThing", "OtherThing"] }) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - /'OtherThing', listed in the 'enforce_privacy' option in .*package.yml, is invalid./, - result.error_value - ) - assert_match( - /Private constants need to be prefixed with the top-level namespace operator `::`/, - result.error_value - ) - end - - test "check_acyclic_graph returns error when package set contains circular dependencies" do - use_template(:minimal) - merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => ["components/timeline"] }) - merge_into_app_yaml_file("components/timeline/package.yml", { "dependencies" => ["components/sales"] }) - - result = validator.check_acyclic_graph - - refute result.ok? - assert_match(/Expected the package dependency graph to be acyclic/, result.error_value) - assert_match %r{components/sales → components/timeline → components/sales}, result.error_value - end - - test "check_package_manifest_paths returns error when config only declares partial list of packages" do - use_template(:minimal) - merge_into_app_yaml_file("components/timeline/package.yml", {}) - merge_into_app_yaml_file("packwerk.yml", { "package_paths" => ["components/sales", "."] }) - - result = validator.check_package_manifest_paths - - refute result.ok? - assert_match(/Expected package paths for all package.ymls to be specified/, result.error_value) - assert_match %r{manifests:\n\ncomponents/timeline/package.yml$}m, result.error_value - end - - test "check_package_manifest_paths returns no error when vendor/**/* is excluded" do - use_template(:skeleton) - merge_into_app_yaml_file("components/timeline/package.yml", {}) - merge_into_app_yaml_file("packwerk.yml", { "package_paths" => ["components/**/*", "."] }) - merge_into_app_yaml_file("packwerk.yml", { "exclude" => ["vendor/**/*"] }) - - package_paths = PackagePaths.new(".", "**").all_paths - vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") - assert_includes(package_paths, vendor_package_path) - - result = validator.check_package_manifest_paths - - assert result.ok? - refute result.error_value - end - - test "check_valid_package_dependencies returns error when config contains invalid package dependency" do - use_template(:minimal) - merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => ["components/timeline"] }) - - result = validator.check_valid_package_dependencies - - refute result.ok? - assert_match(/These dependencies do not point to valid packages:/, result.error_value) - assert_match %r{\n\ncomponents/sales/package.yml:\n - components/timeline\n\n$}m, result.error_value - end - - test "check_root_package_exists returns error when root directory is missing a package.yml file" do - use_template(:minimal) - remove_app_entry("package.yml") - - result = validator.check_root_package_exists - refute result.ok? - assert_match(/A root package does not exist./, result.error_value) - end - - def validator - @application_validator ||= Packwerk::ApplicationValidator.new( - config_file_path: config.config_path, - configuration: config, - environment: "test" - ) - end - end -end diff --git a/test/unit/packwerk/application_validator_test.rb b/test/unit/packwerk/application_validator_test.rb index 998d4bf9e..27f824c60 100644 --- a/test/unit/packwerk/application_validator_test.rb +++ b/test/unit/packwerk/application_validator_test.rb @@ -42,7 +42,7 @@ class ApplicationValidatorTest < Minitest::Test merge_into_app_yaml_file("packwerk.yml", { "package_paths" => ["components/**/*", "."] }) merge_into_app_yaml_file("packwerk.yml", { "exclude" => ["vendor/**/*"] }) - package_paths = PackageSet.package_paths(".", "**") + package_paths = PackagePaths.new(".", "**").all_paths vendor_package_path = Pathname.new("vendor/cache/gems/example/package.yml") assert_includes(package_paths, vendor_package_path) From 897fa13449ef994540cedde59b1095ee11263bc9 Mon Sep 17 00:00:00 2001 From: Andrew Swerlick Date: Wed, 8 Nov 2023 11:30:15 -0500 Subject: [PATCH 8/8] add back tests --- .../custom_executable_integration_test.rb | 272 +++++++++--------- 1 file changed, 136 insertions(+), 136 deletions(-) diff --git a/test/integration/packwerk/custom_executable_integration_test.rb b/test/integration/packwerk/custom_executable_integration_test.rb index ce7f27bb5..bcb1c4a02 100644 --- a/test/integration/packwerk/custom_executable_integration_test.rb +++ b/test/integration/packwerk/custom_executable_integration_test.rb @@ -19,142 +19,142 @@ class CustomExecutableIntegrationTest < Minitest::Test teardown_application_fixture end - # test "'packwerk check' with no violations succeeds in all variants" do - # use_template(:skeleton) - # assert_successful_run("check") - # assert_match(/No offenses detected/, captured_output) - - # reset_output - # assert_successful_run(["check", "components/timeline"]) - # assert_match(/No offenses detected/, captured_output) - - # reset_output - # assert_successful_run(["check", "--packages=components/timeline"]) - # assert_match(/No offenses detected/, captured_output) - # end - - # test "'packwerk check' with violations only in nested packages has different outcomes per variant" do - # use_template(:skeleton) - # open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| - # file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") - # file.flush - # end - - # refute_successful_run("check") - # assert_match(/Dependency violation: ::Order/, captured_output) - # assert_match(/1 offense detected/, captured_output) - - # reset_output - # refute_successful_run(["check", "components/timeline"]) - # assert_match(/Dependency violation: ::Order/, captured_output) - # assert_match(/1 offense detected/, captured_output) - - # reset_output - # assert_successful_run(["check", "--packages=components/timeline"]) - # assert_match(/No offenses detected/, captured_output) - # end - - # test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do - # use_template(:skeleton) - # open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| - # file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") - # file.flush - # end - - # refute_successful_run("check") - # assert_match(/Dependency violation: ::Order/, captured_output) - # assert_match(/1 offense detected/, captured_output) - - # reset_output - # assert_successful_run(["check", "components/sales"]) - # assert_match(/No offenses detected/, captured_output) - - # reset_output - # assert_successful_run(["check", "--packages=components/sales"]) - # assert_match(/No offenses detected/, captured_output) - # end - - # test "'packwerk update-todo' with no violations succeeds and updates no files" do - # use_template(:skeleton) - # package_todo_content = read_package_todo - - # assert_successful_run("update-todo") - - # package_todo_content_after_update = read_package_todo - # expected_output = <<~EOS - # 📦 Packwerk is inspecting 12 files - # \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. - # 📦 Finished in \\d+\\.\\d+ seconds - - # No offenses detected - # ✅ `package_todo.yml` has been updated. - # EOS - - # assert_equal(package_todo_content, package_todo_content_after_update, - # "expected no updates to any package todo file") - # assert_match(/#{expected_output}/, captured_output) - # end - - # test "'packwerk update-todo' with violations succeeds and updates relevant package_todo" do - # use_template(:skeleton) - # package_todo_content = read_package_todo - # timeline_package_todo_path = to_app_path(File.join(TIMELINE_PATH, "package_todo.yml")) - - # open_app_file(TIMELINE_PATH.join("app", "models", "timeline_comment.rb")) do |file| - # file.write("class TimelineComment; belongs_to :order; end") - # file.flush - - # assert_successful_run("update-todo") - - # assert(File.exist?(timeline_package_todo_path), - # "expected new package_todo for timeline package to be created") - - # timeline_package_todo_content = File.read(timeline_package_todo_path) - # assert_match( - # "components/sales:\n \"::Order\":\n violations:\n - dependency", - # timeline_package_todo_content - # ) - - # package_todo_content_after_update = - # read_package_todo.reject { |k, _v| k.match?(timeline_package_todo_path) } - # expected_output = <<~EOS - # 📦 Packwerk is inspecting 13 files - # \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. - # 📦 Finished in \\d+\\.\\d+ seconds - - # No offenses detected - # ✅ `package_todo.yml` has been updated. - # EOS - - # assert_equal(package_todo_content, package_todo_content_after_update, - # "expected no updates to any package todo files besides timeline/package_todo.yml") - # assert_match(/#{expected_output}/, captured_output) - # end - # end - - # test "'packwerk check' does not blow up when parsing files with syntax issues from a false positive association" do - # use_template(:skeleton) - # open_app_file(TIMELINE_PATH.join("app", "models", "bad_file.rb")) do |file| - # # This is an example of a file that has an object called `belongs_to` that accepts methods - # content = <<~CONTENT - # belongs_to.some_method - # CONTENT - - # file.write(content) - # file.flush - - # refute_successful_run("check") - - # assert_match(/Packwerk is inspecting 13 files/, captured_output) - # assert_match(%r{components/timeline/app/models/bad_file.rb}, captured_output) - # assert_match(/Packwerk encountered an internal error/, captured_output) - # assert_match(/For now, you can add this file to `packwerk.yml` `exclude` list./, captured_output) - # assert_match(/Please file an issue and include this error message and stacktrace:/, captured_output) - # assert_match(/Passed `nil` into T.must/, captured_output) - # assert_match(/1 offense detected/, captured_output) - # assert_match(/No stale violations detected/, captured_output) - # end - # end + test "'packwerk check' with no violations succeeds in all variants" do + use_template(:skeleton) + assert_successful_run("check") + assert_match(/No offenses detected/, captured_output) + + reset_output + assert_successful_run(["check", "components/timeline"]) + assert_match(/No offenses detected/, captured_output) + + reset_output + assert_successful_run(["check", "--packages=components/timeline"]) + assert_match(/No offenses detected/, captured_output) + end + + test "'packwerk check' with violations only in nested packages has different outcomes per variant" do + use_template(:skeleton) + open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| + file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") + file.flush + end + + refute_successful_run("check") + assert_match(/Dependency violation: ::Order/, captured_output) + assert_match(/1 offense detected/, captured_output) + + reset_output + refute_successful_run(["check", "components/timeline"]) + assert_match(/Dependency violation: ::Order/, captured_output) + assert_match(/1 offense detected/, captured_output) + + reset_output + assert_successful_run(["check", "--packages=components/timeline"]) + assert_match(/No offenses detected/, captured_output) + end + + test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do + use_template(:skeleton) + open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file| + file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end") + file.flush + end + + refute_successful_run("check") + assert_match(/Dependency violation: ::Order/, captured_output) + assert_match(/1 offense detected/, captured_output) + + reset_output + assert_successful_run(["check", "components/sales"]) + assert_match(/No offenses detected/, captured_output) + + reset_output + assert_successful_run(["check", "--packages=components/sales"]) + assert_match(/No offenses detected/, captured_output) + end + + test "'packwerk update-todo' with no violations succeeds and updates no files" do + use_template(:skeleton) + package_todo_content = read_package_todo + + assert_successful_run("update-todo") + + package_todo_content_after_update = read_package_todo + expected_output = <<~EOS + 📦 Packwerk is inspecting 12 files + \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. + 📦 Finished in \\d+\\.\\d+ seconds + + No offenses detected + ✅ `package_todo.yml` has been updated. + EOS + + assert_equal(package_todo_content, package_todo_content_after_update, + "expected no updates to any package todo file") + assert_match(/#{expected_output}/, captured_output) + end + + test "'packwerk update-todo' with violations succeeds and updates relevant package_todo" do + use_template(:skeleton) + package_todo_content = read_package_todo + timeline_package_todo_path = to_app_path(File.join(TIMELINE_PATH, "package_todo.yml")) + + open_app_file(TIMELINE_PATH.join("app", "models", "timeline_comment.rb")) do |file| + file.write("class TimelineComment; belongs_to :order; end") + file.flush + + assert_successful_run("update-todo") + + assert(File.exist?(timeline_package_todo_path), + "expected new package_todo for timeline package to be created") + + timeline_package_todo_content = File.read(timeline_package_todo_path) + assert_match( + "components/sales:\n \"::Order\":\n violations:\n - dependency", + timeline_package_todo_content + ) + + package_todo_content_after_update = + read_package_todo.reject { |k, _v| k.match?(timeline_package_todo_path) } + expected_output = <<~EOS + 📦 Packwerk is inspecting 13 files + \\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\.\\. + 📦 Finished in \\d+\\.\\d+ seconds + + No offenses detected + ✅ `package_todo.yml` has been updated. + EOS + + assert_equal(package_todo_content, package_todo_content_after_update, + "expected no updates to any package todo files besides timeline/package_todo.yml") + assert_match(/#{expected_output}/, captured_output) + end + end + + test "'packwerk check' does not blow up when parsing files with syntax issues from a false positive association" do + use_template(:skeleton) + open_app_file(TIMELINE_PATH.join("app", "models", "bad_file.rb")) do |file| + # This is an example of a file that has an object called `belongs_to` that accepts methods + content = <<~CONTENT + belongs_to.some_method + CONTENT + + file.write(content) + file.flush + + refute_successful_run("check") + + assert_match(/Packwerk is inspecting 13 files/, captured_output) + assert_match(%r{components/timeline/app/models/bad_file.rb}, captured_output) + assert_match(/Packwerk encountered an internal error/, captured_output) + assert_match(/For now, you can add this file to `packwerk.yml` `exclude` list./, captured_output) + assert_match(/Please file an issue and include this error message and stacktrace:/, captured_output) + assert_match(/Passed `nil` into T.must/, captured_output) + assert_match(/1 offense detected/, captured_output) + assert_match(/No stale violations detected/, captured_output) + end + end test "'packwerk check' for an app with external packages it detects a violation referencing a constant in the external package" do use_template(:external_packages)