0

I have a model with the following callback:

class CheckIn < ActiveRecord::Base
  after_create :send_delighted_survey

  def send_delighted_survey

    position = client&.check_ins&.reverse&.index(self)
    
    if position.present? && type_of_weighin.present?
        survey = SurveyRequirement.find_by(position: [position, "*"], type_of_weighin: [type_of_weighin, "*"])

        if survey.present?
            survey.delighted_survey.sendSurvey(client: self.client, additional_properties: {delay: 3600})
        end
    end
  end
end

I am attempting to test the line: survey.delighted_survey.sendSurvey(client: self.client, additional_properties: {delay: 3600}) to ensure that the correct delighted_survey is receiving sendSurvey.

This test passes:

let!(:week_1_sr) { create(:survey_requirement, :week_1_survey) }

it "should fire a CSAT survey after week 1" do
    expect_any_instance_of(DelightedSurvey).to receive(:sendSurvey).once
    create(:check_in, client_id: client.id, type_of_weighin: "standard")
    create(:check_in, client_id: client.id, type_of_weighin: "standard")
end

However this test fails and I don't understand why

let!(:week_1_sr) { create(:survey_requirement, :week_1_survey) }

it "should fire a CSAT survey after week 1" do
    expect(week_1_sr.delighted_survey).to receive(:sendSurvey).once
    create(:check_in, client_id: client.id, type_of_weighin: "standard")
    create(:check_in, client_id: client.id, type_of_weighin: "standard")
end

When I add print statements, it's definitely calling sendsurvey on week_1_sr.delighted_survey so I don't understand why the test fails.

How should I rearrange this test?

Jeremy Thomas
  • 6,240
  • 9
  • 47
  • 92

2 Answers2

3

In my experience this is a common misunderstanding with expect(..).to receive when dealing with ActiveRecord.

We have to remember that the ActiveRecord object we create in our test stores a row in the database, the code in your model loads the row from the database and populates an entirely different activerecord object that is completely unconnected from the one in your test, except that they both refer to the same underlying database row.

Rspec is not "smart" about activerecord and the method you're stubbing/expecting is only on the instance of object in your test.

So how to fix this. The most direct option is to stub out the object that your code actually uses. This isn't easy, however, since it's an object returned from a method on another object returned by SurveyRequirement.find_by(...). You can do it with something like:

Option 1 - Stub everything

survey_requirement_stub = double(SurveyRequirement)
survey_stub = double(Survey)
allow(SurveyRequirement).to receive(:find_by).and_return(survey_requirement_stub)
allow(survey_requirement_stub).to receive(:delighted_survey).and_return(survey_stub)

expect(survey_stub).to receive(:sendSurvey).once

However I wouldn't recommend this. It closely ties your test to the internal implementation of your method. e.g. Adding a scope (scoped.find_by instead of find_by) breaks the test in a way that's not meaningful.

Option 2 - Test the results, not the implementation

If the point of sendSurvey is enqueuing a background job or sending an email, that may be be a better place to test that it's doing what's expected, for example:

expect { create_checkin }.to have_enqueued_job(MyEmailJob)
# or, if it sends right away
expect { create_checkin }.to change { ActionMailer::Base.deliveries.count }.by(1)

I think this approach is OK, but the implementation means that your code will be enqueuing jobs and firing emails throughout your test base. It will not be possible to create a check-in and not fire these.

This is why I strongly advise our engineers to NEVER use activerecord callbacks for business logic like this.

Instead...

Option 3 - Refactor to use service objects / interactors instead

As your application grows, using activerecord callbacks to create other records, update records, or trigger side-effects (like emails) becomes a significant anti-pattern. I'd take this as an opportunity to restructure the code to be easier to test and remove business logic from your ActiveRecord objects.

This should make each part of this easier to test (e.g. is the survey-requirement looked up correctly? Does it send?). I've run out of time but here's the general idea:

class CheckIn
  def get_survey_requirement
    position = client&.check_ins&.reverse&.index(self)
    return unless position.present? && type_of_weighin.present?

    SurveyRequirement.find_by(position: [position, "*"], type_of_weighin: [type_of_weighin, "*"])
  end
end
class CheckInCreater
  def self.call(params)
    check_in = CheckIn.build(params)
    check_in.save!
    DelightedSurveySender.call(check_in)
  end
end
class DelightedSurveySender
  def self.call(check_in)
    survey = check_in.survey_requirement&.delighted_survey
    return unless survey

    survey.send_survey(client: check_in.client, additional_properties: {delay: 3600})
  end
end
melcher
  • 1,543
  • 9
  • 15
1

This is happening because week_1_sr.delighted_survey in spec and survey.delighted_survey aren't the same instance. Yes, both are instances of the same class and they both represent the same record in the database and same model behavior, but they do not have the same object_id.

In your first test, you are expecting that any instance of DelightedSurvey receives the method and that's, indeed, true. But in your second spec, you expect that that exact instance receives sendSurvey.

There are many ways to rearrange your test. In fact, if you ask 100 developers how to test something, you will get 100 different answers.

There is this approach:

let(:create_week_1_sr) { create(:survey_requirement, :week_1_survey) }

it "should fire a CSAT survey after week 1" do
  week_1_sr = create_week_1_sr # I don't think DRY is the better approach for testing, but it's just my opinion
  allow(SurveyRequirement).to(receive(:find_by).and_return(week_1_sr))
  delighted_survey_spy = instance_spy(DelightedSurvey)
  allow(week_1_sr).to(receive(:delighted_survey).and_return(delighted_survey_spy))

  create(:check_in, client_id: client.id, type_of_weighin: "standard")
  create(:check_in, client_id: client.id, type_of_weighin: "standard")

  expect(delighted_survey_spy).to(have_received(:sendSurvey))
end

First thing about the test I wrote: Arrange, Act and Assert. It's clear to me where I am arranging my test, where I am acting and where I am asserting.

But you can realize that this test is polluted and has some prejudicial mocks. Like:

allow(SurveyRequirement).to(receive(:find_by).and_return(week_1_sr))

It will return week_1_sr even if you pass a wrong parameter to find_by (you can workaround it using with, but it will add logic to your tests).

You can see that it's pretty hard to test and I would agree. So would you consider removing this logic to a service class or whatever?

Oh, and just a heads up: after_create will be triggered even if the record is not commited for whatever reason. So you might consider using after_create_commit

(just finished and got the notice of melcher's answer. his is better)

João Fernandes
  • 673
  • 1
  • 5
  • 15