Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FixtureSupport#run_in_transaction? #2495

Merged
merged 1 commit into from Apr 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Changelog.md
Expand Up @@ -5,6 +5,8 @@ Enhancements:

* Make the API request scaffold template more consistent and compatible with
Rails 6.1. (Naoto Hamada, #2484)
* Provide support to turn off transactional tests for select examples.
(Stan Lo, #2495)

### 5.0.1 / 2021-03-18
[Full Changelog](https://github.com/rspec/rspec-rails/compare/v5.0.0...v5.0.1)
Expand Down
3 changes: 2 additions & 1 deletion lib/rspec/rails/fixture_support.rb
Expand Up @@ -13,7 +13,8 @@ module FixtureSupport
# Monkey patched to avoid collisions with 'let(:name)' in Rails 6.1 and after
# and let(:method_name) before Rails 6.1.
def run_in_transaction?
st0012 marked this conversation as resolved.
Show resolved Hide resolved
use_transactional_tests && !self.class.uses_transaction?(self)
current_example_name = (RSpec.current_example && RSpec.current_example.metadata[:description])
pirj marked this conversation as resolved.
Show resolved Hide resolved
use_transactional_tests && !self.class.uses_transaction?(current_example_name)
end

included do
Expand Down
27 changes: 27 additions & 0 deletions spec/rspec/rails/fixture_support_spec.rb
Expand Up @@ -12,6 +12,33 @@ module RSpec::Rails
end
end

context "with use_transactional_tests set to true" do
it "works with #uses_transaction helper" do
group = RSpec::Core::ExampleGroup.describe do
include FixtureSupport
self.use_transactional_tests = true

uses_transaction "doesn't run in transaction"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha. So uses_transaction is a way to turn use_transactional_tests for select examples?
I missed this feature so much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not documented. But yes, based on the code and the current behavior it turns that off for the given tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its not documented I'm hesitant to support it, if its public api fine but private api I'm a bit reluctant about

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public https://api.rubyonrails.org/classes/ActiveRecord/TestFixtures/ClassMethods.html#method-i-uses_transaction, but undocumented. Originally introduced here.

Frankly, a couple of times I needed it desperately to test code that needs to have control of the outermost transaction or has a conditional statement with a check if the transaction is an outermost or a nested.

With DatabaseCleaner it's possible to turn off per-example transaction for select examples. With use_transactional_fixtures it's not, and it's a major inconvenience.
I was under the spell that it was, and it took me a while to realize that it didn't work like that on my last project. Our documentation lacks this moment.


it "doesn't run in transaction" do
expect(ActiveRecord::Base.connection.transaction_open?).to eq(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a big WTF moment at this, so uses_transaction turns off transactional tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. No doubt uses_transaction is an ambiguous and confusing naming.

end

it "runs in transaction" do
expect(ActiveRecord::Base.connection.transaction_open?).to eq(true)
end
end

expect_to_pass(group)
end

def expect_to_pass(group)
result = group.run(failure_reporter)
failure_reporter.exceptions.map { |e| raise e }
expect(result).to be true
end
end
st0012 marked this conversation as resolved.
Show resolved Hide resolved

it "will allow #setup_fixture to run successfully", skip: Rails.version.to_f <= 6.0 do
group = RSpec::Core::ExampleGroup.describe do
include FixtureSupport
Expand Down