From 55c7bbefc3ce95f338a4e09f3dae95adcdccba5e Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 01:10:46 -0400 Subject: [PATCH 1/7] Stop polluting test output Without capturing logs, the warning makes it to stderr. By capturing, we can not only assert that it would have made it, but prevent it from actually appearing in the test output. --- test/deprecation_toolkit/warning_test.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/deprecation_toolkit/warning_test.rb b/test/deprecation_toolkit/warning_test.rb index e6bdcf1..02c4bbb 100644 --- a/test/deprecation_toolkit/warning_test.rb +++ b/test/deprecation_toolkit/warning_test.rb @@ -59,9 +59,13 @@ class WarningTest < ActiveSupport::TestCase end test "warn works as usual when no warnings are treated as deprecation" do - assert_nothing_raised do - warn "Test warn works correctly" + std_out, stderr = capture_io do + assert_nothing_raised do + warn("Test warn works correctly") + end end + assert_empty std_out + assert_equal "Test warn works correctly\n", stderr end test "Ruby 2.7 two-part keyword argument warning are joined together" do From b6720a984106a7241d657d0130fd1f4d69cbaf83 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 01:10:09 -0400 Subject: [PATCH 2/7] Add PathPrefixNormalizer --- lib/deprecation_toolkit.rb | 1 + .../path_prefix_normalizer.rb | 41 +++++ .../path_prefix_normalizer_test.rb | 152 ++++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 lib/deprecation_toolkit/path_prefix_normalizer.rb create mode 100644 test/deprecation_toolkit/path_prefix_normalizer_test.rb diff --git a/lib/deprecation_toolkit.rb b/lib/deprecation_toolkit.rb index 13b9317..2f99fcd 100644 --- a/lib/deprecation_toolkit.rb +++ b/lib/deprecation_toolkit.rb @@ -8,6 +8,7 @@ module DeprecationToolkit autoload :DeprecationSubscriber, "deprecation_toolkit/deprecation_subscriber" autoload :Configuration, "deprecation_toolkit/configuration" autoload :Collector, "deprecation_toolkit/collector" + autoload :PathPrefixNormalizer, "deprecation_toolkit/path_prefix_normalizer" autoload :ReadWriteHelper, "deprecation_toolkit/read_write_helper" autoload :TestTriggerer, "deprecation_toolkit/test_triggerer" diff --git a/lib/deprecation_toolkit/path_prefix_normalizer.rb b/lib/deprecation_toolkit/path_prefix_normalizer.rb new file mode 100644 index 0000000..0eb2ebb --- /dev/null +++ b/lib/deprecation_toolkit/path_prefix_normalizer.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require "pathname" + +module DeprecationToolkit + class PathPrefixNormalizer + attr_reader :path_prefixes, :replacement + + def initialize(*path_prefixes, replacement: "") + @path_prefixes = path_prefixes.compact.map do |path_prefix| + raise ArgumentError, "path prefixes must be absolute: #{path_prefix}" unless Pathname.new(path_prefix).absolute? + + ending_in_separator(path_prefix) + end.sort_by { |path| -path.length } + @replacement = replacement.empty? ? replacement : ending_in_separator(replacement) + end + + def call(message) + message.gsub(pattern, replacement) + end + + def to_s + "s#{pattern}#{replacement}" + end + + def pattern + # Naively anchor to the start of a path. + # The last character of each segment of a path is likely to match /\w/. + # Therefore, if the preceeding character does not match /w/, we're probably not in in the middle of a path. + # e.g. In a containerized environment, we may be given `/app` as a path prefix (perhaps from Rails.root). + # Given the following path: `/app/components/foo/app/models/bar.rb`, + # we should replace the prefix, producing: `components/foo/app/models/bar.rb`, + # without corrupting other occurences: `components/foomodels/bar.rb` + @pattern ||= /(? Date: Thu, 31 Mar 2022 01:11:15 -0400 Subject: [PATCH 3/7] Normalize paths in deprecation messages --- CHANGELOG.md | 2 + README.md | 32 +++++ lib/deprecation_toolkit/configuration.rb | 30 +++++ .../deprecation_subscriber.rb | 16 ++- test/deprecation_toolkit/warning_test.rb | 123 ++++++++++++++++++ 5 files changed, 198 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12c548c..c812275 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## main (unreleased) +* [#60](https://github.com/Shopify/deprecation_toolkit/pull/60): Normalize paths in deprecation messages. (@sambostock) + ## 2.0.0 (2022-03-16) * [#58](https://github.com/Shopify/deprecation_toolkit/pull/58): Drop support for Ruby < 2.6 & Active Support < 5.2. (@sambostock) diff --git a/README.md b/README.md index ad09c1d..671007c 100644 --- a/README.md +++ b/README.md @@ -120,6 +120,38 @@ This setting accepts an array of regular expressions. To match on all warnings, DeprecationToolkit::Configuration.warnings_treated_as_deprecation = [//] ``` +### 🔨 `#DeprecationToolkit::Configuration#message_normalizers` + +Deprecation Toolkit allows the normalization of deprecation warnings by registering message normalizers. + +Out-of-the-box, various path normalizers are included, to ensure that any paths included in deprecation warnings are consistent from machine to machine (see [`lib/deprecation_toolkit/configuration.rb`](lib/deprecation_toolkit/configuration.rb) for details). + +### Customizing normalization + +Additional normalizers can be added by appending then to the array: + +```ruby +DeprecationToolkit::Configuration.message_normalizers << ->(msg) { msg.downcase } +``` + +Normalizers are expected to respond to `.call`, accepting a `String` and returning a normalized `String`, and are applied in order of registration. + +If you wish to normalize a custom path, you can create your own `DeprecationToolkit::PathPrefixNormalizer`: + +```ruby +DeprecationToolkit::Configuration.message_normalizers << + DeprecationToolkit::PathPrefixNormalizer.new( + '/path/to/something', + replacement: 'optional replacement string', + ) +``` + +You may optionally disable any or all normalizers by mutating or replacing the array. + +```ruby +DeprecationToolkit::Configuration.message_normalizers = [] # remove all normalizers +``` + ## RSpec By default Deprecation Toolkit uses Minitest as its test runner. To use Deprecation Toolkit with RSpec you'll have to configure it. diff --git a/lib/deprecation_toolkit/configuration.rb b/lib/deprecation_toolkit/configuration.rb index 3814905..ecaf260 100644 --- a/lib/deprecation_toolkit/configuration.rb +++ b/lib/deprecation_toolkit/configuration.rb @@ -12,5 +12,35 @@ class Configuration config_accessor(:deprecation_path) { "test/deprecations" } config_accessor(:test_runner) { :minitest } config_accessor(:warnings_treated_as_deprecation) { [] } + + config_accessor(:message_normalizers) do + normalizers = [] + + normalizers << PathPrefixNormalizer.new(Rails.root) if defined?(Rails) + normalizers << PathPrefixNormalizer.new(Bundler.root) if defined?(Bundler) + + if defined?(Gem) + Gem.loaded_specs.each_value do |spec| + normalizers << PathPrefixNormalizer.new(spec.bin_dir, replacement: "") + normalizers << PathPrefixNormalizer.new(spec.extension_dir, replacement: "") + normalizers << PathPrefixNormalizer.new(spec.gem_dir, replacement: "") + end + normalizers << PathPrefixNormalizer.new(*Gem.path, replacement: "") + end + + begin + require "rbconfig" + normalizers << PathPrefixNormalizer.new( + *RbConfig::CONFIG.values_at("prefix", "sitelibdir", "rubylibdir"), + replacement: "", + ) + rescue LoadError + # skip normalizing ruby internal paths + end + + normalizers << PathPrefixNormalizer.new(Dir.pwd) + + normalizers + end end end diff --git a/lib/deprecation_toolkit/deprecation_subscriber.rb b/lib/deprecation_toolkit/deprecation_subscriber.rb index d46f3b7..40c7592 100644 --- a/lib/deprecation_toolkit/deprecation_subscriber.rb +++ b/lib/deprecation_toolkit/deprecation_subscriber.rb @@ -9,18 +9,24 @@ def self.already_attached? end def deprecation(event) - message = event.payload[:message] + message = normalize_message(event.payload[:message]) - Collector.collect(message) unless deprecation_allowed?(event.payload) + Collector.collect(message) unless deprecation_allowed?(message, event.payload[:callstack]) end private - def deprecation_allowed?(payload) + def deprecation_allowed?(message, callstack) allowed_deprecations, procs = Configuration.allowed_deprecations.partition { |el| el.is_a?(Regexp) } - allowed_deprecations.any? { |regex| regex =~ payload[:message] } || - procs.any? { |proc| proc.call(payload[:message], payload[:callstack]) } + allowed_deprecations.any? { |regex| regex =~ message } || + procs.any? { |proc| proc.call(message, callstack) } + end + + def normalize_message(message) + Configuration + .message_normalizers + .reduce(message) { |message, normalizer| normalizer.call(message) } end end end diff --git a/test/deprecation_toolkit/warning_test.rb b/test/deprecation_toolkit/warning_test.rb index 02c4bbb..04b66c8 100644 --- a/test/deprecation_toolkit/warning_test.rb +++ b/test/deprecation_toolkit/warning_test.rb @@ -82,5 +82,128 @@ class WarningTest < ActiveSupport::TestCase assert_match(/Using the last argument as keyword parameters/, error.message) assert_match(/The called method/, error.message) end + + test "'Ruby 2.7 last argument as keyword parameters' real deprecation warning is handled with normalized paths" do + Configuration.warnings_treated_as_deprecation = [/Using the last argument as keyword parameters/] + + error = assert_raises(Behaviors::DeprecationIntroduced) do + warn("#{Dir.pwd}/path/to/caller.rb:1: warning: Using the last argument as keyword parameters is deprecated; " \ + "maybe ** should be added to the call") + warn("#{Dir.pwd}/path/to/callee.rb:1: warning: The called method `method_name' is defined here") + + trigger_deprecation_toolkit_behavior + end + + assert_match(%r{^DEPRECATION WARNING: path/to/caller\.rb:1: warning: Using the last}, + error.message) + assert_match(%r{^path/to/callee\.rb:1: warning: The called method}, error.message) + end + + test "`assert_nil` real deprecation warning is handled with normalized paths" do + Configuration.warnings_treated_as_deprecation = [/Use assert_nil if expecting nil/] + + error = assert_raises(Behaviors::DeprecationIntroduced) do + warn("Use assert_nil if expecting nil from #{Dir.pwd}/path/to/file.rb:1. This will fail in Minitest 6.") + + trigger_deprecation_toolkit_behavior + end + + assert_match( + %r{^DEPRECATION WARNING: Use assert_nil if expecting nil from path/to/file\.rb:1}, error.message + ) + end + + test "the path to warn itself is handled too" do + Configuration.warnings_treated_as_deprecation = [/boom/] + + error = assert_raises(Behaviors::DeprecationIntroduced) do + warn("boom") + + trigger_deprecation_toolkit_behavior + end + + assert_includes(error.message, <<~MESSAGE.chomp) + DEPRECATION WARNING: boom + (called from call at /rubygems/core_ext/kernel_warn.rb:22) + MESSAGE + end + + test "Rails.root is normalized in deprecation messages" do + rails_stub = Object.new + rails_stub.define_singleton_method(:inspect) { "Rails (stub)" } + rails_stub.define_singleton_method(:root) { "/path/to/rails/root" } + + original_rails = defined?(::Rails) && ::Rails + Object.const_set(:Rails, rails_stub) + + assert_normalizes( + from: "#{Rails.root}/app/models/whatever.rb", + to: "app/models/whatever.rb", + ) + ensure + if original_rails.nil? + Object.send(:remove_const, :Rails) + else + Object.const_set(:Rails, original_rails) + end + end + + test "Bundler.root is normalized in deprecation messages" do + assert_normalizes( + from: "#{Bundler.root}/lib/whatever.rb", + to: "lib/whatever.rb", + ) + end + + test "Gem spec gem_dirs are normalized in deprecation messages" do + spec = Gem.loaded_specs.each_value.first + assert_normalizes( + from: "#{spec.gem_dir}/lib/whatever.rb", + to: "/lib/whatever.rb", + ) + end + + test "Gem spec extension_dirs are normalized in deprecation messages" do + spec = Gem.loaded_specs.each_value.first + assert_normalizes( + from: "#{spec.extension_dir}/lib/whatever.rb", + to: "/lib/whatever.rb", + ) + end + + test "Gem spec bin_dirs are normalized in deprecation messages" do + spec = Gem.loaded_specs.each_value.first + assert_normalizes( + from: "#{spec.bin_dir}/lib/whatever.rb", + to: "/lib/whatever.rb", + ) + end + + test "Gem paths are normalized in deprecation messages" do + paths = Gem.path + assert_normalizes( + from: paths.map.with_index { |path, index| "#{path}/file-#{index}" }.join("\n"), + to: Array.new(paths.length) { |index| "/file-#{index}" }.join("\n"), + ) + end + + test "RbConfig paths are normalized in deprecation messages" do + paths = RbConfig::CONFIG.values_at("prefix", "sitelibdir", "rubylibdir").compact + assert_normalizes( + from: paths.map.with_index { |path, index| "#{path}/file-#{index}" }.join("\n"), + to: Array.new(paths.length) { |index| "/file-#{index}" }.join("\n"), + ) + end + + private + + def assert_normalizes(from:, to:) + Configuration.warnings_treated_as_deprecation = [/test deprecation/] + error = assert_raises(Behaviors::DeprecationIntroduced) do + warn("test deprecation: #{from}.") + trigger_deprecation_toolkit_behavior + end + assert_includes(error.message, to) + end end end From 21da5cea438f7ece1dc8e7cba0e3279841c903ee Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 01:48:45 -0400 Subject: [PATCH 4/7] Rearrange where dir.pwd normalization happens --- lib/deprecation_toolkit/configuration.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/deprecation_toolkit/configuration.rb b/lib/deprecation_toolkit/configuration.rb index ecaf260..2e0ec70 100644 --- a/lib/deprecation_toolkit/configuration.rb +++ b/lib/deprecation_toolkit/configuration.rb @@ -18,6 +18,7 @@ class Configuration normalizers << PathPrefixNormalizer.new(Rails.root) if defined?(Rails) normalizers << PathPrefixNormalizer.new(Bundler.root) if defined?(Bundler) + normalizers << PathPrefixNormalizer.new(Dir.pwd) if defined?(Gem) Gem.loaded_specs.each_value do |spec| @@ -38,8 +39,6 @@ class Configuration # skip normalizing ruby internal paths end - normalizers << PathPrefixNormalizer.new(Dir.pwd) - normalizers end end From 762b6436f6a6a11b3ecaef8b50eb58ff0883ab78 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 01:52:07 -0400 Subject: [PATCH 5/7] [DO NOT MERGE] Fail slow to debug CI differences --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9dc2c8..b838287 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,7 @@ jobs: runs-on: ubuntu-latest name: Ruby ${{ matrix.ruby }} / ${{ matrix.gemfile }} strategy: + fail-fast: false matrix: gemfile: [gemfiles/activesupport_5.2.gemfile, gemfiles/activesupport_6.0.gemfile, gemfiles/activesupport_6.1.gemfile, gemfiles/activesupport_7.0.gemfile, gemfiles/activesupport_edge.gemfile] ruby: ["2.6", "2.7", "3.0", "3.1"] From 136b444778eb37d07ed54eb54a5ca08eab76bee5 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 01:56:41 -0400 Subject: [PATCH 6/7] Add CI debug logging --- test/deprecation_toolkit/warning_test.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/deprecation_toolkit/warning_test.rb b/test/deprecation_toolkit/warning_test.rb index 04b66c8..4ebaea8 100644 --- a/test/deprecation_toolkit/warning_test.rb +++ b/test/deprecation_toolkit/warning_test.rb @@ -181,6 +181,11 @@ class WarningTest < ActiveSupport::TestCase test "Gem paths are normalized in deprecation messages" do paths = Gem.path + puts + puts + pp paths # TODO: Remove this (debugging CI) + puts + puts assert_normalizes( from: paths.map.with_index { |path, index| "#{path}/file-#{index}" }.join("\n"), to: Array.new(paths.length) { |index| "/file-#{index}" }.join("\n"), From 0974406b6fc684cb59f1fa991d4c728bb50636d7 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 31 Mar 2022 02:02:18 -0400 Subject: [PATCH 7/7] More loggin --- test/deprecation_toolkit/warning_test.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/deprecation_toolkit/warning_test.rb b/test/deprecation_toolkit/warning_test.rb index 4ebaea8..2869261 100644 --- a/test/deprecation_toolkit/warning_test.rb +++ b/test/deprecation_toolkit/warning_test.rb @@ -183,6 +183,8 @@ class WarningTest < ActiveSupport::TestCase paths = Gem.path puts puts + pp Configuration.message_normalizers + puts pp paths # TODO: Remove this (debugging CI) puts puts