`SetupAndTeardown` has few caveats that breaks libraries:

- In #32472 I introduced a fix in order for all `after_teardown` method provided by libraries and Rails to run, even if the application's `teardown` method raised an error (That's the default minitest behavior). However this change wasn't enough and doesn't take in consideration the ancestors chain.
  If a library's module containing an `after_teardown` method get included after the `SetupAndTeardown` module (one example is the [ActiveRecord::TestFixtures module](7d2400ab61/activerecord/lib/active_record/fixtures.rb (L855-L856)), then the ancestors of the test class would look something like
  ```ruby
    class MyTest < ActiveSupport::TestCase
    end

    puts MyTest.ancestors # [MyTest, ActiveSupport::TestCase, ActiveRecord::TestFixtures, ActiveSupport::Testing::SetupAndTeardown]
  ```
  Any class/module in the ancestors chain that are **before** the `ActiveSupport::Testing::SetupAndTeardown` will behave incorrectly:
    - Their `before_setup` method will get called **after** all regular setup method
    - Their `after_teardown` method won't even get called in case an exception is raised inside a regular's test `teardown`

  A simple reproduction script of the problem here https://gist.github.com/Edouard-chin/70705542a59a8593f619b02e1c0a188c

- One solution to this problem is to have the `AS::SetupAndTeardown` module be the very first in the ancestors chain. By doing that we ensure that no `before_setup` / `after_teardown` get executed prior to running the teardown callbacks
This commit is contained in:
Edouard CHIN 2018-04-26 14:38:02 -04:00
parent 7d2400ab61
commit 1cf8b6c231
3 changed files with 11 additions and 13 deletions

View File

@ -130,7 +130,7 @@ module ActiveSupport
alias_method :method_name, :name
include ActiveSupport::Testing::TaggedLogging
include ActiveSupport::Testing::SetupAndTeardown
prepend ActiveSupport::Testing::SetupAndTeardown
include ActiveSupport::Testing::Assertions
include ActiveSupport::Testing::Deprecation
include ActiveSupport::Testing::TimeHelpers

View File

@ -1,6 +1,5 @@
# frozen_string_literal: true
require "active_support/concern"
require "active_support/callbacks"
module ActiveSupport
@ -19,11 +18,10 @@ module ActiveSupport
# end
# end
module SetupAndTeardown
extend ActiveSupport::Concern
included do
include ActiveSupport::Callbacks
define_callbacks :setup, :teardown
def self.prepended(klass)
klass.include ActiveSupport::Callbacks
klass.define_callbacks :setup, :teardown
klass.extend ClassMethods
end
module ClassMethods
@ -47,12 +45,10 @@ module ActiveSupport
begin
run_callbacks :teardown
rescue => e
error = e
self.failures << Minitest::UnexpectedError.new(e)
end
super
ensure
raise error if error
end
end
end

View File

@ -4,13 +4,14 @@ require "abstract_unit"
module OtherAfterTeardown
def after_teardown
super
@witness = true
end
end
class AfterTeardownTest < Minitest::Test
class AfterTeardownTest < ActiveSupport::TestCase
include OtherAfterTeardown
include ActiveSupport::Testing::SetupAndTeardown
attr_writer :witness
@ -21,11 +22,12 @@ class AfterTeardownTest < Minitest::Test
end
def after_teardown
assert_raises MyError do
assert_changes -> { failures.count }, from: 0, to: 1 do
super
end
assert_equal true, @witness
failures.clear
end
def test_teardown_raise_but_all_after_teardown_method_are_called