From 98ccee7e61b081015e7918d88e053f646a6a61dc Mon Sep 17 00:00:00 2001 From: Jared McFarland Date: Fri, 18 Oct 2024 16:04:38 -0700 Subject: [PATCH] Fix check command stale todos with supplied files Because the PackageTodo class didn't know what files had been supplied it would erroneously suspect files had been deleted if you only passed a single file on the command line. We do this in a pre-commit hook with `bundle exec packwerk check $(git diff --name-only)` and it was causing issues; the tool would assert there were stale todos when there are not. This changes the method signature of OffenseFormatter.show_stale_violations to accept a FilesForProcessing instance instead of Set[String] and plubms the FilesForProcessing through to PackageTodo#stale_violations? so that it can call #files_specified? on it. If there were files specified in the command invokation it skips the #deleted_files_for call. Happy to make any adjustments. This addresses #369 --- Gemfile.lock | 180 ++++++++++-------- lib/packwerk/commands/check_command.rb | 4 +- .../formatters/default_offenses_formatter.rb | 6 +- lib/packwerk/offense_collection.rb | 6 +- lib/packwerk/offenses_formatter.rb | 4 +- lib/packwerk/package_todo.rb | 10 +- test/support/packwerk/factory_helper.rb | 12 ++ test/unit/packwerk/offense_collection_test.rb | 12 +- test/unit/packwerk/package_todo_test.rb | 55 ++++-- 9 files changed, 171 insertions(+), 118 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6405e8edb..36f42cf65 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -16,25 +16,36 @@ PATH GEM remote: https://rubygems.org/ specs: - actionpack (7.0.8.4) - actionview (= 7.0.8.4) - activesupport (= 7.0.8.4) - rack (~> 2.0, >= 2.2.4) + actionpack (7.2.1) + actionview (= 7.2.1) + activesupport (= 7.2.1) + nokogiri (>= 1.8.5) + racc + rack (>= 2.2.4, < 3.2) + rack-session (>= 1.0.1) rack-test (>= 0.6.3) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.0, >= 1.2.0) - actionview (7.0.8.4) - activesupport (= 7.0.8.4) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + useragent (~> 0.16) + actionview (7.2.1) + activesupport (= 7.2.1) builder (~> 3.1) - erubi (~> 1.4) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.1, >= 1.2.0) - activesupport (7.0.8.4) - concurrent-ruby (~> 1.0, >= 1.0.2) + erubi (~> 1.11) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + activesupport (7.2.1) + base64 + bigdecimal + concurrent-ruby (~> 1.0, >= 1.3.1) + connection_pool (>= 2.2.5) + drb i18n (>= 1.6, < 2) + logger (>= 1.4.2) minitest (>= 5.1) - tzinfo (~> 2.0) + securerandom (>= 0.3) + tzinfo (~> 2.0, >= 2.0.5) ast (2.4.2) + base64 (0.2.0) better_html (2.1.1) actionview (>= 6.0) activesupport (>= 6.0) @@ -42,45 +53,55 @@ GEM erubi (~> 1.4) parser (>= 2.4) smart_properties + bigdecimal (3.1.8) builder (3.3.0) byebug (11.1.3) concurrent-ruby (1.3.4) + connection_pool (2.4.1) constant_resolver (0.2.0) crass (1.0.6) + drb (2.2.1) erubi (1.13.0) - i18n (1.14.5) + i18n (1.14.6) concurrent-ruby (~> 1.0) + io-console (0.7.2) + irb (1.14.1) + rdoc (>= 4.0.0) + reline (>= 0.4.2) json (2.7.2) language_server-protocol (3.17.0.3) + logger (1.6.1) loofah (2.22.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) - m (1.6.0) + m (1.6.2) method_source (>= 0.6.7) rake (>= 0.9.2.2) method_source (1.1.0) minitest (5.25.1) - minitest-focus (1.3.1) + minitest-focus (1.4.0) minitest (>= 4, < 6) - mocha (1.14.0) + mocha (2.4.5) + ruby2_keywords (>= 0.0.5) netrc (0.11.0) - nokogiri (1.16.7-aarch64-linux) - racc (~> 1.4) nokogiri (1.16.7-arm64-darwin) racc (~> 1.4) - nokogiri (1.16.7-x86_64-darwin) - racc (~> 1.4) - nokogiri (1.16.7-x86_64-linux) - racc (~> 1.4) - parallel (1.25.1) - parser (3.3.3.0) + parallel (1.26.3) + parser (3.3.5.0) ast (~> 2.4.1) racc - prism (0.27.0) + prism (1.2.0) + psych (5.1.2) + stringio racc (1.8.1) - rack (2.2.9) + rack (3.1.7) + rack-session (2.0.0) + rack (>= 3.0.0) rack-test (2.1.0) rack (>= 1.3) + rackup (2.1.0) + rack (>= 3) + webrick (~> 1.8) rails-dom-testing (2.2.0) activesupport (>= 5.0.0) minitest @@ -88,85 +109,84 @@ GEM rails-html-sanitizer (1.6.0) loofah (~> 2.21) nokogiri (~> 1.14) - railties (7.0.8.4) - actionpack (= 7.0.8.4) - activesupport (= 7.0.8.4) - method_source + railties (7.2.1) + actionpack (= 7.2.1) + activesupport (= 7.2.1) + irb (~> 1.13) + rackup (>= 1.0.0) rake (>= 12.2) - thor (~> 1.0) - zeitwerk (~> 2.5) + thor (~> 1.0, >= 1.2.2) + zeitwerk (~> 2.6) rainbow (3.1.1) - rake (13.0.6) - rbi (0.1.12) - prism (>= 0.18.0, < 0.28) + rake (13.2.1) + rbi (0.2.1) + prism (~> 1.0) sorbet-runtime (>= 0.5.9204) + rdoc (6.7.0) + psych (>= 4.0.0) regexp_parser (2.9.2) - rexml (3.3.6) - strscan - rubocop (1.64.1) + reline (0.5.10) + io-console (~> 0.5) + rubocop (1.66.1) json (~> 2.3) language_server-protocol (>= 3.17.0) parallel (~> 1.10) parser (>= 3.3.0.2) rainbow (>= 2.2.2, < 4.0) - regexp_parser (>= 1.8, < 3.0) - rexml (>= 3.2.5, < 4.0) - rubocop-ast (>= 1.31.1, < 2.0) + regexp_parser (>= 2.4, < 3.0) + rubocop-ast (>= 1.32.2, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 2.4.0, < 3.0) - rubocop-ast (1.31.3) + rubocop-ast (1.32.3) parser (>= 3.3.1.0) - rubocop-performance (1.14.3) - rubocop (>= 1.7.0, < 2.0) - rubocop-ast (>= 0.4.0) - rubocop-shopify (2.9.0) - rubocop (~> 1.33) - rubocop-sorbet (0.6.11) - rubocop (>= 0.90.0) + rubocop-performance (1.22.1) + rubocop (>= 1.48.1, < 2.0) + rubocop-ast (>= 1.31.1, < 2.0) + rubocop-shopify (2.15.1) + rubocop (~> 1.51) + rubocop-sorbet (0.8.5) + rubocop (>= 1) ruby-progressbar (1.13.0) + ruby2_keywords (0.0.5) + securerandom (0.3.1) smart_properties (1.17.0) - sorbet (0.5.11367) - sorbet-static (= 0.5.11367) - sorbet-runtime (0.5.11367) - sorbet-static (0.5.11367-aarch64-linux) - sorbet-static (0.5.11367-universal-darwin) - sorbet-static (0.5.11367-x86_64-linux) - sorbet-static-and-runtime (0.5.11367) - sorbet (= 0.5.11367) - sorbet-runtime (= 0.5.11367) - spoom (1.3.2) + sorbet (0.5.11600) + sorbet-static (= 0.5.11600) + sorbet-runtime (0.5.11600) + sorbet-static (0.5.11600-universal-darwin) + sorbet-static-and-runtime (0.5.11600) + sorbet (= 0.5.11600) + sorbet-runtime (= 0.5.11600) + spoom (1.5.0) erubi (>= 1.10.0) - prism (>= 0.19.0) + prism (>= 0.28.0) sorbet-static-and-runtime (>= 0.5.10187) thor (>= 0.19.2) - spring (4.0.0) - strscan (3.1.0) - tapioca (0.13.3) + spring (4.2.1) + stringio (3.1.1) + tapioca (0.16.3) bundler (>= 2.2.25) netrc (>= 0.11.0) parallel (>= 1.21.0) - rbi (>= 0.1.4, < 0.2) + rbi (~> 0.2) sorbet-static-and-runtime (>= 0.5.11087) spoom (>= 1.2.0) thor (>= 1.2.0) yard-sorbet - thor (1.3.1) + thor (1.3.2) tzinfo (2.0.6) concurrent-ruby (~> 1.0) - unicode-display_width (2.5.0) - yard (0.9.36) - yard-sorbet (0.8.1) - sorbet-runtime (>= 0.5) - yard (>= 0.9) - zeitwerk (2.6.4) + unicode-display_width (2.6.0) + useragent (0.16.10) + webrick (1.8.2) + yard (0.9.37) + yard-sorbet (0.9.0) + sorbet-runtime + yard + zeitwerk (2.6.18) PLATFORMS - aarch64-linux - arm64-darwin-21 - arm64-darwin-22 - x86_64-darwin - x86_64-darwin-20 - x86_64-linux + arm64-darwin-23 DEPENDENCIES byebug @@ -186,4 +206,4 @@ DEPENDENCIES zeitwerk BUNDLED WITH - 2.4.8 + 2.5.11 diff --git a/lib/packwerk/commands/check_command.rb b/lib/packwerk/commands/check_command.rb index 43d9608cf..2c7b770e5 100644 --- a/lib/packwerk/commands/check_command.rb +++ b/lib/packwerk/commands/check_command.rb @@ -35,14 +35,14 @@ def run messages = [ offenses_formatter.show_offenses(offense_collection.outstanding_offenses), - offenses_formatter.show_stale_violations(offense_collection, @files_for_processing.files), + offenses_formatter.show_stale_violations(offense_collection, @files_for_processing), offenses_formatter.show_strict_mode_violations(unlisted_strict_mode_violations), ] out.puts(messages.select(&:present?).join("\n") + "\n") offense_collection.outstanding_offenses.empty? && - !offense_collection.stale_violations?(@files_for_processing.files) && + !offense_collection.stale_violations?(@files_for_processing) && unlisted_strict_mode_violations.empty? end diff --git a/lib/packwerk/formatters/default_offenses_formatter.rb b/lib/packwerk/formatters/default_offenses_formatter.rb index 6f12abaa3..9a683f153 100644 --- a/lib/packwerk/formatters/default_offenses_formatter.rb +++ b/lib/packwerk/formatters/default_offenses_formatter.rb @@ -20,9 +20,9 @@ def show_offenses(offenses) EOS end - sig { override.params(offense_collection: OffenseCollection, file_set: T::Set[String]).returns(String) } - def show_stale_violations(offense_collection, file_set) - if offense_collection.stale_violations?(file_set) + sig { override.params(offense_collection: OffenseCollection, files_for_processing: FilesForProcessing).returns(String) } + def show_stale_violations(offense_collection, files_for_processing) + if offense_collection.stale_violations?(files_for_processing) "There were stale violations found, please run `packwerk update-todo`" else "No stale violations detected" diff --git a/lib/packwerk/offense_collection.rb b/lib/packwerk/offense_collection.rb index 1e6d63823..27f413597 100644 --- a/lib/packwerk/offense_collection.rb +++ b/lib/packwerk/offense_collection.rb @@ -67,10 +67,10 @@ def add_offense(offense) end end - sig { params(for_files: T::Set[String]).returns(T::Boolean) } - def stale_violations?(for_files) + sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) } + def stale_violations?(files_for_processing) @package_todos.values.any? do |package_todo| - package_todo.stale_violations?(for_files) + package_todo.stale_violations?(files_for_processing) end end diff --git a/lib/packwerk/offenses_formatter.rb b/lib/packwerk/offenses_formatter.rb index 356b47472..c073500b8 100644 --- a/lib/packwerk/offenses_formatter.rb +++ b/lib/packwerk/offenses_formatter.rb @@ -70,8 +70,8 @@ def formatter_by_identifier(name) def show_offenses(offenses) end - sig { abstract.params(offense_collection: OffenseCollection, for_files: T::Set[String]).returns(String) } - def show_stale_violations(offense_collection, for_files) + sig { abstract.params(offense_collection: OffenseCollection, files_for_processing: FilesForProcessing).returns(String) } + def show_stale_violations(offense_collection, files_for_processing) end sig { abstract.returns(String) } diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb index 4eecd24fa..7b8240bb5 100644 --- a/lib/packwerk/package_todo.rb +++ b/lib/packwerk/package_todo.rb @@ -54,12 +54,16 @@ def add_entries(reference, violation_type) listed?(reference, violation_type: violation_type) end - sig { params(for_files: T::Set[String]).returns(T::Boolean) } - def stale_violations?(for_files) + sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) } + def stale_violations?(files_for_processing) prepare_entries_for_dump + for_files = files_for_processing.files + old_entries.any? do |package, violations| - files = for_files + deleted_files_for(package) + # We don't try to detect deleted files when files were specified on the CLI because + # we don't know if the file was deleted or just not specified + files = files_for_processing.files_specified? ? for_files : for_files + deleted_files_for(package) violations_for_files = package_violations_for(violations, files: files) # We `next false` because if we cannot find existing violations for `for_files` within diff --git a/test/support/packwerk/factory_helper.rb b/test/support/packwerk/factory_helper.rb index e624ed568..3cfd56151 100644 --- a/test/support/packwerk/factory_helper.rb +++ b/test/support/packwerk/factory_helper.rb @@ -22,5 +22,17 @@ def build_reference( source_location: source_location, ) end + + def build_files_for_processing( + relative_file_paths: [], + configuration: Configuration.new(), + ignore_nested_packages: false + ) + FilesForProcessing.new( + relative_file_paths, + configuration, + ignore_nested_packages + ) + end end end diff --git a/test/unit/packwerk/offense_collection_test.rb b/test/unit/packwerk/offense_collection_test.rb index 2a48d2b59..3e27a84d4 100644 --- a/test/unit/packwerk/offense_collection_test.rb +++ b/test/unit/packwerk/offense_collection_test.rb @@ -28,25 +28,29 @@ class OffenseCollectionTest < Minitest::Test test "#stale_violations? returns true if there are stale violations" do @offense_collection.add_offense(@offense) file_set = Set.new + FilesForProcessing.any_instance.stubs(:files).returns(file_set) + files_for_processing = build_files_for_processing Packwerk::PackageTodo.any_instance .expects(:stale_violations?) - .with(file_set) + .with(files_for_processing) .returns(true) - assert @offense_collection.stale_violations?(file_set) + assert @offense_collection.stale_violations?(files_for_processing) end test "#stale_violations? returns false if no stale violations" do @offense_collection.add_offense(@offense) file_set = Set.new + FilesForProcessing.any_instance.stubs(:files).returns(file_set) + files_for_processing = build_files_for_processing Packwerk::PackageTodo.any_instance .expects(:stale_violations?) - .with(file_set) + .with(files_for_processing) .returns(false) - refute @offense_collection.stale_violations?(Set.new) + refute @offense_collection.stale_violations?(files_for_processing) end test "#listed? returns true if constant is listed in file" do diff --git a/test/unit/packwerk/package_todo_test.rb b/test/unit/packwerk/package_todo_test.rb index 564245ea4..434b62288 100644 --- a/test/unit/packwerk/package_todo_test.rb +++ b/test/unit/packwerk/package_todo_test.rb @@ -39,20 +39,31 @@ class PackageTodoTest < Minitest::Test end test "#stale_violations? returns true if package TODO exist but no violations can be found in code" do - package_todo = PackageTodo.new(destination_package, "test/fixtures/package_todo.yml") - assert package_todo.stale_violations?(Set.new([ + files_to_check = [ "orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb", "orders/app/models/orders/services/adjustment_engine.rb", - ])) + ] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) + + package_todo = PackageTodo.new(destination_package, "test/fixtures/package_todo.yml") + assert package_todo.stale_violations?(build_files_for_processing) end test "#stale_violations? returns false if package TODO does not exist but violations are found in code" do + FilesForProcessing.any_instance.stubs(:files).returns(Set.new) + package_todo = PackageTodo.new(destination_package, "nonexistant_file_path") package_todo.add_entries(build_reference, ReferenceChecking::Checkers::DependencyChecker::VIOLATION_TYPE) - refute package_todo.stale_violations?(Set.new) + refute package_todo.stale_violations?(build_files_for_processing()) end test "#stale_violations? returns false if package TODO violation match violations found in code" do + files_to_check = [ + "orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb", + "orders/app/models/orders/services/adjustment_engine.rb", + ] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) + package = Package.new(name: "buyers", config: { "enforce_dependencies" => true }) first_violated_reference = build_reference( @@ -69,13 +80,15 @@ class PackageTodoTest < Minitest::Test package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml") package_todo.add_entries(first_violated_reference, "some_checker_type") package_todo.add_entries(second_violated_reference, "some_checker_type") - refute package_todo.stale_violations?(Set.new([ - "orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb", - "orders/app/models/orders/services/adjustment_engine.rb", - ])) + refute package_todo.stale_violations?(build_files_for_processing) end test "#stale_violations? returns true if one type of violation turns into a different type of violation" do + files_to_check = [ + "orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb", + "orders/app/models/orders/services/adjustment_engine.rb", + ] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) package = Package.new(name: "buyers", config: { "enforce_dependencies" => true }) first_violated_reference = build_reference( @@ -94,21 +107,22 @@ class PackageTodoTest < Minitest::Test ReferenceChecking::Checkers::DependencyChecker::VIOLATION_TYPE) package_todo.add_entries(second_violated_reference, ReferenceChecking::Checkers::DependencyChecker::VIOLATION_TYPE) - assert package_todo.stale_violations?(Set.new([ - "orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb", - "orders/app/models/orders/services/adjustment_engine.rb", - ])) + assert package_todo.stale_violations?(build_files_for_processing) end test "#stale_violations? returns true if violations in package_todo.yml exist but are not found when checking for violations" do + files_to_check = ["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) + package = Package.new(name: "buyers", config: { "enforce_dependencies" => true }) package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml") - assert package_todo.stale_violations?( - Set.new(["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"]) - ) + assert package_todo.stale_violations?(build_files_for_processing) end test "#stale_violations? returns false if violations in package_todo.yml exist but are found when checking for violations" do + files_to_check = ["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) + package = Package.new(name: "buyers", config: { "enforce_dependencies" => true }) violated_reference = build_reference( @@ -118,18 +132,17 @@ class PackageTodoTest < Minitest::Test ) package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml") package_todo.add_entries(violated_reference, "some_checker_type") - refute package_todo.stale_violations?( - Set.new(["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"]) - ) + refute package_todo.stale_violations?(build_files_for_processing) end test "#stale_violations? returns true when deleted files are present" do + files_to_check = ["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"] + FilesForProcessing.any_instance.stubs(:files).returns(Set.new(files_to_check)) + package = Package.new(name: "buyers", config: { "enforce_dependencies" => true }) package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml") - assert package_todo.stale_violations?( - Set.new(["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"]) - ) + assert package_todo.stale_violations?(build_files_for_processing) end test "#listed? returns false if constant is not violated" do