Bug when using `to_s` as a subject name

[corroded]

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.6.6
Rails version: 6.0.3.5
RSpec version: 3.10.1

Observed behaviour

We have dependabot and the recent upgrade failed some of our specs. Particularly specs that specifically test the method to_s on an object. An example:

AR Object

class SomeActiveRecordObject < ApplicationRecord
  validates :name, presence: true
  delegate :to_s, to: :name
end

Spec

RSpec.describe SomeActiveRecordObject do
  subject(:some_active_record_object) { described_class.new }

  describe '#to_s' do
    subject(:to_s) { some_active_record_object.to_s }

    before { some_active_record_object.name = 'new name' }

    it 'delegates to name' do
      expect(some_active_record_object.to_s).to eq 'new name'
    end
  end
end

Expected behaviour

The expected behaviour is for the spec to pass seeing it's very simple. This spec fails with the new code because name is blank in the beginning and is somehow being memoised already.

Can you provide an example app?

The above already shows the example. Further more, we already have an idea on what the bug is but not sure how to fix it.

This is the specific commit that breaks it: https://github.com/rspec/rspec-rails/pull/2461/files

More specifically this change:

        def run_in_transaction?
          use_transactional_tests && !self.class.uses_transaction?(self)
        end

The issue is when the subject is named to_s (as in subject(:to_s) { something.to_s }), uses_transaction somehow uses the actual active record object - instead of the RSpec example.

This is the "offending line": https://github.com/rails/rails/blob/df41acdad93783dcac49f03036c3f34cb1c6c667/activerecord/lib/active_record/test_fixtures.rb#L96

      def uses_transaction?(method)
        @uses_transaction = [] unless defined?(@uses_transaction)
        @uses_transaction.include?(method.to_s) # for some reason "method" here becomes the AR object - so it 'memoises' the value already even before you hit the before block
      end
    end

I have added an example spec to the existing spec for name collision - I think it's related there. Here is the sample:

main...corroded:main

Final note

You don't actually need to inherit from AR for this to happen - but you do need to have included include ActiveRecord::TestFixtures since it's that method that is the problem. Not sure if it's an rspec-rails issue or a bug with active-record fixtures (or both!)

[JonRowe]

Your example spec doesn't actually use subject there at all (you would have to call subject or to_s to trigger it).

Subjects however, are methods, to_s is a special method for Ruby, as it's used to cast objects to strings, so it's possible RSpec is calling to_s on the example to print something that is obscuring the real cause, can you change the name to find the real error?

[corroded]

Your example spec doesn't actually use subject there at all (you would have to call subject or to_s to trigger it).

That's the thing. From my understanding to_s is lazily assigned and the subject won't run until you call subject or to_s as you mentioned, but that line uses_transaction? apparently calls that subject instead. method is SUPPOSED to be an RSpec example - but in this case, it becomes whatever the subject was i.e. some_active_record_object.

If you look at the sample spec I added, if you change the name of the subject to to_ss it will pass. It is specifically if the name is to_s that it fails

[corroded]

@JonRowe Here is an example screenshot of a pry I did to illustrate (see memoized_helpers.rb:339 -> active_record/test_fixtures.rb:94)

See how method is supposed to be an RSpec::ExampleGroups::ToS but calling to_s on it makes it the actual subject?

If I change the subject name to to_ss it never reaches that uses_transaction? part (from memoized_helpers.rb:339 -> memoized_helpers.rb:121)

[pirj]

We have a germinal concept of reserved memoized helper variable names here https://github.com/rspec/rspec-core/blob/ea8554afd1a2b63677c6593059fa8f2476181deb/lib/rspec/core/memoized_helpers.rb#L311

I suggest extending the list from just :initialize to include :to_s as well.

What's being left out is the check for names passed as String, e.g.:

RSpec.describe 'A' do
  let('initialize') { 1 }
  it { expect(initialize).to eq(1) }
end

does not result in a sensible error,

RuntimeError:
  #let or #subject called with a reserved name #initialize

but rather

/home/pirj/source/rspec-core/lib/rspec/core/memoized_helpers.rb:268:in `block in let': wrong number of arguments (given 1, expected 0) (ArgumentError)

Would you like to fix those two problems with a small PR, @corroded ?

:hash might be problematic, too, but it would be a breaking change fixing an imaginary problem, so probably not now.

[tomdalling]

I think this is solved pretty well by just adding a nice error message about reserved names. The main problem is how difficult it is to debug, and an error message will make it easy.