How to properly mock an instance of an Active Record model to test that a method has been called-CodePudding

With the following controller action:

def create
  my_foo = MyFoo.find params[:foo_id]

  my_bar = my_foo.my_bars.create! my_bar_params
  my_bar.send_notifications

  redirect_to my_bar
end

In my test, I am trying to assert that the method send_notifications) is called in my_bar, which is an instance of an AR model.

One way to test this would be to ensure that the notifications are sent (expect { request}.to enqueue_mail ...). But this is probably not a good practice because it pierces through abstraction layers.

Another option would be to use expect_any_instance_of:

it 'sends the notifications' do
  expect_any_instance_of(MyBar).to receive :send_notifications

  post my_bars_path, params: { my_bar: valid_attributes }
end

I like this method because it's clean and straightforward, but it seems that the creators of RSpec deprecated it.

The other method I tried requires mocking many AR methods:

it 'sends the notifications' do
  my_bar = instance_double MyBar

  allow(MyBar).to receive(:new).and_return my_bar
  allow(my_bar).to receive :_has_attribute?
  allow(my_bar).to receive :_write_attribute
  allow(my_bar).to receive :save!
  allow(my_bar).to receive :new_record?

  expect(my_bar).to receive :send_notifications

  allow(my_bar).to receive(:to_model).and_return my_bar
  allow(my_bar).to receive(:persisted?).and_return true
  allow(my_bar).to receive(:model_name).and_return ActiveModel::Name.new MyBar

  post my_bars_path, params: { my_bar: valid_attributes }
end

The allows over the expect are there to mock the line @my_bar = @my_foo.my_bars.create! my_bar_params. The rest of the allows under the expect are there to mock redirect_to @my_bar.

I don’t know if this is what the creators of RSpec want us to write, but it does not seem very ergonomic.

So, my question is: is there any other way to write a test like this that does not involve mocking lots of AR internals and does not require me to change the code in my controller action?

CodePudding user response：

I like [using expect_any_instance_of] because it's clean and straightforward, but it seems that the creators of RSpec deprecated it.

They discourage it with good reason. What if the controller code changes and something else calls send_notifications? The test will pass.

Having to use expect_any_instance_of or allow_any_instance_of indicates the code is doing too much and can be redesigned.

What can't be solved by adding another layer of abstraction?

def create
  my_bar.send_notifications

  redirect_to my_bar
end

private

def my_foo
  MyFoo.find params[:foo_id]
end

def my_bar
  my_foo.my_bars.create! my_bar_params
end

Now you can mock my_bar to return a double. If the method made more extensive use of my_bar, you can also return a real MyBar.

it 'sends the notifications' do
  bar = instance_double(MyBar)
  allow(@controller).to receive(:my_bar).and_return(bar)

  post my_bars_path, params: { my_bar: valid_attributes }
end

Encapsulating finding and creating models and records within a controller is a common pattern.

It also creates a pleasing symmetry between the test and the method which indicates the method is doing exactly as much as it has to and no more.

Or, use a service object to handle the notification and check that.

class MyNotifier
  def self.send(message)
    ...
  end
end

class MyBar
  NOTIFICATION_MESSAGE = "A bar, a bar, dancing in the night".freeze

  def send_notification
    MyNotifier.send(NOTIFICATION_MESSAGE)
  end
end

Then test the notification happens.

it 'sends the notifications' do
  expect(MyNotifier).to receive(:send)
    .with(MyBar.const_get(:NOTIFICATION_MESSAGE))

  post my_bars_path, params: { my_bar: valid_attributes }
end

By making send a class method, we don't need to use expect_any_instance_of. Writing services objects as singleton classes which have no state is a common pattern for this reason and many others.

The downside here is it does require knowledge of how MyBar#send_notification works, but if the app uses the same service object to do notifications this is acceptable.

Or, create a MyFoo for it to find. Mock its call to create MyBar being sure to check the arguments are correct.

let(:foo) {
  MyFoo.create!(...)
}
let(:foo_id) { foo.id }

it 'sends the notifications' do
  bar = instance_double(MyBar)
  expect(bar).to receive(:send_notifications).with(no_args)

  allow(foo).to receive_message_chain(:my_bars, :create!)
    .with(valid_attributes)
    .and_return(bar)

  post my_bars_path, params: { foo_id: foo_id, my_bar: valid_attributes }
end

This requires more knowledge of the internals, and rubocop-rspec does not like message chains.

Or, mock MyFoo.find to return a MyFoo double. Again, be sure to only accept the proper arguments.

it 'sends the notifications' do
  foo = instance_double(MyFoo)
  allow(foo).to receive_message_chain(:my_bars, :create!)
    .with(valid_attributes)
    .and_return(bar)

  bar = instance_double(MyBar)
  expect(bar).to receive(:send_notifications).with(no_args)

  allow(MyFoo).to receive(:find).with(foo.id).and_return(foo)

  post my_bars_path, params: { foo_id: foo_id, my_bar: valid_attributes }
end

Alternatively you could allow(MyFoo).to receive(:find).with(foo_id).and_return(mocked_foo), but I find it's better to mock as little as possible.

CodePudding user response：

What I'd do in this case is to make the controller "dumb", move the persistence and notification logic into a service, and test the service in integration instead of stubbing.

# controller
def create
  my_bar = CreateBar.call params[:foo_id], my_bar_params

  redirect_to my_bars_path(my_bar) 
end

# service
class CreateBar
  def self.call(foo_id, bar_params)
    foo = MyFoo.find params[:foo_id]

    bar = foo.my_bars.create! bar_params
    bar.send_notifications

    bar
  end
end

# controller spec
it 'creates a new bar' do
  bar = double :bar, id: 'whatever'

  expect(CreateBar).to receive(:call).with(<attributes>).and_return(bar)

  post my_bars_path, params: {my_bar: valid_attribute}
end

it 'redirects to the created bar' do
  bar = double :bar, id: 1

  allow(CreateBar).to receive(:call).with(<attributes>).and_return(bar)

  post my_bars_path, params: {my_bar: valid_attribute}

  expect(response).to redirect_to(my_bars_path(1))
end

# service spec
it 'creates a bar' do
  # Or just create the object via AR's interface if you don't use factory bot.
  foo = FactoryBot.create :foo

  CreateBar.call foo.id, <bar_params>

  bars = Foo.find(foo.id).bars

  expect(bars.count).to eq 1
  expect(bars.first).to have_attribute <expected_attributes>
end

it 'sends a bar notification' do
  foo = FactoryBot.create :foo

  CreateBar.call foo.id, <bar_params>

  # NotificationCenter is expected to be called from #send_notification
  # and in test environment it records your notifications instead of actually
  # sending them.
  notifications = NotificationCenter.notifications

  expect(notifications.count).to eq <expected notifications count>
  #   assert that the notifications have the properties based on the <bar_params>
end

Depending on the complexity if #send_notifications, you can either inline it in CreateFoo (thus keeping the persistence model separate from the business logic) or have a separate service SendFooNotifications.