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

Prevent collisions on let(:name) and let(:method_name) #2461

Merged
merged 3 commits into from Feb 20, 2021

Conversation

benoittgt
Copy link
Member

Fix: #2451

Two commits. One to see the failures then one to see the fix. I did nothing specific, just implement the great work of @grekko and the choice from @JonRowe.

# 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?
use_transactional_tests && !self.class.uses_transaction?(@example)

Choose a reason for hiding this comment

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

Not “reviewing,” just curious - were you able to determine whether any RSpec code still sets @example to a meaningful value?

Copy link
Member Author

Choose a reason for hiding this comment

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

No I didn't search this yet. But I will.

Copy link
Member

Choose a reason for hiding this comment

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

For us this probably needs to be example or self as we don't assign @example within a test like mini test does afaik

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!

@pirj pirj added this to the 4.1 milestone Feb 18, 2021
@pirj pirj changed the title Prevent collisions on let(:name) and let(:method_name Prevent collisions on let(:name) and let(:method_name) Feb 18, 2021
@pirj
Copy link
Member

pirj commented Feb 19, 2021

Rebased. Green.
Apart from ruby: 2.2.10 with Rails 5-2-stable, probably flaky. Passed on pull_request trigger, failed on push.

@JonRowe JonRowe merged commit 9272757 into rails-6-1-dev Feb 20, 2021
@JonRowe JonRowe deleted the rails-6-1-issue-2451-dev branch February 20, 2021 09:12
JonRowe added a commit that referenced this pull request Feb 20, 2021
Prevent collisions on let(:name) and let(:method_name)
JonRowe added a commit that referenced this pull request Feb 20, 2021
pirj pushed a commit that referenced this pull request Feb 21, 2021
Prevent collisions on let(:name) and let(:method_name)
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec <4.0
until it was fixed with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec 3x with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec <4.0
until it was fixed with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec 3x with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
It seems like rails 6.1 broke rspec-rails.
RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461
so we skip rspec-rails >4 with rails 6.1 in CI
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
Some of the missing combinations in the matrix:

* Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails
* Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
Some of the missing combinations in the matrix:

* Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails
* Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
alexrothenberg added a commit to alexrothenberg/ammeter that referenced this pull request Feb 24, 2021
Some of the missing combinations in the matrix:

* Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails
* Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants