Fix teardown callbacks

[ttstarck]

Motivation / Background

This Pull Request has been created because ActiveRecord::TestFixtures.teardown_fixtures method does not get called if a MiniTest::Assertion error is raised in ActiveSupport::TestCase.teardown callbacks.

Test Case to Reproduce the Problem

class DrinkTest < ActiveSupport::TestCase
  teardown do
    if @failure_test
      flunk
    end
  end

  test "should fail" do
    @failure_test = true
    Drink.create!(name: "test")
  end

  test "should pass" do
    assert_equal 0, Drink.where(name: "test").size
  end
end

Running the above test case with a seed that runs should fail and then should pass:

bundle exec ruby test/models/drink_test.rb
Running 2 tests in a single process (parallelization threshold is 50)
Run options: --seed 6897

# Running:

F

Failure:
DrinkTest#test_should_fail [test/models/drink_test.rb:6]:
Epic Fail!


bin/rails test test/models/drink_test.rb:10

F

Failure:
DrinkTest#test_should_pass [test/models/drink_test.rb:16]:
Expected: 0
  Actual: 1


bin/rails test test/models/drink_test.rb:15



Finished in 0.016991s, 117.7094 runs/s, 117.7094 assertions/s.
2 runs, 2 assertions, 2 failures, 0 errors, 0 skips

Running the test file with a seed with the following order: should pass -> should fail:

bundle exec ruby test/models/drink_test.rb
Running 2 tests in a single process (parallelization threshold is 50)
Run options: --seed 3383

# Running:

.F

Failure:
DrinkTest#test_should_fail [test/models/drink_test.rb:6]:
Epic Fail!


bin/rails test test/models/drink_test.rb:10



Finished in 0.016423s, 121.7804 runs/s, 121.7804 assertions/s.
2 runs, 2 assertions, 1 failures, 0 errors, 0 skips

Specifications

ruby: 3.2.2
rails: 7.1.1
minitest: 5.21.0
OS: MacOS arm64-darwin
Database: SQLite

Detail

This Pull Request changes the following files:

ActiveSupport::Testing::SetupAndTeardown

Renames the following class methods in

• after_teardown -> teardown
• before_setup -> setup.

Why switch from after_teardown to teardown and before_setup to setup?

When a user defines a teardown callback block in their test case, it will actually run in the after_teardown Minitest Lifecycle hook. This is confusing and has a major side effect that if a user defines an assertion in that teardown code, it will raise out without getting rescued (see the calling code). This means super will never get called, ActiveRecord::TestFixtures.teardown_fixtures will never run, and the database state will not get rolled back.

By moving the teardown callbacks to run in the teardown lifecycle, all after_teardown code will run since the error will get rescued by Minitest's lifecycle management:

• Lifecycle hooks
• capture_exceptions

Minitest also provides recommendations about when to use after_teardown and before_setup:
https://docs.seattlerb.org/minitest/Minitest/Test/LifecycleHooks.html

after_teardown()
Runs after every test, after teardown. This hook is meant for libraries to extend minitest. It is not meant to be used by test developers.

There is a slight grey area here, where the setup and teardown callbacks are code written and provided by the "test developers". It was surprising at least to me to find that they were not run in setup and teardown Minitest Lifecycle hooks but rather before_setup and after_teardown.

Moving the callbacks to run in their respective Minitest Lifecycle hooks properly will follow Minitest's recommendations and clear up any confusion about where these callbacks are run.

ActiveRecord::TestFixture.after_teardown

• Wrap teardown_fixtures in ensure block to always run the code that rolls back the test's database transactions.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[Edouard-chin]

it will raise out without getting rescued (see the calling code)

We actually do rescue any exceptions

rails/activesupport/lib/active_support/testing/setup_and_teardown.rb

Lines 47 to 49 in 979c6b0

	rescue => e
	self.failures << Minitest::UnexpectedError.new(e)
	end

This was fixed some time ago in #32472 , maybe you are using a old Rails version? I tried with your reproduction script in a new app and I couldn't reproduce the problem

[ttstarck]

@Edouard-chin here is the specifications I'm using to run the test case I posted above:

ruby: 3.2.2
rails: 7.1.1
minitest: 5.21.0
OS: MacOS arm64-darwin
Database: SQLite

I have just tested the code you linked in a rails/console session via the following:

require 'minitest'
begin
  raise Minitest::Assertion, "assertion failed"
rescue => ex
  puts "Exception #{ex} was caught"
end

The results:

irb(main):002* begin
irb(main):003*   raise Minitest::Assertion, "assertion failed"
irb(main):004* rescue => ex
irb(main):005*   puts "Exception #{ex} was caught"
irb(main):006> end
(irb):3:in `<main>': assertion failed (Minitest::Assertion)

The exception is not caught and is raised out.

I believe this is because: rescue => ex is equivalent to the following: rescue StandardError => ex

Minitest::Assertion inherits from Exception (code), not StandardError so that general rescue => ex clauses don't catch it.

[rafaelfranca]

This change can't be done. It is breaking a lot of tests, as you can see for all the files you had to change. The commit message for this change also is wrong. It say we can't use those hooks, but the hook documentation says clearly: This hook is meant for libraries to extend minitest. We are a library and we are extending minitest

[ttstarck]

Thanks for the review! I think you're right, I didn't realize how much of a breaking change that would be.

I believe I have a more elegant solution that shouldn't break anything that is ready for review!

There are two test thread failures in actionpack that seem to be flakey tests, and not related to my changes. I have not been able to reproduce locally, but I also cannot retry the step in Buildkite

[Edouard-chin]

I believe this is because: rescue => ex is equivalent to the following: rescue StandardError => ex

Thanks, you are right, I missed that.

[ttstarck]

@Edouard-chin @rafaelfranca

I have reverted the changes such that setup and teardown callbacks are unchanged in which Minitest lifecycle hook they are run in.

The new changes are:

ActiveSupport::TestCase.after_teardown

• Catch Minitest::Assertion exceptions raised by teardown callbacks such that other after_teardown methods in the super chain can be called.

Since this change is now much smaller, I have not updated the changelogs per the PR template note:

Minor bug fixes and documentation changes should not be included.

[ttstarck]

Looking into the actionpack:isolated (3.3) step failure:

InheritedEtagRenderTest#test_etag_reflects_template_digest
undefined method `_implicit_render_test_hello_world_erb___3011208120951429117_8300' for an instance of #Class:0x00007fb534d237a0

I have found this run in main branch builds that is failing in the same manner:
https://buildkite.com/rails/rails/builds/103798#018cfa06-cb26-469b-99c0-38a80b208f4e

Unless someone believes otherwise, I will consider this failure not related to my changes.

[ttstarck]

@rafaelfranca @Edouard-chin Thank you both for the reviews and getting this merged upstream!