raise ArgumentError if object cannot be proxied

[zhisme]

fixes #1356

[zhisme]

@pirj can you take a look?

however I cannot understand why travis CI failed, I will try to reproduce with the given ruby versions on localhost to identify what's wrong

[JonRowe]

Thanks for tackling this, some feedback

[zhisme]

thanks @JonRowe will update

[zhisme]

I couldn't find changelog for ruby 2.0.0, but surely in previous ruby versions symbols where not frozen by default, that's why travis was failing for older ruby versions.
consider this example, I reproduced and fixed with ruby 1.9.3

irb(main):001:0> RUBY_VERSION
=> "1.9.3"
irb(main):002:0> :subject.frozen?
=> false

[zhisme]

@pirj @JonRowe and I see on another PR #1349 you planning to drop ruby < 2.3, maybe there is no need to adjust this code to be compatible with older ruby versions. what do you think?

[JonRowe]

@zhisme If you wish to target this off 4-0-dev for release in 4.0.0 you are welcome to drop the compatibility code, we plan to release around the launch of Ruby 3.0.0 but there is no guaranteed timeline for it. I will probably backport this to main however for release in a bugpatch in the 3.x series.

[zhisme]

@JonRowe any idea of travis-ci build error?
And other seems like failed due to this one. Need your help

Failure/Error: require 'thread_order'
LoadError:
  cannot load such file -- thread_order

[JonRowe]

Thanks, don't worry about Travis we're phasing it out as its become really unreliable since they essentially dropped open source.

[keegangroth]

Hi, I've just pulled this in and am getting test failures in areas where I've stubbed destroyed ActiveRecord objects, which freezes them. I'm not understanding why rspec should refuse to stub these objects and I'm not seeing any explanation here, but hoping someone could explain it to me?

[JonRowe]

@keegangroth Frozen objects cannot be modified, which is required to stub them.

[keegangroth]

OK, I think there's something specific to ActiveRecord here, because old and new versions of rspec both throw errors for Hash objects. I'll see if I can come up with a trivial example for activerecord. Here's an example with Hash that behaves more or less the same both before and after this change.

describe 'frozen stubbing' do
  let(:h) { Hash.new }
  let(:error) { StandardError.new 'mine' }

  context 'expect_any_instance_of' do
    before do
      expect_any_instance_of(Hash).to receive(:map).and_raise(error)
    end

    # fails with either
    # expected #<StandardError: mine>, got #<ArgumentError: Cannot proxy frozen objects, rspec-mocks relies on proxies for method stubbing and expectations. Hash: {}>
    # or 
    # expected #<StandardError: mine>, got #<FrozenError: can't modify frozen class/module>
    it 'fails' do
      h.freeze
      expect{h.map}.to raise_error(error)
    end
  end

  context 'instance stub' do
    before do
      expect(h).to receive(:map).and_raise(error)
    end

    it 'works' do
      h.freeze
      expect{h.map}.to raise_error(error)
    end
  end
end

[keegangroth]

The behavior has definitely changed for destroyed ActiveRecord Models. I don't know what makes their version of freezing different from hash. Trivial example using rails to do the db setup:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "6.1.0"
  gem "rspec", "3.10.0"
  #gem "rspec-mocks", "3.10.0" # works find
  gem "rspec-mocks", "3.10.1" # fails
  gem "activerecord-jdbcsqlite3-adapter", "61.0", platform: :jruby
  gem "sqlite3", platform: :mri
end

require "active_record"
require "rspec"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :posts, force: true
end

class Post < ActiveRecord::Base
  def my_method
    10
  end
end

RSpec.describe Post do
  let(:p) { described_class.create! }
  let(:error) { StandardError.new 'mine' }

  context 'expect_any_instance_of' do
    before do
      expect_any_instance_of(described_class).to receive(:my_method).and_raise(error)
    end

    it 'fails' do
      p.destroy! # "freezes" the instance
      expect{p.my_method}.to raise_error(error)
    end
  end

  context 'instance stub' do
    before do
      expect(p).to receive(:my_method).and_raise(error)
    end

    it 'works' do
      p.destroy! # "freezes" the instance
      expect{p.my_method}.to raise_error(error)
    end
  end
end

result on rspec-mocks 3.10.0:

Finished in 0.47361 seconds (files took 5.57 seconds to load)
2 examples, 0 failures

result on rspec-mocks 3.10.1:

Failures:

  1) Post expect_any_instance_of fails
     Failure/Error: expect{p.my_method}.to raise_error(error)

       expected #<StandardError: mine>, got #<ArgumentError: Cannot proxy frozen objects, rspec-mocks relies on proxies for method stubbing and expectations.> with backtrace:
         # ./freeze_spec.rb:46:in `block in <main>'
         # ./freeze_spec.rb:46:in `block in <main>'
     # ./freeze_spec.rb:46:in `block in <main>'

Finished in 0.17197 seconds (files took 5.14 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./freeze_spec.rb:44 # Post expect_any_instance_of fails

Probably warrants it's own (new) issue? Kind of niche, but it's now a major headache to test certain kinds of functionality in active record (I actually haven't yet figured out the work around for my particular case).

[flugsio]

The activerecord models are not really frozen, they override that method and delegates it to the attributes, which are frozen when the record is destroyed. https://github.com/rails/rails/blob/914caca2d31bd753f47f9168f2a375921d9e91cc/activerecord/lib/active_record/core.rb#L618

def frozen?
  @attributes.frozen?
end

I'm not sure if there's a better way to check the real frozen status for the model itself.

[JonRowe]

Feel free to raise that as a bug with the Rails team, if an object isn't actually frozen it probably shouldn't return that it is.

[keegangroth]

I don't mind raising this with rails, but does any one really think they're going to change their interface for this? Seems like there are some pretty simple alternatives here in rspec such as just catching the FrozenError (which looks like it was always raised in these cases) and re-raising with the appropriate message.

[JonRowe]

They might not be aware they are breaking an expected Ruby behaviour, they might have a reason for it being the way it is, I don't know, but currently these objects don't behave as they should by the standards Ruby sets, its worth finding out if this is blocking you, equally If you'd like to create an alternative implementation for this warning feel free.

[keegangroth]

#1401 for what it's worth