From 702d2dce1b9b47bc3b72173eae1e1a872bc57efc Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 22 Nov 2021 07:36:40 -0500 Subject: [PATCH] Pull load paths from application --- USAGE.md | 1 - lib/packwerk/application_validator.rb | 18 ----------- lib/packwerk/cli.rb | 15 +++++---- lib/packwerk/configuration.rb | 15 +++++++-- lib/packwerk/constant_discovery.rb | 2 +- lib/packwerk/generators/configuration_file.rb | 23 +++---------- .../generators/templates/packwerk.yml.erb | 3 -- test/fixtures/minimal/packwerk.yml | 2 -- test/fixtures/skeleton/packwerk.yml | 6 ---- test/unit/application_validator_test.rb | 32 ------------------- test/unit/cli_test.rb | 23 +++---------- test/unit/configuration_test.rb | 3 -- test/unit/constant_discovery_test.rb | 2 +- .../generators/configuration_file_test.rb | 19 +---------- test/unit/parse_run_test.rb | 15 +++++++++ 15 files changed, 48 insertions(+), 131 deletions(-) diff --git a/USAGE.md b/USAGE.md index 8472a6f1b..b095654ac 100644 --- a/USAGE.md +++ b/USAGE.md @@ -77,7 +77,6 @@ Packwerk reads from the `packwerk.yml` configuration file in the root directory. | include | **/*.{rb,rake,erb} | list of patterns for folder paths to include | | exclude | {bin,node_modules,script,tmp,vendor}/**/* | list of patterns for folder paths to exclude | | package_paths | **/ | a single pattern or a list of patterns to find package configuration files, see: [Defining packages](#Defining-packages) | -| load_paths | All application autoload paths | list of load paths | | custom_associations | N/A | list of custom associations, if any | | parallel | true | when true, fork code parsing out to subprocesses | diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb index e127e1aab..0f8a46bf6 100644 --- a/lib/packwerk/application_validator.rb +++ b/lib/packwerk/application_validator.rb @@ -14,15 +14,12 @@ def initialize(config_file_path:, configuration:, environment:) @config_file_path = config_file_path @configuration = configuration @environment = environment - - @application_load_paths = ApplicationLoadPaths.extract_relevant_paths(configuration.root_path, environment) end Result = Struct.new(:ok?, :error_value) def check_all results = [ - check_autoload_path_cache, check_package_manifests_for_privacy, check_package_manifest_syntax, check_application_structure, @@ -36,21 +33,6 @@ def check_all merge_results(results) end - def check_autoload_path_cache - expected = Set.new(@application_load_paths) - actual = Set.new(@configuration.load_paths) - if expected == actual - Result.new(true) - else - Result.new( - false, - "Load path cache in #{@config_file_path} incorrect!\n"\ - "Paths missing from file:\n#{format_yaml_strings(expected - actual)}\n"\ - "Extraneous load paths in file:\n#{format_yaml_strings(actual - expected)}" - ) - end - end - def check_package_manifests_for_privacy privacy_settings = package_manifests_settings_for("enforce_privacy") diff --git a/lib/packwerk/cli.rb b/lib/packwerk/cli.rb index 28cb45495..fcb46cdb1 100644 --- a/lib/packwerk/cli.rb +++ b/lib/packwerk/cli.rb @@ -88,10 +88,10 @@ def init def generate_configs configuration_file = Packwerk::Generators::ConfigurationFile.generate( - load_paths: Packwerk::ApplicationLoadPaths.extract_relevant_paths(@configuration.root_path, @environment), root: @configuration.root_path, out: @out ) + inflections_file = Packwerk::Generators::InflectionsFile.generate(root: @configuration.root_path, out: @out) root_package = Packwerk::Generators::RootPackage.generate(root: @configuration.root_path, out: @out) @@ -139,11 +139,6 @@ def fetch_files_to_process(paths, ignore_nested_packages) def validate(_paths) @progress_formatter.started_validation do - checker = Packwerk::ApplicationValidator.new( - config_file_path: @configuration.config_path, - configuration: @configuration, - environment: @environment, - ) result = checker.check_all list_validation_errors(result) @@ -152,6 +147,14 @@ def validate(_paths) end end + def checker + Packwerk::ApplicationValidator.new( + config_file_path: @configuration.config_path, + configuration: @configuration, + environment: @environment, + ) + end + def list_validation_errors(result) @out.puts if result.ok? diff --git a/lib/packwerk/configuration.rb b/lib/packwerk/configuration.rb index 5725f3355..977162ebd 100644 --- a/lib/packwerk/configuration.rb +++ b/lib/packwerk/configuration.rb @@ -34,24 +34,35 @@ def from_packwerk_config(path) DEFAULT_EXCLUDE_GLOBS = ["{bin,node_modules,script,tmp,vendor}/**/*"] attr_reader( - :include, :exclude, :root_path, :package_paths, :custom_associations, :load_paths, :inflections_file, + :include, :exclude, :root_path, :package_paths, :custom_associations, :inflections_file, :config_path, ) def initialize(configs = {}, config_path: nil) + if configs["load_paths"] + warning = <<~WARNING + DEPRECATION WARNING: The 'load_paths' key in `packwerk.yml` is deprecated. + This value is no longer cached, and you can remove the key from `packwerk.yml`. + WARNING + + warn(warning) + end @include = configs["include"] || DEFAULT_INCLUDE_GLOBS @exclude = configs["exclude"] || DEFAULT_EXCLUDE_GLOBS root = config_path ? File.dirname(config_path) : "." @root_path = File.expand_path(root) @package_paths = configs["package_paths"] || "**/" @custom_associations = configs["custom_associations"] || [] - @load_paths = (configs["load_paths"] || []).uniq @inflections_file = File.expand_path(configs["inflections_file"] || "config/inflections.yml", @root_path) @parallel = configs.key?("parallel") ? configs["parallel"] : true @config_path = config_path end + def load_paths + @load_paths ||= ApplicationLoadPaths.extract_relevant_paths(@root_path, "test") + end + def parallel? @parallel end diff --git a/lib/packwerk/constant_discovery.rb b/lib/packwerk/constant_discovery.rb index 88ac4bf42..815cce763 100644 --- a/lib/packwerk/constant_discovery.rb +++ b/lib/packwerk/constant_discovery.rb @@ -45,7 +45,7 @@ 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 + "\n Make sure autoload paths are added to the config file.") + raise(ConstantResolver::Error, e.message) end return unless constant diff --git a/lib/packwerk/generators/configuration_file.rb b/lib/packwerk/generators/configuration_file.rb index 2f41c0aba..136440202 100644 --- a/lib/packwerk/generators/configuration_file.rb +++ b/lib/packwerk/generators/configuration_file.rb @@ -11,18 +11,15 @@ class ConfigurationFile CONFIGURATION_TEMPLATE_FILE_PATH = "templates/packwerk.yml.erb" class << self - def generate(load_paths:, root:, out:) - new(load_paths: load_paths, root: root, out: out).generate + def generate(root:, out:) + new(root: root, out: out).generate end end - sig { params(load_paths: T::Array[String], root: String, out: T.any(StringIO, IO)).void } - def initialize(load_paths:, root:, out: $stdout) - @load_paths = load_paths + sig { params(root: String, out: T.any(StringIO, IO)).void } + def initialize(root:, out: $stdout) @root = root @out = out - - set_template_variables end sig { returns(T::Boolean) } @@ -43,18 +40,6 @@ def generate private - def set_template_variables - @load_paths_formatted = if @load_paths.empty? - "# load_paths:\n# - 'app/models'\n" - else - @load_paths.map { |path| "- #{path}\n" }.join - end - - @load_paths_comment = unless @load_paths.empty? - "# These load paths were auto generated by Packwerk.\nload_paths:\n" - end - end - def render ERB.new(template, trim_mode: "-").result(binding) end diff --git a/lib/packwerk/generators/templates/packwerk.yml.erb b/lib/packwerk/generators/templates/packwerk.yml.erb index dbbbdc8ac..a4aac81c5 100644 --- a/lib/packwerk/generators/templates/packwerk.yml.erb +++ b/lib/packwerk/generators/templates/packwerk.yml.erb @@ -12,9 +12,6 @@ # Patterns to find package configuration files # package_paths: "**/" -# List of application load paths -<%= @load_paths_comment -%> -<%= @load_paths_formatted %> # List of custom associations, if any # custom_associations: # - "cache_belongs_to" diff --git a/test/fixtures/minimal/packwerk.yml b/test/fixtures/minimal/packwerk.yml index ff2ef1195..603dbfb48 100644 --- a/test/fixtures/minimal/packwerk.yml +++ b/test/fixtures/minimal/packwerk.yml @@ -1,4 +1,2 @@ include: - "**/*.rb" -load_paths: - - components/sales/app/models diff --git a/test/fixtures/skeleton/packwerk.yml b/test/fixtures/skeleton/packwerk.yml index 2bd593903..72f2cb89b 100644 --- a/test/fixtures/skeleton/packwerk.yml +++ b/test/fixtures/skeleton/packwerk.yml @@ -3,11 +3,5 @@ include: - "**/*.{blop,rb}" # To test for duplicated files exclude: - "**/temp.rb" -load_paths: - - components/platform/app/models - - components/sales/app/models - - components/timeline/app/models - - components/timeline/app/models/concerns - - vendor/cache/gems/example/models inflections_file: "custom_inflections.yml" parallel: false diff --git a/test/unit/application_validator_test.rb b/test/unit/application_validator_test.rb index 1f4b68059..8935d8d74 100644 --- a/test/unit/application_validator_test.rb +++ b/test/unit/application_validator_test.rb @@ -26,38 +26,6 @@ class ApplicationValidatorTest < Minitest::Test assert result.ok?, result.error_value end - test "check_autoload_path_cache fails on extraneous config load paths" do - use_template(:minimal) - ApplicationLoadPaths.expects(:extract_relevant_paths).returns([]) - - result = validator.check_autoload_path_cache - - refute result.ok?, result.error_value - assert_match %r{Extraneous load paths in file:.*components/sales/app/models}m, result.error_value - end - - test "check_autoload_path_cache succeeds on duplicated, but correct config load paths" do - use_template(:minimal) - config.expects(:load_paths).returns(["components/timeline/app/models"] * 2) - ApplicationLoadPaths.expects(:extract_relevant_paths).returns(["components/timeline/app/models"]) - - result = validator.check_autoload_path_cache - - assert result.ok?, result.error_value - end - - test "check_autoload_path_cache fails on missing config load paths" do - use_template(:minimal) - ApplicationLoadPaths - .expects(:extract_relevant_paths) - .returns(["components/sales/app/models", "components/timeline/app/models"]) - - result = validator.check_autoload_path_cache - - refute result.ok?, result.error_value - assert_match %r{Paths missing from file:.*components/timeline/app/models}m, result.error_value - end - test "check_package_manifest_syntax returns an error for unknown package keys" do use_template(:minimal) merge_into_app_yaml_file("package.yml", { "enforce_correctness" => false }) diff --git a/test/unit/cli_test.rb b/test/unit/cli_test.rb index 2c19ab501..18dedaec5 100644 --- a/test/unit/cli_test.rb +++ b/test/unit/cli_test.rb @@ -24,6 +24,7 @@ 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 @@ -48,6 +49,8 @@ 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) .returns([offense]) @@ -101,25 +104,6 @@ class CliTest < Minitest::Test refute success end - test "#execute_command with init subcommand fails application validation generator for non-Rails app" do - string_io = StringIO.new - configuration = Configuration.new - configuration.stubs( - root_path: @temp_dir, - load_paths: ["path"], - package_paths: "**/", - custom_associations: ["cached_belongs_to"], - inflections_file: "config/inflections.yml" - ) - cli = ::Packwerk::Cli.new(configuration: configuration, out: string_io) - - e = assert_raises(RuntimeError) do - cli.execute_command(["init"]) - end - - assert_includes e.message, "A Rails application could not be found in" - end - test "#execute_command with empty subcommand lists all the valid subcommands" do @cli.execute_command([]) @@ -151,6 +135,7 @@ def show_stale_violations(_offense_collection) 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/configuration_test.rb b/test/unit/configuration_test.rb index c5e0579cf..897e9bd17 100644 --- a/test/unit/configuration_test.rb +++ b/test/unit/configuration_test.rb @@ -32,7 +32,6 @@ class ConfigurationTest < Minitest::Test "include" => ["xyz/*.rb"], "exclude" => ["{exclude_dir,bin,tmp}/**/*"], "package_paths" => "**/*/", - "load_paths" => ["app/models"], "custom_associations" => ["custom_association"], "inflections_file" => "custom_inflections.yml", } @@ -43,7 +42,6 @@ class ConfigurationTest < Minitest::Test assert_equal ["xyz/*.rb"], configuration.include assert_equal ["{exclude_dir,bin,tmp}/**/*"], configuration.exclude assert_equal app_dir, configuration.root_path - assert_equal ["app/models"], configuration.load_paths assert_equal "**/*/", configuration.package_paths assert_equal ["custom_association"], configuration.custom_associations assert_equal to_app_path("custom_inflections.yml"), configuration.inflections_file @@ -58,7 +56,6 @@ class ConfigurationTest < Minitest::Test assert_equal ["**/*.{rb,rake,erb}"], configuration.include assert_equal ["{bin,node_modules,script,tmp,vendor}/**/*"], configuration.exclude assert_equal app_dir, configuration.root_path - assert_empty configuration.load_paths assert_equal "**/", configuration.package_paths assert_empty configuration.custom_associations assert_equal to_app_path("config/inflections.yml"), configuration.inflections_file diff --git a/test/unit/constant_discovery_test.rb b/test/unit/constant_discovery_test.rb index 67aee1c30..a1e6a11a9 100644 --- a/test/unit/constant_discovery_test.rb +++ b/test/unit/constant_discovery_test.rb @@ -46,7 +46,7 @@ def setup discovery.context_for("Sales::RecordNewOrder") end - assert_equal(error.message, "initial error message\n Make sure autoload paths are added to the config file.") + assert_equal(error.message, "initial error message") end end end diff --git a/test/unit/generators/configuration_file_test.rb b/test/unit/generators/configuration_file_test.rb index 9b6ce0eac..7772d3e1e 100644 --- a/test/unit/generators/configuration_file_test.rb +++ b/test/unit/generators/configuration_file_test.rb @@ -9,31 +9,15 @@ class ConfigurationFileTest < Minitest::Test setup do @string_io = StringIO.new @temp_dir = Dir.mktmpdir - @test_path = "the/path/we/want" end teardown do FileUtils.remove_entry(@temp_dir) end - test ".generate creates a configuration file with load paths if available" do - load_paths = [@test_path] - generated_file_path = File.join(@temp_dir, Packwerk::Configuration::DEFAULT_CONFIG_PATH) - - assert(Packwerk::Generators::ConfigurationFile.generate( - load_paths: load_paths, - root: @temp_dir, - out: @string_io - )) - assert(File.exist?(generated_file_path)) - - contents = YAML.load_file(generated_file_path) - assert_equal(load_paths, contents["load_paths"]) - end - test ".generate creates a default configuration file if there were empty load paths array" do generated_file_path = File.join(@temp_dir, Packwerk::Configuration::DEFAULT_CONFIG_PATH) - assert(Packwerk::Generators::ConfigurationFile.generate(load_paths: [], root: @temp_dir, out: @string_io)) + assert(Packwerk::Generators::ConfigurationFile.generate(root: @temp_dir, out: @string_io)) assert(File.exist?(generated_file_path)) end @@ -41,7 +25,6 @@ class ConfigurationFileTest < Minitest::Test file_path = File.join(@temp_dir, Packwerk::Configuration::DEFAULT_CONFIG_PATH) File.open(file_path, "w") do |_f| assert(Packwerk::Generators::ConfigurationFile.generate( - load_paths: [@test_path], root: @temp_dir, out: @string_io )) diff --git a/test/unit/parse_run_test.rb b/test/unit/parse_run_test.rb index 4007b9ac2..19c6c81a5 100644 --- a/test/unit/parse_run_test.rb +++ b/test/unit/parse_run_test.rb @@ -5,8 +5,18 @@ module Packwerk class ParseRunTest < Minitest::Test include FactoryHelper + include ApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end test "#detect_stale_violations returns expected Result when stale violations present" do + use_template(:minimal) OffenseCollection.any_instance.stubs(:stale_violations?).returns(true) RunContext.any_instance.stubs(:process_file).returns([]) @@ -17,6 +27,7 @@ class ParseRunTest < Minitest::Test end test "#update_deprecations returns success when there are no offenses" do + use_template(:minimal) RunContext.any_instance.stubs(:process_file).returns([]) OffenseCollection.any_instance.expects(:dump_deprecated_references_files).once @@ -31,6 +42,7 @@ class ParseRunTest < Minitest::Test end test "#update_deprecations returns exit code 1 when there are offenses" do + use_template(:minimal) offense = Offense.new(file: "path/of/exile.rb", message: "something") RunContext.any_instance.stubs(:process_file).returns([offense]) OffenseCollection.any_instance.expects(:dump_deprecated_references_files).once @@ -51,6 +63,7 @@ class ParseRunTest < Minitest::Test end test "#check only reports error progress for unlisted violations" do + use_template(:minimal) offense = ReferenceOffense.new(reference: build_reference, violation_type: ViolationType::Privacy) DeprecatedReferences.any_instance.stubs(:listed?).returns(true) out = StringIO.new @@ -78,6 +91,7 @@ class ParseRunTest < Minitest::Test end test "#check result has failure status when stale violations exist" do + use_template(:minimal) offense = ReferenceOffense.new(reference: build_reference, violation_type: ViolationType::Privacy) DeprecatedReferences.any_instance.stubs(:listed?).returns(true) OffenseCollection.any_instance.stubs(:stale_violations?).returns(true) @@ -107,6 +121,7 @@ class ParseRunTest < Minitest::Test end test "runs in parallel" do + use_template(:minimal) offense = ReferenceOffense.new(reference: build_reference, violation_type: ViolationType::Privacy) offense2 = ReferenceOffense.new( reference: build_reference(path: "some/other_path.rb"),