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 e30af52c3..5552d28e4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,12 +6,11 @@ PATH ast better_html bundler - constant_resolver (>= 0.2.0) parallel parser prism (>= 0.25.0) sorbet-runtime (>= 0.5.9914) - zeitwerk (>= 2.6.1) + zeitwerk (>= 2.6.14) GEM remote: https://rubygems.org/ @@ -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) @@ -156,19 +154,19 @@ 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 DEPENDENCIES byebug - constant_resolver m minitest-focus mocha diff --git a/lib/packwerk.rb b/lib/packwerk.rb index 39023d1ca..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" diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb index 36125210e..7be2c7bb2 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( + sig { params(configuration: Configuration, packages: PackageSet).returns(Validator::Result) } + def check_application_structure(configuration, packages:) + constant_discovery = ConstantDiscovery.new( + packages, root_path: configuration.root_path.to_s, - load_paths: configuration.load_paths + loaders: configuration.loaders ) 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/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 4a1779ad9..6fad17ef5 100644 --- a/lib/packwerk/constant_discovery.rb +++ b/lib/packwerk/constant_discovery.rb @@ -1,30 +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 - # @param constant_resolver [ConstantResolver] - # @param packages [Packwerk::PackageSet] + class Error < StandardError; end + 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. @@ -56,20 +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 - return unless constant + paths_by_const = all_cpaths.invert + validate_constant_paths(paths_by_const, all_cpaths: all_cpaths) + @const_locations = paths_by_const + end - package = @packages.package_from_path(constant.location) - ConstantContext.new( - constant.name, - constant.location, - package, - ) + 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 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 d51c314d8..448602e91 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 @@ -17,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, @@ -25,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 @@ -32,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), @@ -41,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, @@ -53,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 @@ -112,17 +110,9 @@ 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( + package_set, root_path: @root_path, - load_paths: @load_paths, - inflector: @inflector, + loaders: @loaders ) 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/packwerk.gemspec b/packwerk.gemspec index dea4c163f..2d6bd00ef 100644 --- a/packwerk.gemspec +++ b/packwerk.gemspec @@ -40,10 +40,9 @@ 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.1") + spec.add_dependency("zeitwerk", ">= 2.6.14") # For Ruby parsing spec.add_dependency("ast") 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/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 8e5e13857..36cbbba4c 100644 --- a/test/support/rails_application_fixture_helper.rb +++ b/test/support/rails_application_fixture_helper.rb @@ -37,6 +37,9 @@ 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 + when :blank, :extended else raise "Unknown fixture template #{template}" end @@ -53,10 +56,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/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 7e77a3bf7..63cc71db5 100644 --- a/test/unit/packwerk/constant_discovery_test.rb +++ b/test/unit/packwerk/constant_discovery_test.rb @@ -8,51 +8,163 @@ module Business module Packwerk class ConstantDiscoveryTest < Minitest::Test - include TypedMock + include RailsApplicationFixtureHelper - 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 + setup do + setup_application_fixture + end - @discovery = ConstantDiscovery.new( - constant_resolver: ConstantResolver.new(root_path: @root_path, load_paths: load_paths), - packages: PackageSet.load_all_from(@root_path) - ) - super + teardown do + teardown_application_fixture end test "discovers simple constant" do - constant = @discovery.context_for("Order") + 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 - constant = @discovery.context_for("Business::Special") + 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 "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(@root_path) + 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(ConstantDiscovery::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 = ConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: [empty_loader] ) - error = assert_raises(ConstantResolver::Error) do - discovery.context_for("Sales::RecordNewOrder") + error = assert_raises(ConstantDiscovery::Error) do + with_empty_load_paths.validate_constants end - assert_equal(error.message, "initial error message") + 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(ConstantDiscovery::Error) do + discovery.validate_constants + end + + assert_match(/Ambiguous constant definition/, error.message) + end + + private + + def discovery + yield(Rails.autoloaders.main) if block_given? + + ConstantDiscovery.new( + PackageSet.load_all_from(app_dir), + root_path: app_dir, + loaders: Rails.autoloaders + ) end 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 699ec508d..34f6bc193 100644 --- a/test/unit/packwerk/reference_extractor_test.rb +++ b/test/unit/packwerk/reference_extractor_test.rb @@ -3,27 +3,17 @@ require "test_helper" require "support/packwerk/parser_test_helper" -require "constant_resolver" 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 } - - 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.new(packages, root_path: app_dir, loaders: Rails.autoloaders) end def teardown