rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 357 forks source link

Mock on ActiveRecord object is lost if several conditions apply (stopped working during ActiveRecord 6.1 -> 7.0 upgrade) #1490

Closed dmytro-savochkin closed 1 year ago

dmytro-savochkin commented 1 year ago

I am sorry if it's not the right place for this issue but so far I've failed to understand the exact reason of the fail so it's hard for me to understand where to report it (here? other rspec repo? rails repo? or maybe problem in my code?).

Subject of the issue

I am attempting an upgrade of our Rails 6.1 application that includes rspec 3.11 to Rails 7.0. During this upgrade I stumbled upon a spec in our code that is not working after the upgrade and I can't understand why. It seems like a combination of factors all put together prevent the mocked record to correctly understand it has been called. Why I believe it has been called is because I can see in the output the message inside dummy. (Well, actually I see it two times but the first one is printed before the transaction and setting of the expectation in the spec.)

Below is the setup. It's minimized as much as possible. Basically I have three models and when all the conditions (including a 'through' relation in model, 'after_initialize' callback, 'after_save_commit' callback, reload of model in spec, transaction in spec) are met the spec below fails in Rails 7.0 but passes in Rails 6.1. If even one of the conditions is not met (removed) then the spec passes.

script.rb

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  ruby "3.0.2"
  gem "activerecord", "7.0.4"
  gem "sqlite3", "1.5.3"
  gem "rspec", "3.11.0"
end

puts "Ruby version is: #{RUBY_VERSION}"

require "rspec/autorun"
require "active_record"

class CreateTables < ActiveRecord::Migration[6.1]
  def change
    drop_table :articles if table_exists? :articles
    drop_table :reviews if table_exists? :reviews
    drop_table :article_reviews if table_exists? :article_reviews
    create_table :articles
    create_table :reviews do |t|
      t.integer "desired_article_id"
    end
    create_table :article_reviews do |t|
      t.integer  "review_id"
      t.integer  "article_id"
    end
  end
end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "test.sqlite3")
CreateTables.new.change

class Review < ActiveRecord::Base
  belongs_to :desired_article, class_name: "Article", inverse_of: :review, optional: true
  has_one :article_review, inverse_of: :review
  has_one :article, through: :article_review

  after_initialize do
    self.desired_article = build_desired_article
  end

  after_save_commit :dummy
  def dummy
    puts 'inside dummy'
  end
end

class Article < ActiveRecord::Base
  has_one :review, foreign_key: "desired_article_id", inverse_of: :desired_article
  has_one :article_review
end

class ArticleReview < ActiveRecord::Base
  belongs_to :article, required: true
  belongs_to :review, required: true
end

RSpec.describe Review do
  it do
    record = Review.create!
    record.reload
    expect(record).to receive(:dummy)
    record.transaction do
      record.article = record.build_desired_article
      record.save!
    end
  end
end

Your environment

Steps to reproduce

Put the code from the script above to some file (e.g. script.rb) and run

ruby script.rb

To run it with Rails 6 just change gem "activerecord", "7.0.4" to gem "activerecord", "6.1.7".

Expected behavior

Spec passes. As it does with Active Record 6.1:

$ ruby script.rb
...
inside dummy
.

Finished in 0.03471 seconds (files took 0.42997 seconds to load)
1 example, 0 failures

Here 'inside dummy' is printed once because the second call is prevented when we overwrite record with a stub to spy on it for receiving of :dummy call

Actual behavior

Spec fails:

$ ruby script.rb
...
inside dummy
inside dummy
F

