SetupAndTeardown#teardown should call any subsequent after_teardown:
#32472
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @pixeltrix (@rails-bot has picked a reviewer for you, use r? to override) |
Edouard-chin
force-pushed
the
ec-activesupport-teardown
branch
from
de47f58
to
8fe2867
Compare
If you have a regular test that have a teardown block, and for any reason an exception get raised, ActiveSupport will not run subsequent after_teardown method provided by other module or gems. One of them being the ActiveRecord::TestFixtures which won't rollback the transation when the test ends making all subsequent test to be in a weird state. The default implementation of minitest is to run all teardown methods from the user's test, rescue all exceptions, run all after_teardown methods provided by libraries and finally re-raise the exception that happened in the user's teardown method. Rails should do the same.
Edouard-chin
force-pushed
the
ec-activesupport-teardown
branch
from
8fe2867
to
70fae9a
Compare
kirs
reviewed
Apr 6, 2018
| begin | ||
| run_callbacks :teardown | ||
| rescue => e | ||
| error = e |
There was a problem hiding this comment.
What if multiple teardown callbacks would raise?
There was a problem hiding this comment.
The first one would halt the execution of the others. Like it would happen if we were using def teardown instead of the teardown callback.
👍 for doing the same as MiniTest. |
rafaelfranca
reviewed
Apr 6, 2018
| begin | ||
| run_callbacks :teardown | ||
| rescue => e | ||
| error = e |
There was a problem hiding this comment.
The first one would halt the execution of the others. Like it would happen if we were using def teardown instead of the teardown callback.
|
Oh, forget about it, |
rafaelfranca
requested changes
Apr 6, 2018
| end | ||
| end | ||
|
|
||
| class SetupAndTeardownTest < Minitest::Test |
There was a problem hiding this comment.
We have a class with the same name in activesupport/test/test_case_test.rb. We need to rename this class.
rafaelfranca
approved these changes
Apr 6, 2018
rafaelfranca
added a commit
that referenced
this pull request
Apr 6, 2018
`SetupAndTeardown#teardown` should call any subsequent after_teardown:
rafaelfranca
added a commit
that referenced
this pull request
Apr 6, 2018
`SetupAndTeardown#teardown` should call any subsequent after_teardown:
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Apr 27, 2018
- In rails#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](https://github.com/rails/rails/blob/7d2400ab61c8e3ed95e14d03ba3844e8ba2e36e4/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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SetupAndTeardown#teardownshould call any subsequent after_teardown:If you have a regular test that have a teardown block, and for any reason an exception get raised,
ActiveSupport will not run subsequent after_teardown method provided by other module or gems.
One of them being the ActiveRecord::TestFixtures which won't rollback the transation when the test
ends making all subsequent test to be in a weird state.
The default implementation of minitest is to run all teardown methods from the user's test, rescue all
exceptions, run all after_teardown methods provided by libraries and finally re-raise the exception
that happened in the user's teardown method.
Rails should do the same.