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

Conversation

st0012
Copy link
Contributor

@st0012 st0012 commented Apr 19, 2021

ActiveRecord::TestFixture's uses_transaction is designed to be used like this:

uses_transaction :the_test_method_name

And in RSpec, the method name would be the example's name.

uses_transaction "does someting"

it "does someting" {}

But in the current implementation, it's passing the example object instead of its name, which would always fail the name comparison in https://github.com/rails/rails/blob/main/activerecord/lib/active_record/test_fixtures.rb#L94-L97

So this PR fixes the issue by passing the example's name instead of the example object.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

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.

spec/rspec/rails/fixture_support_spec.rb Show resolved Hide resolved
lib/rspec/rails/fixture_support.rb Show resolved Hide resolved
lib/rspec/rails/fixture_support.rb Show resolved Hide resolved
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

All looks good.
Thanks a lot for this contribution.

spec/rspec/rails/fixture_support_spec.rb Outdated Show resolved Hide resolved
lib/rspec/rails/fixture_support.rb Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented Apr 21, 2021

Ruby: 2.5.8, Rails: ~> 5.2.0
GitHub Actions has encountered an internal error when running your job.

🤷‍♂️

Re-running.

uses_transaction "doesn't run in transaction"

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.

@JonRowe
Copy link
Member

JonRowe commented Apr 28, 2021

@pirj I'll leave any decisions on this up to you, my feedback is non blocking

`ActiveRecord::TestFixture`'s `uses_transaction` is designed to be used
like this:

```ruby
uses_transaction :the_test_method_name
```

And in RSpec, the method name would be the example's name.

```ruby
uses_transaction "does someting"

it "does someting" {}
```

But in the current implementation, it's passing the example object
instead of its name, which would always fail the name comparison in
https://github.com/rails/rails/blob/adc0146a07ccc72405aec78ccb65aac3502a4300/activerecord/lib/active_record/test_fixtures.rb#L94-L97

So this commit fixes the issue by passing the example's name instead of
the example object.
@pirj pirj merged commit ab2ea2b into rspec:main Apr 28, 2021
@pirj
Copy link
Member

pirj commented Apr 28, 2021

Thank you for the contribution and for revealing the mystery behind uses_transaction, @st0012 !

@st0012
Copy link
Contributor Author

st0012 commented Apr 29, 2021

@pirj I've been using rspec-rails for years and it's finally my turn to make a small contribution 🙂

@st0012 st0012 deleted the fix-uses_transaction branch April 29, 2021 02:59
JonRowe added a commit that referenced this pull request Apr 29, 2021
JonRowe pushed a commit that referenced this pull request Apr 29, 2021
Fix FixtureSupport#run_in_transaction?
JonRowe added a commit that referenced this pull request Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants