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