From 33fdae0584b5715d7111d91360792e62f188db92 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Wed, 7 Oct 2020 15:03:11 -0500 Subject: [PATCH] Fix backtraces for generated plugin tests `Minitest.plugin_rails_init` sets `Minitest.backtrace_filter` to `Rails.backtrace_cleaner` right before tests are run, overwriting the value set in test_helper.rb. `Rails.backtrace_cleaner` silences backtrace lines that do not start with `Rails.root` followed by e.g. "lib/" or "test/". Thus when `Rails.root` is a subdirectory of the project directory -- for example, when testing a plugin that has a dummy app -- all lines of the backtrace are silenced. This commit adds a fallback such that when all backtrace lines are silenced, the original `Minitest.backtrace_filter` is used instead. Additionally, this commit refactors and expands existing test coverage. --- actionmailbox/test/test_helper.rb | 2 - actiontext/test/test_helper.rb | 4 - activestorage/test/test_helper.rb | 3 - railties/lib/minitest/rails_plugin.rb | 29 ++--- .../plugin/templates/test/test_helper.rb.tt | 3 - railties/test/application/test_runner_test.rb | 34 ------ .../test/generators/plugin_generator_test.rb | 1 - railties/test/minitest/rails_plugin_test.rb | 110 ++++++++++++++---- 8 files changed, 106 insertions(+), 80 deletions(-) diff --git a/actionmailbox/test/test_helper.rb b/actionmailbox/test/test_helper.rb index 09f6cc818d..74c67d32d7 100644 --- a/actionmailbox/test/test_helper.rb +++ b/actionmailbox/test/test_helper.rb @@ -10,8 +10,6 @@ require "rails/test_help" require "byebug" require "webmock/minitest" -Minitest.backtrace_filter = Minitest::BacktraceFilter.new - require "rails/test_unit/reporter" Rails::TestUnitReporter.executable = "bin/test" diff --git a/actiontext/test/test_helper.rb b/actiontext/test/test_helper.rb index 556ace73e5..150ccb3877 100644 --- a/actiontext/test/test_helper.rb +++ b/actiontext/test/test_helper.rb @@ -7,10 +7,6 @@ require_relative "../test/dummy/config/environment" ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] require "rails/test_help" -# Filter out Minitest backtrace while allowing backtrace from other libraries -# to be shown. -Minitest.backtrace_filter = Minitest::BacktraceFilter.new - require "rails/test_unit/reporter" Rails::TestUnitReporter.executable = "bin/test" diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 7c2064c777..6ce5fef294 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -21,9 +21,6 @@ require "active_job" ActiveJob::Base.queue_adapter = :test ActiveJob::Base.logger = ActiveSupport::Logger.new(nil) -# Filter out the backtrace from minitest while preserving the one from other libraries. -Minitest.backtrace_filter = Minitest::BacktraceFilter.new - SERVICE_CONFIGURATIONS = begin ActiveSupport::ConfigurationFile.parse(File.expand_path("service/configurations.yml", __dir__)).deep_symbolize_keys rescue Errno::ENOENT diff --git a/railties/lib/minitest/rails_plugin.rb b/railties/lib/minitest/rails_plugin.rb index e0c7a68d13..660066bab6 100644 --- a/railties/lib/minitest/rails_plugin.rb +++ b/railties/lib/minitest/rails_plugin.rb @@ -5,6 +5,19 @@ require "rails/test_unit/reporter" require "rails/test_unit/runner" module Minitest + class BacktraceFilterWithFallback + def initialize(preferred, fallback) + @preferred = preferred + @fallback = fallback + end + + def filter(backtrace) + filtered = @preferred.filter(backtrace) + filtered = @fallback.filter(backtrace) if filtered.empty? + filtered + end + end + class SuppressedSummaryReporter < SummaryReporter # Disable extra failure output after a run if output is inline. def aggregated_results(*) @@ -35,24 +48,14 @@ module Minitest options[:output_inline] = true end - class RailsBacktraceCleanerDecorator - def initialize(backtrace_cleaner) - @backtrace_cleaner = backtrace_cleaner - end - - def filter(backtrace) - filtered = @backtrace_cleaner.filter(backtrace) - filtered = backtrace.dup if filtered.empty? - filtered - end - end - # Owes great inspiration to test runner trailblazers like RSpec, # minitest-reporters, maxitest and others. def self.plugin_rails_init(options) unless options[:full_backtrace] || ENV["BACKTRACE"] # Plugin can run without Rails loaded, check before filtering. - Minitest.backtrace_filter = RailsBacktraceCleanerDecorator.new(::Rails.backtrace_cleaner) if ::Rails.respond_to?(:backtrace_cleaner) + if ::Rails.respond_to?(:backtrace_cleaner) + Minitest.backtrace_filter = BacktraceFilterWithFallback.new(::Rails.backtrace_cleaner, Minitest.backtrace_filter) + end end # Suppress summary reports when outputting inline rerun snippets. diff --git a/railties/lib/rails/generators/rails/plugin/templates/test/test_helper.rb.tt b/railties/lib/rails/generators/rails/plugin/templates/test/test_helper.rb.tt index 4f7a8d3d6e..d0e863fb6d 100644 --- a/railties/lib/rails/generators/rails/plugin/templates/test/test_helper.rb.tt +++ b/railties/lib/rails/generators/rails/plugin/templates/test/test_helper.rb.tt @@ -10,9 +10,6 @@ ActiveRecord::Migrator.migrations_paths << File.expand_path('../db/migrate', __d <% end -%> require "rails/test_help" -# Filter out the backtrace from minitest while preserving the one from other libraries. -Minitest.backtrace_filter = Minitest::BacktraceFilter.new - <% unless engine? -%> require "rails/test_unit/reporter" Rails::TestUnitReporter.executable = 'bin/test' diff --git a/railties/test/application/test_runner_test.rb b/railties/test/application/test_runner_test.rb index f115ca80a6..95a75c519e 100644 --- a/railties/test/application/test_runner_test.rb +++ b/railties/test/application/test_runner_test.rb @@ -511,28 +511,6 @@ module ApplicationTests end end - def test_shows_filtered_backtrace_by_default - create_backtrace_test - - assert_match "Minitest::RailsBacktraceCleanerDecorator", run_test_command("test/unit/backtrace_test.rb") - end - - def test_backtrace_option - create_backtrace_test - - assert_match "Minitest::BacktraceFilter", run_test_command("test/unit/backtrace_test.rb -b") - assert_match "Minitest::BacktraceFilter", - run_test_command("test/unit/backtrace_test.rb --backtrace") - end - - def test_show_full_backtrace_using_backtrace_environment_variable - create_backtrace_test - - switch_env "BACKTRACE", "true" do - assert_match "Minitest::BacktraceFilter", run_test_command("test/unit/backtrace_test.rb") - end - end - def test_run_app_without_rails_loaded # Simulate a real Rails app boot. app_file "config/boot.rb", <<-RUBY @@ -956,18 +934,6 @@ module ApplicationTests RUBY end - def create_backtrace_test - app_file "test/unit/backtrace_test.rb", <<-RUBY - require "test_helper" - - class BacktraceTest < ActiveSupport::TestCase - def test_backtrace - puts Minitest.backtrace_filter - end - end - RUBY - end - def create_schema app_file "db/schema.rb", "" end diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb index b33152d158..328b32a26d 100644 --- a/railties/test/generators/plugin_generator_test.rb +++ b/railties/test/generators/plugin_generator_test.rb @@ -71,7 +71,6 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "test/test_helper.rb" do |content| assert_match(/require_relative.+test\/dummy\/config\/environment/, content) assert_match(/ActiveRecord::Migrator\.migrations_paths.+test\/dummy\/db\/migrate/, content) - assert_match(/Minitest\.backtrace_filter = Minitest::BacktraceFilter\.new/, content) assert_match(/Rails::TestUnitReporter\.executable = 'bin\/test'/, content) end assert_file "lib/bukkits/railtie.rb", /module Bukkits\n class Railtie < ::Rails::Railtie\n end\nend/ diff --git a/railties/test/minitest/rails_plugin_test.rb b/railties/test/minitest/rails_plugin_test.rb index 7c3a2022a9..4459c6d85b 100644 --- a/railties/test/minitest/rails_plugin_test.rb +++ b/railties/test/minitest/rails_plugin_test.rb @@ -1,42 +1,112 @@ # frozen_string_literal: true require "abstract_unit" +require "env_helpers" class Minitest::RailsPluginTest < ActiveSupport::TestCase + include EnvHelpers + setup do - @options = Minitest.process_args [] @output = StringIO.new("".encode("UTF-8")) end - test "default reporters are replaced" do - with_reporter Minitest::CompositeReporter.new do |reporter| - reporter << Minitest::SummaryReporter.new(@output, @options) - reporter << Minitest::ProgressReporter.new(@output, @options) - reporter << Minitest::Reporter.new(@output, @options) + test "replaces backtrace filter with one that silences gem lines" do + backtrace = ["lib/my_code.rb", backtrace_gem_line("rails")] - Minitest.plugin_rails_init({}) - - assert_equal 3, reporter.reporters.count - assert reporter.reporters.any? { |candidate| candidate.kind_of?(Minitest::SuppressedSummaryReporter) } - assert reporter.reporters.any? { |candidate| candidate.kind_of?(::Rails::TestUnitReporter) } - assert reporter.reporters.any? { |candidate| candidate.kind_of?(Minitest::Reporter) } + with_plugin do + assert_equal backtrace.take(1), Minitest.backtrace_filter.filter(backtrace) end end - test "no custom reporters are added if nothing to replace" do - with_reporter Minitest::CompositeReporter.new do |reporter| - Minitest.plugin_rails_init({}) + test "replacement backtrace filter never returns an empty backtrace" do + backtrace = [backtrace_gem_line("rails")] - assert_empty reporter.reporters + with_plugin do + assert_equal backtrace, Minitest.backtrace_filter.filter(backtrace) + end + end + + test "replacement backtrace filter silences Minitest lines when all lines are gem lines" do + backtrace = [backtrace_gem_line("rails"), backtrace_gem_line("minitest")] + + with_plugin do + assert_equal backtrace.take(1), Minitest.backtrace_filter.filter(backtrace) + end + end + + test "does not replace backtrace filter when using --backtrace option" do + backtrace_filter = baseline_backtrace_filter + + with_plugin("--backtrace", initial_backtrace_filter: backtrace_filter) do + assert_same backtrace_filter, Minitest.backtrace_filter + end + + with_plugin("-b", initial_backtrace_filter: backtrace_filter) do + assert_same backtrace_filter, Minitest.backtrace_filter + end + end + + test "does not replace backtrace filter when BACKTRACE environment variable is set" do + backtrace_filter = baseline_backtrace_filter + + switch_env "BACKTRACE", "true" do + with_plugin(initial_backtrace_filter: backtrace_filter) do + assert_same backtrace_filter, Minitest.backtrace_filter + end + end + end + + test "replaces Minitest::SummaryReporter reporter" do + with_plugin do + assert_empty Minitest.reporter.reporters.select { |reporter| reporter.instance_of? Minitest::SummaryReporter } + assert_not_empty Minitest.reporter.reporters.grep(Minitest::SuppressedSummaryReporter) + end + end + + test "replaces Minitest::ProgressReporter reporter" do + with_plugin do + assert_empty Minitest.reporter.reporters.grep(Minitest::ProgressReporter) + assert_not_empty Minitest.reporter.reporters.grep(::Rails::TestUnitReporter) + end + end + + test "keeps non-default reporters" do + custom_reporter = Minitest::Reporter.new(@output) + + with_plugin(initial_reporters: [custom_reporter]) do + assert_includes Minitest.reporter.reporters, custom_reporter + end + end + + test "does not add reporters when not replacing reporters" do + with_plugin(initial_reporters: []) do + assert_empty Minitest.reporter.reporters end end private - def with_reporter(reporter) - old_reporter, Minitest.reporter = Minitest.reporter, reporter + def baseline_backtrace_filter + Minitest::BacktraceFilter.new + end - yield reporter + def baseline_reporters + [Minitest::SummaryReporter.new(@output), Minitest::ProgressReporter.new(@output)] + end + + def with_plugin(*args, initial_backtrace_filter: baseline_backtrace_filter, initial_reporters: baseline_reporters) + original_backtrace_filter, Minitest.backtrace_filter = Minitest.backtrace_filter, initial_backtrace_filter + original_reporter, Minitest.reporter = Minitest.reporter, Minitest::CompositeReporter.new(*initial_reporters) + + options = Minitest.process_args(args) + Minitest.plugin_rails_init(options) + + yield ensure - Minitest.reporter = old_reporter + Minitest.backtrace_filter = original_backtrace_filter + Minitest.reporter = original_reporter + end + + def backtrace_gem_line(gem_name) + caller.grep(%r"/lib/minitest\.rb:").first.gsub("minitest", gem_name) end end