From 7367e7e29d9d377d7e697a62f3f4a843d04fff29 Mon Sep 17 00:00:00 2001 From: Lewis Reid Date: Tue, 16 Jul 2024 17:34:08 +0900 Subject: [PATCH 1/3] Refactor to encapsulate usage of ConstantResolver within ConstantDiscovery --- lib/packwerk/application_validator.rb | 14 ++--- lib/packwerk/constant_discovery.rb | 24 +++++++ lib/packwerk/run_context.rb | 15 +---- lib/packwerk/validator.rb | 1 - .../components/sales/app/models/order.rb | 5 ++ .../components/sales/app/services/order.rb | 5 ++ .../ambiguous/components/sales/package.yml | 2 + test/fixtures/ambiguous/config/environment.rb | 0 test/fixtures/ambiguous/package.yml | 1 + test/fixtures/ambiguous/packwerk.yml | 4 ++ test/unit/packwerk/constant_discovery_test.rb | 63 +++++++++++++------ .../unit/packwerk/reference_extractor_test.rb | 8 +-- 12 files changed, 97 insertions(+), 45 deletions(-) create mode 100644 test/fixtures/ambiguous/components/sales/app/models/order.rb create mode 100644 test/fixtures/ambiguous/components/sales/app/services/order.rb create mode 100644 test/fixtures/ambiguous/components/sales/package.yml create mode 100644 test/fixtures/ambiguous/config/environment.rb create mode 100644 test/fixtures/ambiguous/package.yml create mode 100644 test/fixtures/ambiguous/packwerk.yml diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb index 36125210e..67995b3ca 100644 --- a/lib/packwerk/application_validator.rb +++ b/lib/packwerk/application_validator.rb @@ -1,7 +1,6 @@ # typed: strict # frozen_string_literal: true -require "constant_resolver" require "pathname" require "yaml" @@ -25,7 +24,7 @@ def check_all(package_set, configuration) def call(package_set, configuration) results = [ check_package_manifest_syntax(configuration), - check_application_structure(configuration), + check_application_structure(configuration, packages: package_set), check_package_manifest_paths(configuration), check_root_package_exists(configuration), ] @@ -68,15 +67,16 @@ def check_package_manifest_syntax(configuration) end end - sig { params(configuration: Configuration).returns(Validator::Result) } - def check_application_structure(configuration) - resolver = ConstantResolver.new( - root_path: configuration.root_path.to_s, + sig { params(configuration: Configuration, packages: PackageSet).returns(Validator::Result) } + def check_application_structure(configuration, packages:) + constant_discovery = ConstantDiscovery.for( + packages, + root_path: configuration.root_path.to_s, load_paths: configuration.load_paths ) begin - resolver.file_map + constant_discovery.validate_constants Validator::Result.new(ok: true) rescue => e Validator::Result.new(ok: false, error_value: e.message) diff --git a/lib/packwerk/constant_discovery.rb b/lib/packwerk/constant_discovery.rb index 4a1779ad9..a38a947cf 100644 --- a/lib/packwerk/constant_discovery.rb +++ b/lib/packwerk/constant_discovery.rb @@ -17,6 +17,23 @@ module Packwerk class ConstantDiscovery extend T::Sig + class << self + extend T::Sig + + sig do + params( + packages: PackageSet, + root_path: String, + load_paths: T::Hash[String, Module], + inflector: T.class_of(ActiveSupport::Inflector) + ).returns(ConstantDiscovery) + end + def for(packages, root_path:, load_paths:, inflector: ActiveSupport::Inflector) + resolver = ConstantResolver.new(root_path: root_path, load_paths: load_paths, inflector: inflector) + new(constant_resolver: resolver, packages: packages) + end + end + # @param constant_resolver [ConstantResolver] # @param packages [Packwerk::PackageSet] sig do @@ -71,6 +88,13 @@ def context_for(const_name, current_namespace_path: []) package, ) end + + sig do + returns(ConstantDiscovery) + end + def validate_constants + tap { @resolver.file_map } + end end private_constant :ConstantDiscovery diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index d51c314d8..320bf1a29 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -1,8 +1,6 @@ # typed: strict # frozen_string_literal: true -require "constant_resolver" - module Packwerk # Holds the context of a Packwerk run across multiple files. class RunContext @@ -111,18 +109,11 @@ def node_processor_factory sig { returns(ConstantDiscovery) } def context_provider - @context_provider ||= ConstantDiscovery.new( - constant_resolver: resolver, - packages: package_set - ) - end - - sig { returns(ConstantResolver) } - def resolver - ConstantResolver.new( + @context_provider ||= ConstantDiscovery.for( + package_set, root_path: @root_path, load_paths: @load_paths, - inflector: @inflector, + inflector: @inflector ) end diff --git a/lib/packwerk/validator.rb b/lib/packwerk/validator.rb index 9fde86d09..414372f63 100644 --- a/lib/packwerk/validator.rb +++ b/lib/packwerk/validator.rb @@ -1,7 +1,6 @@ # typed: strict # frozen_string_literal: true -require "constant_resolver" require "pathname" require "yaml" diff --git a/test/fixtures/ambiguous/components/sales/app/models/order.rb b/test/fixtures/ambiguous/components/sales/app/models/order.rb new file mode 100644 index 000000000..627a03838 --- /dev/null +++ b/test/fixtures/ambiguous/components/sales/app/models/order.rb @@ -0,0 +1,5 @@ +# typed: ignore +# frozen_string_literal: true + +class Order +end diff --git a/test/fixtures/ambiguous/components/sales/app/services/order.rb b/test/fixtures/ambiguous/components/sales/app/services/order.rb new file mode 100644 index 000000000..627a03838 --- /dev/null +++ b/test/fixtures/ambiguous/components/sales/app/services/order.rb @@ -0,0 +1,5 @@ +# typed: ignore +# frozen_string_literal: true + +class Order +end diff --git a/test/fixtures/ambiguous/components/sales/package.yml b/test/fixtures/ambiguous/components/sales/package.yml new file mode 100644 index 000000000..10b753ff7 --- /dev/null +++ b/test/fixtures/ambiguous/components/sales/package.yml @@ -0,0 +1,2 @@ +--- +enforce_dependencies: true diff --git a/test/fixtures/ambiguous/config/environment.rb b/test/fixtures/ambiguous/config/environment.rb new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/ambiguous/package.yml b/test/fixtures/ambiguous/package.yml new file mode 100644 index 000000000..a04d9566a --- /dev/null +++ b/test/fixtures/ambiguous/package.yml @@ -0,0 +1 @@ +enforce_dependencies: true diff --git a/test/fixtures/ambiguous/packwerk.yml b/test/fixtures/ambiguous/packwerk.yml new file mode 100644 index 000000000..9d3fea52f --- /dev/null +++ b/test/fixtures/ambiguous/packwerk.yml @@ -0,0 +1,4 @@ +include: + - "**/*.rb" + +offenses_formatter: plain diff --git a/test/unit/packwerk/constant_discovery_test.rb b/test/unit/packwerk/constant_discovery_test.rb index 7e77a3bf7..588010002 100644 --- a/test/unit/packwerk/constant_discovery_test.rb +++ b/test/unit/packwerk/constant_discovery_test.rb @@ -10,31 +10,19 @@ module Packwerk class ConstantDiscoveryTest < Minitest::Test include TypedMock - def setup - @root_path = "test/fixtures/skeleton/" - load_paths = - Dir.glob(File.join(@root_path, "components/*/{app,test}/*{/concerns,}")) - .map { |p| Pathname.new(p).relative_path_from(@root_path).to_s } - .each_with_object({}) { |p, h| h[p] = Object } - - load_paths["components/sales/app/internal"] = Business - - @discovery = ConstantDiscovery.new( - constant_resolver: ConstantResolver.new(root_path: @root_path, load_paths: load_paths), - packages: PackageSet.load_all_from(@root_path) - ) - super - end - test "discovers simple constant" do - constant = @discovery.context_for("Order") + constant = discovery.context_for("Order") assert_equal("::Order", constant.name) assert_equal("components/sales/app/models/order.rb", constant.location) assert_equal("components/sales", constant.package.name) end test "discovers constant in root dir with non-default namespace" do - constant = @discovery.context_for("Business::Special") + with_non_default_namespace = discovery do |load_paths| + load_paths["components/sales/app/internal"] = Business + end + + constant = with_non_default_namespace.context_for("Business::Special") assert_equal("::Business::Special", constant.name) assert_equal("components/sales/app/internal/special.rb", constant.location) assert_equal("components/sales", constant.package.name) @@ -45,7 +33,7 @@ def setup constant_resolver.stubs(:resolve).raises(ConstantResolver::Error, "initial error message") discovery = ConstantDiscovery.new( constant_resolver: constant_resolver, - packages: PackageSet.load_all_from(@root_path) + packages: PackageSet.load_all_from("test/fixtures/skeleton/") ) error = assert_raises(ConstantResolver::Error) do @@ -54,5 +42,42 @@ def setup assert_equal(error.message, "initial error message") end + + test "raises when validating constants and load paths contain no ruby files" do + with_no_ruby_files = discovery(&:clear) + + error = assert_raises(ConstantResolver::Error) do + with_no_ruby_files.validate_constants + end + + assert_match(/Could not find any ruby files/, error.message) + end + + test "raises when validating constants that include an ambiguous reference" do + with_ambiguous_ref = discovery("test/fixtures/ambiguous/") + + error = assert_raises(ConstantResolver::Error) do + with_ambiguous_ref.validate_constants + end + + assert_match(/Ambiguous constant definition/, error.message) + end + + private + + def discovery(root_path = "test/fixtures/skeleton/") + load_paths = + Dir.glob(File.join(root_path, "components/*/{app,test}/*{/concerns,}")) + .map { |p| Pathname.new(p).relative_path_from(root_path).to_s } + .each_with_object({}) { |p, h| h[p] = Object } + + yield(load_paths) if block_given? + + ConstantDiscovery.for( + PackageSet.load_all_from(root_path), + root_path: root_path, + load_paths: load_paths + ) + end end end diff --git a/test/unit/packwerk/reference_extractor_test.rb b/test/unit/packwerk/reference_extractor_test.rb index 699ec508d..7fec88fa9 100644 --- a/test/unit/packwerk/reference_extractor_test.rb +++ b/test/unit/packwerk/reference_extractor_test.rb @@ -3,7 +3,6 @@ require "test_helper" require "support/packwerk/parser_test_helper" -require "constant_resolver" module Packwerk class ReferenceExtractorTest < Minitest::Test @@ -16,14 +15,11 @@ def setup load_paths = Dir.glob(to_app_path("components/*/{app,test}/*{/concerns,}")) .map { |p| Pathname.new(p).relative_path_from(app_dir).to_s } + .each_with_object({}) { |p, h| h[p] = Object } - resolver = ConstantResolver.new(root_path: app_dir, load_paths: load_paths) packages = ::Packwerk::PackageSet.load_all_from(app_dir) - @context_provider = ConstantDiscovery.new( - constant_resolver: resolver, - packages: packages - ) + @context_provider = ConstantDiscovery.for(packages, root_path: app_dir, load_paths: load_paths) end def teardown From ff62ac18e90d50f4e63ea81027b0897f3aecc559 Mon Sep 17 00:00:00 2001 From: Lewis Reid Date: Thu, 18 Jul 2024 00:53:09 +0900 Subject: [PATCH 2/3] Implemented ConstantDiscovery alternative that uses Zeitwerk for constant paths --- Gemfile.lock | 5 +- lib/packwerk.rb | 1 + lib/packwerk/zeitwerk_constant_discovery.rb | 159 ++++++++++++++++ packwerk.gemspec | 2 +- .../rails_application_fixture_helper.rb | 8 + .../zeitwerk_constant_discovery_test.rb | 170 ++++++++++++++++++ 6 files changed, 342 insertions(+), 3 deletions(-) create mode 100644 lib/packwerk/zeitwerk_constant_discovery.rb create mode 100644 test/unit/packwerk/zeitwerk_constant_discovery_test.rb diff --git a/Gemfile.lock b/Gemfile.lock index e30af52c3..2ca42809e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -11,7 +11,7 @@ PATH parser prism (>= 0.25.0) sorbet-runtime (>= 0.5.9914) - zeitwerk (>= 2.6.1) + zeitwerk (>= 2.6.14) GEM remote: https://rubygems.org/ @@ -156,12 +156,13 @@ GEM yard-sorbet (0.8.1) sorbet-runtime (>= 0.5) yard (>= 0.9) - zeitwerk (2.6.4) + zeitwerk (2.6.16) PLATFORMS aarch64-linux arm64-darwin-21 arm64-darwin-22 + arm64-darwin-23 x86_64-darwin x86_64-darwin-20 x86_64-linux diff --git a/lib/packwerk.rb b/lib/packwerk.rb index 39023d1ca..a34c52000 100644 --- a/lib/packwerk.rb +++ b/lib/packwerk.rb @@ -59,6 +59,7 @@ module Validators autoload :AssociationInspector autoload :Cache autoload :ConstantDiscovery + autoload :ZeitwerkConstantDiscovery autoload :ConstantNameInspector autoload :ConstNodeInspector autoload :ExtensionLoader diff --git a/lib/packwerk/zeitwerk_constant_discovery.rb b/lib/packwerk/zeitwerk_constant_discovery.rb new file mode 100644 index 000000000..5fb4ba59d --- /dev/null +++ b/lib/packwerk/zeitwerk_constant_discovery.rb @@ -0,0 +1,159 @@ +# typed: strict +# frozen_string_literal: true + +module Packwerk + # Determines context for constants based on the provided `Zeitwerk::Loaders` such as + # their file location and the package they belong to. + class ZeitwerkConstantDiscovery + extend T::Sig + + class Error < StandardError; end + + sig do + params( + packages: PackageSet, + root_path: String, + loaders: T::Enumerable[Zeitwerk::Loader] + ).void + end + def initialize(packages, root_path:, loaders:) + @packages = packages + @root_path = root_path + @loaders = loaders + end + + # Get the package that owns a given file path. + # + # @param path [String] the file path + # + # @return [Packwerk::Package] the package that contains the given file, + # or nil if the path is not owned by any component + sig do + params( + path: String, + ).returns(Packwerk::Package) + end + def package_from_path(path) + @packages.package_from_path(path) + end + + # Analyze a constant via its name. + # If the constant is unresolved, we need the current namespace path to correctly infer its full name + # + # @param const_name [String] The unresolved constant's name. + # @param current_namespace_path [Array] (optional) The namespace of the context in which the constant is + # used, e.g. ["Apps", "Models"] for `Apps::Models`. Defaults to [] which means top level. + # @return [ConstantContext] + sig do + params( + const_name: String, + current_namespace_path: T.nilable(T::Array[String]), + ).returns(T.nilable(ConstantContext)) + end + def context_for(const_name, current_namespace_path: []) + current_namespace_path = [] if const_name.start_with?("::") + const_name, location = resolve_constant(const_name.delete_prefix("::"), current_namespace_path) + + return unless location + + location = location.delete_prefix("#{@root_path}#{File::SEPARATOR}").to_s + ConstantContext.new(const_name, location, package_from_path(location)) + end + + # Analyze the constants and raise errors if any potential issues are encountered that would prevent + # resolving the context for constants, such as ambiguous constant locations. + # + # @return [ZeitwerkConstantDiscovery] + sig { returns(ZeitwerkConstantDiscovery) } + def validate_constants + tap { const_locations } + end + + sig { returns(String) } + def inspect + "#<#{self.class.name}>" + end + + private + + sig { returns(T::Hash[String, String]) } + def const_locations + return @const_locations unless @const_locations.nil? + + all_cpaths = @loaders.inject({}) do |cpaths, loader| + paths = loader.all_expected_cpaths.filter do |cpath, _const| + cpath.ends_with?(".rb") + end + cpaths.merge(paths) + end + + paths_by_const = all_cpaths.invert + validate_constant_paths(paths_by_const, all_cpaths: all_cpaths) + @const_locations = paths_by_const + end + + sig do + params( + const_name: String, + current_namespace_path: T.nilable(T::Array[String]), + original_name: String + ).returns(T::Array[T.nilable(String)]) + end + def resolve_constant(const_name, current_namespace_path, original_name: const_name) + namespace, location = resolve_traversing_namespace_path(const_name, current_namespace_path) + if location + ["::" + namespace.push(original_name).join("::"), location] + elsif !const_name.include?("::") + # constant could not be resolved to a file in the given load paths + [nil, nil] + else + parent_constant = const_name.split("::")[0..-2].join("::") + resolve_constant(parent_constant, current_namespace_path, original_name: original_name) + end + end + + sig do + params( + const_name: String, + current_namespace_path: T.nilable(T::Array[String]), + ).returns(T::Array[T.nilable(String)]) + end + def resolve_traversing_namespace_path(const_name, current_namespace_path) + fully_qualified_name_guess = (current_namespace_path + [const_name]).join("::") + + location = const_locations[fully_qualified_name_guess] + if location || fully_qualified_name_guess == const_name + [current_namespace_path, location] + else + resolve_traversing_namespace_path(const_name, current_namespace_path[0..-2]) + end + end + + sig do + params( + paths_by_constant: T::Hash[String, String], + all_cpaths: T::Hash[String, String] + ).void + end + def validate_constant_paths(paths_by_constant, all_cpaths:) + raise(Error, "Could not find any ruby files.") if all_cpaths.empty? + + is_ambiguous = all_cpaths.size != paths_by_constant.size + raise(Error, ambiguous_constants_hint(paths_by_constant, all_cpaths: all_cpaths)) if is_ambiguous + end + + sig do + params( + paths_by_constant: T::Hash[String, String], + all_cpaths: T::Hash[String, String] + ).returns(String) + end + def ambiguous_constants_hint(paths_by_constant, all_cpaths:) + ambiguous_constants = all_cpaths.except(*paths_by_constant.invert.keys).values + <<~MSG + Ambiguous constant definition: + #{ambiguous_constants.map { |const| " - #{const}" }.join("\n")} + MSG + end + end +end diff --git a/packwerk.gemspec b/packwerk.gemspec index dea4c163f..4b8a015d1 100644 --- a/packwerk.gemspec +++ b/packwerk.gemspec @@ -43,7 +43,7 @@ Gem::Specification.new do |spec| spec.add_dependency("constant_resolver", ">= 0.2.0") spec.add_dependency("parallel") spec.add_dependency("sorbet-runtime", ">= 0.5.9914") - spec.add_dependency("zeitwerk", ">= 2.6.1") + spec.add_dependency("zeitwerk", ">= 2.6.14") # For Ruby parsing spec.add_dependency("ast") diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb index 8e5e13857..d630747ee 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -37,6 +37,8 @@ def use_template(template) set_load_paths_for_minimal_template when :skeleton set_load_paths_for_skeleton_template + when :ambiguous + set_load_paths_for_ambiguous_template else raise "Unknown fixture template #{template}" end @@ -53,10 +55,16 @@ def set_load_paths_for_minimal_template def set_load_paths_for_skeleton_template Rails.autoloaders.main.push_dir(*to_app_paths("/components/sales/app/models")) + Rails.autoloaders.main.push_dir(*to_app_paths("/components/sales/app/public")) Rails.autoloaders.main.push_dir(*to_app_paths("components/platform/app/models")) Rails.autoloaders.once.push_dir(*to_app_paths("components/timeline/app/models")) 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_ambiguous_template + Rails.autoloaders.main.push_dir(*to_app_paths("/components/sales/app/models")) + Rails.autoloaders.main.push_dir(*to_app_paths("/components/sales/app/services")) + end end diff --git a/test/unit/packwerk/zeitwerk_constant_discovery_test.rb b/test/unit/packwerk/zeitwerk_constant_discovery_test.rb new file mode 100644 index 000000000..9fdaf5546 --- /dev/null +++ b/test/unit/packwerk/zeitwerk_constant_discovery_test.rb @@ -0,0 +1,170 @@ +# typed: true +# frozen_string_literal: true + +require "test_helper" + +module Business +end + +module Packwerk + class ZeitwerkConstantDiscoveryTest < Minitest::Test + include RailsApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end + + test "discovers simple constant" do + use_template(:skeleton) + constant = discovery.context_for("Order") + assert_equal("::Order", constant.name) + assert_equal("components/sales/app/models/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "does not discover constant with invalid casing" do + use_template(:skeleton) + constant = discovery.context_for("ORDER") + assert_nil(constant) + end + + test "discovers nested constant" do + use_template(:skeleton) + constant = discovery.context_for("Sales::Order") + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + + constant = discovery.context_for("HasTimeline") + assert_equal("::HasTimeline", constant.name) + assert_equal("components/timeline/app/models/concerns/has_timeline.rb", constant.location) + assert_equal("components/timeline", constant.package.name) + end + + test "discovers constant that is fully qualified but does not have its own file" do + use_template(:skeleton) + constant = discovery.context_for("Sales::Errors::SomethingWentWrong") + assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is partially qualified inferring their full qualification" do + use_template(:skeleton) + constant = discovery.context_for( + "Errors", + current_namespace_path: ["Sales", "SomeEntryPoint"] + ) + assert_equal("::Sales::Errors", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is both partially qualified and do not have its own file" do + use_template(:skeleton) + constant = discovery.context_for( + "Errors::SomethingWentWrong", + current_namespace_path: ["Sales", "SomeEntrypoint"], + ) + assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is partially qualified and has a common name" do + use_template(:skeleton) + constant = discovery.context_for("Order", current_namespace_path: ["Sales", "Order"]) + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant in root dir with non-default namespace" do + use_template(:skeleton) + with_non_default_namespace = discovery do |loader| + loader.push_dir(*to_app_paths("components/sales/app/internal"), namespace: Business) + end + + constant = with_non_default_namespace.context_for("Business::Special") + assert_equal("::Business::Special", constant.name) + assert_equal("components/sales/app/internal/special.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is explicitly top level" do + use_template(:skeleton) + constant = discovery.context_for("::Order") + assert_equal("::Order", constant.name) + assert_equal("components/sales/app/models/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "respects top level prefix when discovering constants" do + use_template(:skeleton) + constant = discovery.context_for("Order", current_namespace_path: ["Sales"]) + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + + constant = discovery.context_for("::Order", current_namespace_path: ["Sales"]) + assert_equal("::Order", constant.name) + assert_equal("components/sales/app/models/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "raises with helpful message if there are errors resolving constants" do + use_template(:ambiguous) + + error = assert_raises(ZeitwerkConstantDiscovery::Error) do + discovery.context_for("Order") + end + + assert_match(/Ambiguous constant definition/, error.message) + end + + test "raises when validating constants and load paths contain no ruby files" do + use_template(:skeleton) + + empty_loader = Zeitwerk::Loader.new + empty_loader.setup + + with_empty_load_paths = ZeitwerkConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: [empty_loader] + ) + + error = assert_raises(ZeitwerkConstantDiscovery::Error) do + with_empty_load_paths.validate_constants + end + + assert_match(/Could not find any ruby files/, error.message) + end + + test "raises when validating constants that include an ambiguous reference" do + use_template(:ambiguous) + + error = assert_raises(ZeitwerkConstantDiscovery::Error) do + discovery.validate_constants + end + + assert_match(/Ambiguous constant definition/, error.message) + end + + private + + def discovery + yield(Rails.autoloaders.main) if block_given? + + ZeitwerkConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: Rails.autoloaders + ) + end + end +end From 80672a72ac728735e5b754168d664eb23d25b43e Mon Sep 17 00:00:00 2001 From: Lewis Reid Date: Thu, 18 Jul 2024 22:09:39 +0900 Subject: [PATCH 3/3] Replace ConstantDiscovery and remove ConstantResolver --- Gemfile | 1 - Gemfile.lock | 3 - lib/packwerk.rb | 2 +- lib/packwerk/application_validator.rb | 6 +- lib/packwerk/configuration.rb | 9 +- lib/packwerk/constant_discovery.rb | 154 +++++++++++----- lib/packwerk/rails_load_paths.rb | 45 +---- lib/packwerk/run_context.rb | 15 +- lib/packwerk/zeitwerk_constant_discovery.rb | 159 ---------------- packwerk.gemspec | 1 - test/fixtures/blank/config/.gitkeep | 0 test/fixtures/blank/config/environment.rb | 1 + .../blank/config/my_local_extension.rb | 22 +++ .../custom_executable_integration_test.rb | 2 +- .../rails_application_fixture_helper.rb | 1 + test/unit/packwerk/cli_test.rb | 5 +- .../commands/update_todo_command_test.rb | 2 +- test/unit/packwerk/constant_discovery_test.rb | 147 +++++++++++---- test/unit/packwerk/rails_load_paths_test.rb | 58 ------ .../unit/packwerk/reference_extractor_test.rb | 10 +- .../zeitwerk_constant_discovery_test.rb | 170 ------------------ 21 files changed, 270 insertions(+), 543 deletions(-) delete mode 100644 lib/packwerk/zeitwerk_constant_discovery.rb create mode 100644 test/fixtures/blank/config/.gitkeep create mode 100644 test/fixtures/blank/config/environment.rb create mode 100644 test/fixtures/blank/config/my_local_extension.rb delete mode 100644 test/unit/packwerk/rails_load_paths_test.rb delete mode 100644 test/unit/packwerk/zeitwerk_constant_discovery_test.rb diff --git a/Gemfile b/Gemfile index fd08875f4..db87c6cfb 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,6 @@ gemspec # Specify the same dependency sources as the application Gemfile gem("spring") -gem("constant_resolver", require: false) gem("rubocop-performance", require: false) gem("rubocop-sorbet", require: false) gem("mocha", require: false) diff --git a/Gemfile.lock b/Gemfile.lock index 2ca42809e..5552d28e4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,7 +6,6 @@ PATH ast better_html bundler - constant_resolver (>= 0.2.0) parallel parser prism (>= 0.25.0) @@ -45,7 +44,6 @@ GEM builder (3.2.4) byebug (11.1.3) concurrent-ruby (1.1.10) - constant_resolver (0.2.0) crass (1.0.6) erubi (1.11.0) i18n (1.12.0) @@ -169,7 +167,6 @@ PLATFORMS DEPENDENCIES byebug - constant_resolver m minitest-focus mocha diff --git a/lib/packwerk.rb b/lib/packwerk.rb index a34c52000..65fe90335 100644 --- a/lib/packwerk.rb +++ b/lib/packwerk.rb @@ -4,6 +4,7 @@ require "sorbet-runtime" require "active_support" require "fileutils" +require "zeitwerk" # Provides String#pluralize require "active_support/core_ext/string" @@ -59,7 +60,6 @@ module Validators autoload :AssociationInspector autoload :Cache autoload :ConstantDiscovery - autoload :ZeitwerkConstantDiscovery autoload :ConstantNameInspector autoload :ConstNodeInspector autoload :ExtensionLoader diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb index 67995b3ca..7be2c7bb2 100644 --- a/lib/packwerk/application_validator.rb +++ b/lib/packwerk/application_validator.rb @@ -69,10 +69,10 @@ def check_package_manifest_syntax(configuration) sig { params(configuration: Configuration, packages: PackageSet).returns(Validator::Result) } def check_application_structure(configuration, packages:) - constant_discovery = ConstantDiscovery.for( + constant_discovery = ConstantDiscovery.new( packages, - root_path: configuration.root_path.to_s, - load_paths: configuration.load_paths + root_path: configuration.root_path.to_s, + loaders: configuration.loaders ) begin diff --git a/lib/packwerk/configuration.rb b/lib/packwerk/configuration.rb index 7d106f8ed..566a3474d 100644 --- a/lib/packwerk/configuration.rb +++ b/lib/packwerk/configuration.rb @@ -96,12 +96,9 @@ def initialize(configs = {}, config_path: nil) end end - sig { returns(T::Hash[String, Module]) } - def load_paths - @load_paths ||= T.let( - RailsLoadPaths.for(@root_path, environment: "test"), - T.nilable(T::Hash[String, Module]), - ) + sig { returns(T::Enumerable[Zeitwerk::Loader]) } + def loaders + @loaders ||= RailsLoadPaths.loaders_for(@root_path, environment: "test") end sig { returns(T::Boolean) } diff --git a/lib/packwerk/constant_discovery.rb b/lib/packwerk/constant_discovery.rb index a38a947cf..6fad17ef5 100644 --- a/lib/packwerk/constant_discovery.rb +++ b/lib/packwerk/constant_discovery.rb @@ -1,47 +1,25 @@ # typed: strict # frozen_string_literal: true -require "constant_resolver" - module Packwerk - # Get information about unresolved constants without loading the application code. - # Information gathered: Fully qualified name, path to file containing the definition, package, - # and visibility (public/private to the package). - # - # The implementation makes a few assumptions about the code base: - # - `Something::SomeOtherThing` is defined in a path of either `something/some_other_thing.rb` or `something.rb`, - # relative to the load path. Rails' `zeitwerk` autoloader makes the same assumption. - # - It is OK to not always infer the exact file defining the constant. For example, when a constant is inherited, we - # have no way of inferring the file it is defined in. You could argue though that inheritance means that another - # constant with the same name exists in the inheriting class, and this view is sufficient for all our use cases. + # Determines context for constants based on the provided `Zeitwerk::Loaders` such as + # their file location and the package they belong to. class ConstantDiscovery extend T::Sig - class << self - extend T::Sig - - sig do - params( - packages: PackageSet, - root_path: String, - load_paths: T::Hash[String, Module], - inflector: T.class_of(ActiveSupport::Inflector) - ).returns(ConstantDiscovery) - end - def for(packages, root_path:, load_paths:, inflector: ActiveSupport::Inflector) - resolver = ConstantResolver.new(root_path: root_path, load_paths: load_paths, inflector: inflector) - new(constant_resolver: resolver, packages: packages) - end - end + class Error < StandardError; end - # @param constant_resolver [ConstantResolver] - # @param packages [Packwerk::PackageSet] sig do - params(constant_resolver: ConstantResolver, packages: Packwerk::PackageSet).void + params( + packages: PackageSet, + root_path: String, + loaders: T::Enumerable[Zeitwerk::Loader] + ).void end - def initialize(constant_resolver:, packages:) + def initialize(packages, root_path:, loaders:) @packages = packages - @resolver = constant_resolver + @root_path = root_path + @loaders = loaders end # Get the package that owns a given file path. @@ -73,27 +51,109 @@ def package_from_path(path) ).returns(T.nilable(ConstantContext)) end def context_for(const_name, current_namespace_path: []) - begin - constant = @resolver.resolve(const_name, current_namespace_path: current_namespace_path) - rescue ConstantResolver::Error => e - raise(ConstantResolver::Error, e.message) + current_namespace_path = [] if const_name.start_with?("::") + const_name, location = resolve_constant(const_name.delete_prefix("::"), current_namespace_path) + + return unless location + + location = location.delete_prefix("#{@root_path}#{File::SEPARATOR}").to_s + ConstantContext.new(const_name, location, package_from_path(location)) + end + + # Analyze the constants and raise errors if any potential issues are encountered that would prevent + # resolving the context for constants, such as ambiguous constant locations. + # + # @return [ConstantDiscovery] + sig { returns(ConstantDiscovery) } + def validate_constants + tap { const_locations } + end + + sig { returns(String) } + def inspect + "#<#{self.class.name}>" + end + + private + + sig { returns(T::Hash[String, String]) } + def const_locations + return @const_locations unless @const_locations.nil? + + all_cpaths = @loaders.inject({}) do |cpaths, loader| + paths = loader.all_expected_cpaths.filter do |cpath, _const| + cpath.ends_with?(".rb") + end + cpaths.merge(paths) + end + + paths_by_const = all_cpaths.invert + validate_constant_paths(paths_by_const, all_cpaths: all_cpaths) + @const_locations = paths_by_const + end + + sig do + params( + const_name: String, + current_namespace_path: T.nilable(T::Array[String]), + original_name: String + ).returns(T::Array[T.nilable(String)]) + end + def resolve_constant(const_name, current_namespace_path, original_name: const_name) + namespace, location = resolve_traversing_namespace_path(const_name, current_namespace_path) + if location + ["::" + namespace.push(original_name).join("::"), location] + elsif !const_name.include?("::") + # constant could not be resolved to a file in the given load paths + [nil, nil] + else + parent_constant = const_name.split("::")[0..-2].join("::") + resolve_constant(parent_constant, current_namespace_path, original_name: original_name) + end + end + + sig do + params( + const_name: String, + current_namespace_path: T.nilable(T::Array[String]), + ).returns(T::Array[T.nilable(String)]) + end + def resolve_traversing_namespace_path(const_name, current_namespace_path) + fully_qualified_name_guess = (current_namespace_path + [const_name]).join("::") + + location = const_locations[fully_qualified_name_guess] + if location || fully_qualified_name_guess == const_name + [current_namespace_path, location] + else + resolve_traversing_namespace_path(const_name, current_namespace_path[0..-2]) end + end - return unless constant + sig do + params( + paths_by_constant: T::Hash[String, String], + all_cpaths: T::Hash[String, String] + ).void + end + def validate_constant_paths(paths_by_constant, all_cpaths:) + raise(Error, "Could not find any ruby files.") if all_cpaths.empty? - package = @packages.package_from_path(constant.location) - ConstantContext.new( - constant.name, - constant.location, - package, - ) + is_ambiguous = all_cpaths.size != paths_by_constant.size + raise(Error, ambiguous_constants_hint(paths_by_constant, all_cpaths: all_cpaths)) if is_ambiguous end sig do - returns(ConstantDiscovery) + params( + paths_by_constant: T::Hash[String, String], + all_cpaths: T::Hash[String, String] + ).returns(String) end - def validate_constants - tap { @resolver.file_map } + def ambiguous_constants_hint(paths_by_constant, all_cpaths:) + ambiguous_constants = all_cpaths.except(*paths_by_constant.invert.keys).values + <<~MSG + Ambiguous constant definition: + #{ambiguous_constants.map { |const| " - #{const}" }.join("\n")} + MSG end end diff --git a/lib/packwerk/rails_load_paths.rb b/lib/packwerk/rails_load_paths.rb index c2b2336e3..c7e0634e8 100644 --- a/lib/packwerk/rails_load_paths.rb +++ b/lib/packwerk/rails_load_paths.rb @@ -11,43 +11,14 @@ module RailsLoadPaths class << self extend T::Sig - sig { params(root: String, environment: String).returns(T::Hash[String, Module]) } - def for(root, environment:) + sig { params(root: String, environment: String).returns(T::Enumerable[Zeitwerk::Loader]) } + def loaders_for(root, environment:) require_application(root, environment) - all_paths = extract_application_autoload_paths - relevant_paths = filter_relevant_paths(all_paths) - assert_load_paths_present(relevant_paths) - relative_path_strings(relevant_paths) + Rails.autoloaders end private - sig { returns(T::Hash[String, Module]) } - def extract_application_autoload_paths - Rails.autoloaders.inject({}) do |h, loader| - h.merge(loader.dirs(namespaces: true)) - end - end - - sig do - params(all_paths: T::Hash[String, Module], bundle_path: Pathname, rails_root: Pathname) - .returns(T::Hash[Pathname, Module]) - 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 - - sig { params(load_paths: T::Hash[Pathname, Module], rails_root: Pathname).returns(T::Hash[String, Module]) } - def relative_path_strings(load_paths, rails_root: Rails.root) - load_paths.transform_keys { |path| Pathname.new(path).relative_path_from(rails_root).to_s } - end - sig { params(root: String, environment: String).void } def require_application(root, environment) environment_file = "#{root}/config/environment" @@ -60,16 +31,6 @@ def require_application(root, environment) raise "A Rails application could not be found in #{root}" end end - - sig { params(paths: T::Hash[T.untyped, Module]).void } - def assert_load_paths_present(paths) - if paths.empty? - raise <<~EOS - We could not extract autoload paths from your Rails app. This is likely a configuration error. - Packwerk will not work correctly without any autoload paths. - EOS - end - end end end end diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 320bf1a29..448602e91 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -15,7 +15,6 @@ class << self def from_configuration(configuration) new( root_path: configuration.root_path, - load_paths: configuration.load_paths, package_paths: configuration.package_paths, inflector: ActiveSupport::Inflector, custom_associations: configuration.custom_associations, @@ -23,6 +22,7 @@ def from_configuration(configuration) cache_enabled: configuration.cache_enabled?, cache_directory: configuration.cache_directory, config_path: configuration.config_path, + loaders: configuration.loaders ) end end @@ -30,7 +30,6 @@ def from_configuration(configuration) sig do params( root_path: String, - load_paths: T::Hash[String, Module], inflector: T.class_of(ActiveSupport::Inflector), cache_directory: Pathname, config_path: T.nilable(String), @@ -39,11 +38,11 @@ def from_configuration(configuration) associations_exclude: T::Array[String], checkers: T::Array[Checker], cache_enabled: T::Boolean, + loaders: T::Enumerable[Zeitwerk::Loader] ).void end def initialize( root_path:, - load_paths:, inflector:, cache_directory:, config_path: nil, @@ -51,10 +50,11 @@ def initialize( custom_associations: [], associations_exclude: [], checkers: Checker.all, - cache_enabled: false + cache_enabled: false, + loaders: [] ) @root_path = root_path - @load_paths = load_paths + @loaders = loaders @package_paths = package_paths @inflector = inflector @custom_associations = custom_associations @@ -109,11 +109,10 @@ def node_processor_factory sig { returns(ConstantDiscovery) } def context_provider - @context_provider ||= ConstantDiscovery.for( + @context_provider ||= ConstantDiscovery.new( package_set, root_path: @root_path, - load_paths: @load_paths, - inflector: @inflector + loaders: @loaders ) end diff --git a/lib/packwerk/zeitwerk_constant_discovery.rb b/lib/packwerk/zeitwerk_constant_discovery.rb deleted file mode 100644 index 5fb4ba59d..000000000 --- a/lib/packwerk/zeitwerk_constant_discovery.rb +++ /dev/null @@ -1,159 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module Packwerk - # Determines context for constants based on the provided `Zeitwerk::Loaders` such as - # their file location and the package they belong to. - class ZeitwerkConstantDiscovery - extend T::Sig - - class Error < StandardError; end - - sig do - params( - packages: PackageSet, - root_path: String, - loaders: T::Enumerable[Zeitwerk::Loader] - ).void - end - def initialize(packages, root_path:, loaders:) - @packages = packages - @root_path = root_path - @loaders = loaders - end - - # Get the package that owns a given file path. - # - # @param path [String] the file path - # - # @return [Packwerk::Package] the package that contains the given file, - # or nil if the path is not owned by any component - sig do - params( - path: String, - ).returns(Packwerk::Package) - end - def package_from_path(path) - @packages.package_from_path(path) - end - - # Analyze a constant via its name. - # If the constant is unresolved, we need the current namespace path to correctly infer its full name - # - # @param const_name [String] The unresolved constant's name. - # @param current_namespace_path [Array] (optional) The namespace of the context in which the constant is - # used, e.g. ["Apps", "Models"] for `Apps::Models`. Defaults to [] which means top level. - # @return [ConstantContext] - sig do - params( - const_name: String, - current_namespace_path: T.nilable(T::Array[String]), - ).returns(T.nilable(ConstantContext)) - end - def context_for(const_name, current_namespace_path: []) - current_namespace_path = [] if const_name.start_with?("::") - const_name, location = resolve_constant(const_name.delete_prefix("::"), current_namespace_path) - - return unless location - - location = location.delete_prefix("#{@root_path}#{File::SEPARATOR}").to_s - ConstantContext.new(const_name, location, package_from_path(location)) - end - - # Analyze the constants and raise errors if any potential issues are encountered that would prevent - # resolving the context for constants, such as ambiguous constant locations. - # - # @return [ZeitwerkConstantDiscovery] - sig { returns(ZeitwerkConstantDiscovery) } - def validate_constants - tap { const_locations } - end - - sig { returns(String) } - def inspect - "#<#{self.class.name}>" - end - - private - - sig { returns(T::Hash[String, String]) } - def const_locations - return @const_locations unless @const_locations.nil? - - all_cpaths = @loaders.inject({}) do |cpaths, loader| - paths = loader.all_expected_cpaths.filter do |cpath, _const| - cpath.ends_with?(".rb") - end - cpaths.merge(paths) - end - - paths_by_const = all_cpaths.invert - validate_constant_paths(paths_by_const, all_cpaths: all_cpaths) - @const_locations = paths_by_const - end - - sig do - params( - const_name: String, - current_namespace_path: T.nilable(T::Array[String]), - original_name: String - ).returns(T::Array[T.nilable(String)]) - end - def resolve_constant(const_name, current_namespace_path, original_name: const_name) - namespace, location = resolve_traversing_namespace_path(const_name, current_namespace_path) - if location - ["::" + namespace.push(original_name).join("::"), location] - elsif !const_name.include?("::") - # constant could not be resolved to a file in the given load paths - [nil, nil] - else - parent_constant = const_name.split("::")[0..-2].join("::") - resolve_constant(parent_constant, current_namespace_path, original_name: original_name) - end - end - - sig do - params( - const_name: String, - current_namespace_path: T.nilable(T::Array[String]), - ).returns(T::Array[T.nilable(String)]) - end - def resolve_traversing_namespace_path(const_name, current_namespace_path) - fully_qualified_name_guess = (current_namespace_path + [const_name]).join("::") - - location = const_locations[fully_qualified_name_guess] - if location || fully_qualified_name_guess == const_name - [current_namespace_path, location] - else - resolve_traversing_namespace_path(const_name, current_namespace_path[0..-2]) - end - end - - sig do - params( - paths_by_constant: T::Hash[String, String], - all_cpaths: T::Hash[String, String] - ).void - end - def validate_constant_paths(paths_by_constant, all_cpaths:) - raise(Error, "Could not find any ruby files.") if all_cpaths.empty? - - is_ambiguous = all_cpaths.size != paths_by_constant.size - raise(Error, ambiguous_constants_hint(paths_by_constant, all_cpaths: all_cpaths)) if is_ambiguous - end - - sig do - params( - paths_by_constant: T::Hash[String, String], - all_cpaths: T::Hash[String, String] - ).returns(String) - end - def ambiguous_constants_hint(paths_by_constant, all_cpaths:) - ambiguous_constants = all_cpaths.except(*paths_by_constant.invert.keys).values - <<~MSG - Ambiguous constant definition: - #{ambiguous_constants.map { |const| " - #{const}" }.join("\n")} - MSG - end - end -end diff --git a/packwerk.gemspec b/packwerk.gemspec index 4b8a015d1..2d6bd00ef 100644 --- a/packwerk.gemspec +++ b/packwerk.gemspec @@ -40,7 +40,6 @@ Gem::Specification.new do |spec| spec.add_dependency("activesupport", ">= 6.0") spec.add_dependency("bundler") - spec.add_dependency("constant_resolver", ">= 0.2.0") spec.add_dependency("parallel") spec.add_dependency("sorbet-runtime", ">= 0.5.9914") spec.add_dependency("zeitwerk", ">= 2.6.14") diff --git a/test/fixtures/blank/config/.gitkeep b/test/fixtures/blank/config/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/test/fixtures/blank/config/environment.rb b/test/fixtures/blank/config/environment.rb new file mode 100644 index 000000000..b102b2a0c --- /dev/null +++ b/test/fixtures/blank/config/environment.rb @@ -0,0 +1 @@ +# this file intentionally left blank diff --git a/test/fixtures/blank/config/my_local_extension.rb b/test/fixtures/blank/config/my_local_extension.rb new file mode 100644 index 000000000..daaab4ba7 --- /dev/null +++ b/test/fixtures/blank/config/my_local_extension.rb @@ -0,0 +1,22 @@ +module MyLocalExtension +end + +class MyOffensesFormatter + include Packwerk::OffensesFormatter + + def show_offenses(offenses) + ["hi i am a custom offense formatter", *offenses].join("\n") + end + + def show_stale_violations(_offense_collection, _fileset) + "stale violations report" + end + + def identifier + 'my_offenses_formatter' + end + + def show_strict_mode_violations(offenses) + "strict mode violations report" + end +end diff --git a/test/integration/packwerk/custom_executable_integration_test.rb b/test/integration/packwerk/custom_executable_integration_test.rb index 2289a46fd..34dbdee44 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") diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb index d630747ee..36cbbba4c 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -39,6 +39,7 @@ def use_template(template) set_load_paths_for_skeleton_template when :ambiguous set_load_paths_for_ambiguous_template + when :blank, :extended else raise "Unknown fixture template #{template}" end diff --git a/test/unit/packwerk/cli_test.rb b/test/unit/packwerk/cli_test.rb index 4f3edf780..1fcafcd28 100644 --- a/test/unit/packwerk/cli_test.rb +++ b/test/unit/packwerk/cli_test.rb @@ -7,7 +7,7 @@ module Packwerk class CliTest < Minitest::Test include TypedMock - include ApplicationFixtureHelper + include RailsApplicationFixtureHelper setup do setup_application_fixture @@ -27,7 +27,6 @@ class CliTest < Minitest::Test offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new({ "parallel" => false }) - configuration.stubs(load_paths: {}) RunContext.any_instance.stubs(:process_file).at_least_once.returns([offense]) string_io = StringIO.new @@ -56,7 +55,6 @@ class CliTest < Minitest::Test offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new({ "parallel" => false }) - configuration.stubs(load_paths: {}) RunContext.any_instance.stubs(:process_file) .at_least(2) @@ -183,7 +181,6 @@ def show_strict_mode_violations(_offenses) offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new - configuration.stubs(load_paths: {}) RunContext.any_instance.stubs(:process_file) .returns([offense]) diff --git a/test/unit/packwerk/commands/update_todo_command_test.rb b/test/unit/packwerk/commands/update_todo_command_test.rb index d59fa9fb1..3e64358d6 100644 --- a/test/unit/packwerk/commands/update_todo_command_test.rb +++ b/test/unit/packwerk/commands/update_todo_command_test.rb @@ -8,7 +8,7 @@ module Packwerk module Commands class UpdateTodoCommandTest < Minitest::Test include FactoryHelper - include ApplicationFixtureHelper + include RailsApplicationFixtureHelper setup do setup_application_fixture diff --git a/test/unit/packwerk/constant_discovery_test.rb b/test/unit/packwerk/constant_discovery_test.rb index 588010002..63cc71db5 100644 --- a/test/unit/packwerk/constant_discovery_test.rb +++ b/test/unit/packwerk/constant_discovery_test.rb @@ -8,18 +8,85 @@ module Business module Packwerk class ConstantDiscoveryTest < Minitest::Test - include TypedMock + include RailsApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end test "discovers simple constant" do + use_template(:skeleton) constant = discovery.context_for("Order") assert_equal("::Order", constant.name) assert_equal("components/sales/app/models/order.rb", constant.location) assert_equal("components/sales", constant.package.name) end + test "does not discover constant with invalid casing" do + use_template(:skeleton) + constant = discovery.context_for("ORDER") + assert_nil(constant) + end + + test "discovers nested constant" do + use_template(:skeleton) + constant = discovery.context_for("Sales::Order") + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + + constant = discovery.context_for("HasTimeline") + assert_equal("::HasTimeline", constant.name) + assert_equal("components/timeline/app/models/concerns/has_timeline.rb", constant.location) + assert_equal("components/timeline", constant.package.name) + end + + test "discovers constant that is fully qualified but does not have its own file" do + use_template(:skeleton) + constant = discovery.context_for("Sales::Errors::SomethingWentWrong") + assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is partially qualified inferring their full qualification" do + use_template(:skeleton) + constant = discovery.context_for( + "Errors", + current_namespace_path: ["Sales", "SomeEntryPoint"] + ) + assert_equal("::Sales::Errors", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is both partially qualified and do not have its own file" do + use_template(:skeleton) + constant = discovery.context_for( + "Errors::SomethingWentWrong", + current_namespace_path: ["Sales", "SomeEntrypoint"], + ) + assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) + assert_equal("components/sales/app/public/sales/errors.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "discovers constant that is partially qualified and has a common name" do + use_template(:skeleton) + constant = discovery.context_for("Order", current_namespace_path: ["Sales", "Order"]) + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + test "discovers constant in root dir with non-default namespace" do - with_non_default_namespace = discovery do |load_paths| - load_paths["components/sales/app/internal"] = Business + use_template(:skeleton) + with_non_default_namespace = discovery do |loader| + loader.push_dir(*to_app_paths("components/sales/app/internal"), namespace: Business) end constant = with_non_default_namespace.context_for("Business::Special") @@ -28,36 +95,61 @@ class ConstantDiscoveryTest < Minitest::Test assert_equal("components/sales", constant.package.name) end - test "raises with helpful message if there is a constant resolver error" do - constant_resolver = typed_mock - constant_resolver.stubs(:resolve).raises(ConstantResolver::Error, "initial error message") - discovery = ConstantDiscovery.new( - constant_resolver: constant_resolver, - packages: PackageSet.load_all_from("test/fixtures/skeleton/") - ) + test "discovers constant that is explicitly top level" do + use_template(:skeleton) + constant = discovery.context_for("::Order") + assert_equal("::Order", constant.name) + assert_equal("components/sales/app/models/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end - error = assert_raises(ConstantResolver::Error) do - discovery.context_for("Sales::RecordNewOrder") + test "respects top level prefix when discovering constants" do + use_template(:skeleton) + constant = discovery.context_for("Order", current_namespace_path: ["Sales"]) + assert_equal("::Sales::Order", constant.name) + assert_equal("components/sales/app/models/sales/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + + constant = discovery.context_for("::Order", current_namespace_path: ["Sales"]) + assert_equal("::Order", constant.name) + assert_equal("components/sales/app/models/order.rb", constant.location) + assert_equal("components/sales", constant.package.name) + end + + test "raises with helpful message if there are errors resolving constants" do + use_template(:ambiguous) + + error = assert_raises(ConstantDiscovery::Error) do + discovery.context_for("Order") end - assert_equal(error.message, "initial error message") + assert_match(/Ambiguous constant definition/, error.message) end test "raises when validating constants and load paths contain no ruby files" do - with_no_ruby_files = discovery(&:clear) + use_template(:skeleton) + + empty_loader = Zeitwerk::Loader.new + empty_loader.setup - error = assert_raises(ConstantResolver::Error) do - with_no_ruby_files.validate_constants + with_empty_load_paths = ConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: [empty_loader] + ) + + error = assert_raises(ConstantDiscovery::Error) do + with_empty_load_paths.validate_constants end assert_match(/Could not find any ruby files/, error.message) end test "raises when validating constants that include an ambiguous reference" do - with_ambiguous_ref = discovery("test/fixtures/ambiguous/") + use_template(:ambiguous) - error = assert_raises(ConstantResolver::Error) do - with_ambiguous_ref.validate_constants + error = assert_raises(ConstantDiscovery::Error) do + discovery.validate_constants end assert_match(/Ambiguous constant definition/, error.message) @@ -65,18 +157,13 @@ class ConstantDiscoveryTest < Minitest::Test private - def discovery(root_path = "test/fixtures/skeleton/") - load_paths = - Dir.glob(File.join(root_path, "components/*/{app,test}/*{/concerns,}")) - .map { |p| Pathname.new(p).relative_path_from(root_path).to_s } - .each_with_object({}) { |p, h| h[p] = Object } - - yield(load_paths) if block_given? + def discovery + yield(Rails.autoloaders.main) if block_given? - ConstantDiscovery.for( - PackageSet.load_all_from(root_path), - root_path: root_path, - load_paths: load_paths + ConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: Rails.autoloaders ) end end diff --git a/test/unit/packwerk/rails_load_paths_test.rb b/test/unit/packwerk/rails_load_paths_test.rb deleted file mode 100644 index c3e5841b3..000000000 --- a/test/unit/packwerk/rails_load_paths_test.rb +++ /dev/null @@ -1,58 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -require "test_helper" -require "support/rails_test_helper" - -module Packwerk - class RailsLoadPathsTest < Minitest::Test - test ".for makes paths relative" do - RailsLoadPaths.expects(:require_application).with("/application/", "test").once.returns(true) - rails_root = Pathname.new("/application/") - Rails.expects(:root).twice.returns(rails_root) - relative_path = "app/models" - absolute_path = rails_root.join(relative_path) - RailsLoadPaths.expects(:extract_application_autoload_paths).once.returns(absolute_path.to_s => Object) - relative_path_strings = RailsLoadPaths.for(rails_root.to_s, environment: "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/") - 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 + ["/application/vendor/something/app/models"] - 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 calls out to filter the paths" do - RailsLoadPaths.expects(:filter_relevant_paths).once.returns(Pathname.new("/fake_path").to_s => Object) - RailsLoadPaths.expects(:require_application).with("/application", "test").once.returns(true) - - RailsLoadPaths.for("/application", environment: "test") - end - end -end diff --git a/test/unit/packwerk/reference_extractor_test.rb b/test/unit/packwerk/reference_extractor_test.rb index 7fec88fa9..34f6bc193 100644 --- a/test/unit/packwerk/reference_extractor_test.rb +++ b/test/unit/packwerk/reference_extractor_test.rb @@ -6,20 +6,14 @@ module Packwerk class ReferenceExtractorTest < Minitest::Test - include ApplicationFixtureHelper + include RailsApplicationFixtureHelper def setup setup_application_fixture use_template(:skeleton) - load_paths = - Dir.glob(to_app_path("components/*/{app,test}/*{/concerns,}")) - .map { |p| Pathname.new(p).relative_path_from(app_dir).to_s } - .each_with_object({}) { |p, h| h[p] = Object } - packages = ::Packwerk::PackageSet.load_all_from(app_dir) - - @context_provider = ConstantDiscovery.for(packages, root_path: app_dir, load_paths: load_paths) + @context_provider = ConstantDiscovery.new(packages, root_path: app_dir, loaders: Rails.autoloaders) end def teardown diff --git a/test/unit/packwerk/zeitwerk_constant_discovery_test.rb b/test/unit/packwerk/zeitwerk_constant_discovery_test.rb deleted file mode 100644 index 9fdaf5546..000000000 --- a/test/unit/packwerk/zeitwerk_constant_discovery_test.rb +++ /dev/null @@ -1,170 +0,0 @@ -# typed: true -# frozen_string_literal: true - -require "test_helper" - -module Business -end - -module Packwerk - class ZeitwerkConstantDiscoveryTest < Minitest::Test - include RailsApplicationFixtureHelper - - setup do - setup_application_fixture - end - - teardown do - teardown_application_fixture - end - - test "discovers simple constant" do - use_template(:skeleton) - constant = discovery.context_for("Order") - assert_equal("::Order", constant.name) - assert_equal("components/sales/app/models/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "does not discover constant with invalid casing" do - use_template(:skeleton) - constant = discovery.context_for("ORDER") - assert_nil(constant) - end - - test "discovers nested constant" do - use_template(:skeleton) - constant = discovery.context_for("Sales::Order") - assert_equal("::Sales::Order", constant.name) - assert_equal("components/sales/app/models/sales/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - - constant = discovery.context_for("HasTimeline") - assert_equal("::HasTimeline", constant.name) - assert_equal("components/timeline/app/models/concerns/has_timeline.rb", constant.location) - assert_equal("components/timeline", constant.package.name) - end - - test "discovers constant that is fully qualified but does not have its own file" do - use_template(:skeleton) - constant = discovery.context_for("Sales::Errors::SomethingWentWrong") - assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) - assert_equal("components/sales/app/public/sales/errors.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "discovers constant that is partially qualified inferring their full qualification" do - use_template(:skeleton) - constant = discovery.context_for( - "Errors", - current_namespace_path: ["Sales", "SomeEntryPoint"] - ) - assert_equal("::Sales::Errors", constant.name) - assert_equal("components/sales/app/public/sales/errors.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "discovers constant that is both partially qualified and do not have its own file" do - use_template(:skeleton) - constant = discovery.context_for( - "Errors::SomethingWentWrong", - current_namespace_path: ["Sales", "SomeEntrypoint"], - ) - assert_equal("::Sales::Errors::SomethingWentWrong", constant.name) - assert_equal("components/sales/app/public/sales/errors.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "discovers constant that is partially qualified and has a common name" do - use_template(:skeleton) - constant = discovery.context_for("Order", current_namespace_path: ["Sales", "Order"]) - assert_equal("::Sales::Order", constant.name) - assert_equal("components/sales/app/models/sales/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "discovers constant in root dir with non-default namespace" do - use_template(:skeleton) - with_non_default_namespace = discovery do |loader| - loader.push_dir(*to_app_paths("components/sales/app/internal"), namespace: Business) - end - - constant = with_non_default_namespace.context_for("Business::Special") - assert_equal("::Business::Special", constant.name) - assert_equal("components/sales/app/internal/special.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "discovers constant that is explicitly top level" do - use_template(:skeleton) - constant = discovery.context_for("::Order") - assert_equal("::Order", constant.name) - assert_equal("components/sales/app/models/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "respects top level prefix when discovering constants" do - use_template(:skeleton) - constant = discovery.context_for("Order", current_namespace_path: ["Sales"]) - assert_equal("::Sales::Order", constant.name) - assert_equal("components/sales/app/models/sales/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - - constant = discovery.context_for("::Order", current_namespace_path: ["Sales"]) - assert_equal("::Order", constant.name) - assert_equal("components/sales/app/models/order.rb", constant.location) - assert_equal("components/sales", constant.package.name) - end - - test "raises with helpful message if there are errors resolving constants" do - use_template(:ambiguous) - - error = assert_raises(ZeitwerkConstantDiscovery::Error) do - discovery.context_for("Order") - end - - assert_match(/Ambiguous constant definition/, error.message) - end - - test "raises when validating constants and load paths contain no ruby files" do - use_template(:skeleton) - - empty_loader = Zeitwerk::Loader.new - empty_loader.setup - - with_empty_load_paths = ZeitwerkConstantDiscovery.new( - PackageSet.load_all_from(app_dir), - root_path: app_dir, - loaders: [empty_loader] - ) - - error = assert_raises(ZeitwerkConstantDiscovery::Error) do - with_empty_load_paths.validate_constants - end - - assert_match(/Could not find any ruby files/, error.message) - end - - test "raises when validating constants that include an ambiguous reference" do - use_template(:ambiguous) - - error = assert_raises(ZeitwerkConstantDiscovery::Error) do - discovery.validate_constants - end - - assert_match(/Ambiguous constant definition/, error.message) - end - - private - - def discovery - yield(Rails.autoloaders.main) if block_given? - - ZeitwerkConstantDiscovery.new( - PackageSet.load_all_from(app_dir), - root_path: app_dir, - loaders: Rails.autoloaders - ) - end - end -end