Failures:

  1) Review is expected to receive dummy(*(any args)) 1 time
     Failure/Error: expect(record).to receive(:dummy)

       (#<Review id: 1, desired_article_id: 2>).dummy(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # script.rb:66:in `block (2 levels) in <main>'

Finished in 0.0346 seconds (files took 0.44189 seconds to load)
1 example, 1 failure

Failed examples:

rspec script.rb:63 # Review is expected to receive dummy(*(any args)) 1 time
pirj commented 1 year ago

https://stackoverflow.com/questions/22988968/testing-after-commit-with-rspec-and-mocking

Does this help?

I recall using test_after_commit with transactional tests before it was fixed in Rails Might be a regression?

On 20 Oct 2022, at 20:38, Dmytro Savochkin @.***> wrote:

 I am sorry if it's not the right place for this issue but so far I've failed to understand the exact reason of the fail so it's hard for me to understand where to report it (here? other rspec repo? rails repo? or maybe problem in my code?).

Subject of the issue

I am attempting an upgrade of our Rails 6.1 application that includes rspec 3.11 to Rails 7.0. During this upgrade I stumbled upon a spec in our code that is not working after the upgrade and I can't understand why. It seems like a combination of factors all put together prevent the mocked record to correctly understand it has been called. Why I believe it has been called is because I can see in the output the message inside dummy. (Well, actually I see it two times but the second one is printed before the transaction and setting of the expectation in the spec.)

Below is the setup. It's minimized as much as possible. Basically I have three models and when all the conditions (including a 'through' relation in model, 'after_initialize' callback, 'after_save_commit' callback, reload of model in spec, transaction in spec) are met the spec below fails in Rails 7.0 but passes in Rails 6.1. If even one of the conditions is not met (removed) then the spec passes.

script.rb

require "bundler/inline"

gemfile(true) do source "https://rubygems.org" ruby "3.0.2" gem "activerecord", "7.0.4" gem "sqlite3", "1.5.3" gem "rspec", "3.11.0" end

puts "Ruby version is: #{RUBY_VERSION}"

require "rspec/autorun" require "active_record"

class CreateTables < ActiveRecord::Migration[6.1] def change drop_table :articles if table_exists? :articles drop_table :reviews if table_exists? :reviews drop_table :articles_reviews if table_exists? :articles_reviews create_table :articles create_table :reviews do |t| t.integer "desired_article_id" end create_table :articles_reviews do |t| t.integer "review_id" t.integer "article_id" end end end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "test.sqlite3") CreateTables.new.change

class Review < ActiveRecord::Base belongs_to :desired_article, class_name: "Article", inverse_of: :review, optional: true has_one :article_review, inverse_of: :review has_one :article, through: :article_review

after_initialize do self.desired_article = build_desired_article end

after_save_commit :dummy def dummy puts 'inside dummy' end end

class Article < ActiveRecord::Base has_one :review, foreign_key: "desired_article_id", inverse_of: :desired_article has_one :article_review end

class ArticleReview < ActiveRecord::Base belongs_to :article, required: true belongs_to :review, required: true end

RSpec.describe Review do it do record = Review.create! record.reload expect(record).to receive(:dummy) record.transaction do record.article = record.build_desired_article record.save! end end end Your environment

Ruby version: 3.0.2 rspec-mocks version: 3.11.1 or 3.11.0 Active Record version (working): 6.1.7 Active Record version (not working): 7.0.4 Steps to reproduce

Put the code from the script above to some file (e.g. script.rb) and run

ruby script.rb To run it with Rails 6 just change gem "activerecord", "7.0.4" to gem "activerecord", "6.1.7".

Expected behavior

Spec passes. As it does with Active Record 6.1:

$ ruby script.rb ... inside dummy ..

Finished in 0.03471 seconds (files took 0.42997 seconds to load) 1 example, 0 failures Here 'inside dummy' is printed once because the second call is prevented when we overwrite record with a stub to spy on it for receiving of :dummy call

Actual behavior

Spec fails:

$ ruby script.rb ... inside dummy inside dummy F

Failures:

1) Review is expected to receive dummy(*(any args)) 1 time Failure/Error: expect(record).to receive(:dummy)

   (#<Review id: 1, desired_article_id: 2>).dummy(*(any args))
       expected: 1 time with any arguments
       received: 0 times with any arguments
 # script.rb:66:in `block (2 levels) in <main>'

Finished in 0.0346 seconds (files took 0.44189 seconds to load) 1 example, 1 failure

Failed examples:

rspec script.rb:63 # Review is expected to receive dummy(*(any args)) 1 time — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

dmytro-savochkin commented 1 year ago

Thanks! This doesn't help though, neither using record.run_callbacks(:commit), nor using test_after_commit gem (or rather extracting its code from it because running it is disabled for Rails >= 5).

dmytro-savochkin commented 1 year ago

Also, it looks a bit different to me, in that post on Stackoverflow they are talking about commit callbacks not being called at all, right? In my case they are called but expect(record).to receive(:dummy) doesn't notice that.

dmytro-savochkin commented 1 year ago

Okay, I've managed to reproduce it with activerecord only, without rspec, so I am gonna close it here and continue with this issue inside rails repo instead.