1

Firstly, this question may stray into opinion but I think it's a valuable question to ask. I will give a very specific example for my application which handles absence management and tracking.

An Account has many Users and a User has many Absences. The Account can create PublicHolidays which should be ignored when calculating the number of days that an Absence uses.

Example: If a person takes a week off, the days used will be 5. If one of those days is a PublicHoliday, the days used would be 4.

I want to implement a method such that when a PublicHoliday is created, the days used for any Absences created prior to the date of creation and which cross the date of the PublicHoliday are recalculated.

My current RSpec test looks like this:

it 'triggers a recalculation of absence days on create for absences created before the date of creation of the public holiday' do
  robin = FactoryGirl.create(:robin)
  absence = FactoryGirl.create(:basic_absence, user: robin)
  expect(absence.days_used).to eq(1)
  ph = FactoryGirl.create(:public_holiday, country: "England", account: robin.account)
  expect(absence.reload.days_used).to eq(0)
end

In this test, ph is the same date as the absence so I expect it to calculate one day to start with and then I intend to use an after create callback to recalculate the days used.

Is this the right way to do this test? Is there a more efficient way without creating a number of associated objects?

4
  • My assumption is that for every time a user is absent (including holidays), a new Absence record is created. Since you said days_used = absences - holidays, then wouldnt it be better to put the days_used method as part of the User model instead, and not the Absence model? I imagine I would do something like @user.days_used instead of @absence.days_used, unless I misunderstand something Commented Mar 31, 2016 at 8:49
  • There is a similar method to days_used on the user model. There are a lot of edge cases around how many days an absence uses and the logic is quite complex so I don't want to move it. Commented Mar 31, 2016 at 9:32
  • Aside from explicitly specifying the date as the following ph = FactoryGirl.create(:public_holiday, country: "England", account: robin.account, created_at: absence.created_at) (also makes sure that at midnight, the test will not fail), I think the test is sound. Commented Mar 31, 2016 at 9:45
  • I'd write this as two tests, not because there's anything wrong with multiple expectations in a single test in general, but because your two expectations are on different actions. But your general strategy seems fine to me. Commented Mar 31, 2016 at 12:03

1 Answer 1

2

Firstly - it's good practice to use lets instead of local variables, and secondly - split your tests so each test tests just one thing. Thirdly: anything that sets up a context for tests should be put into a context-block (even if there's only one test in that context)

eg, here's a re-writing of your spec the standard way:

let(:robin) { FactoryGirl.create(:robin) }
let(:absence) { FactoryGirl.create(:basic_absence, user: robin) }


context "with no public holidays" do
  it 'counts the absence day to be a day used' do
    expect(absence.days_used).to eq(1)
  end
end

context "with a public holiday for the absence" do
  before do
    FactoryGirl.create(:public_holiday, country: "England", account: robin.account)
  end
  it 'does not consider the absence day to be a day used' do
    expect(absence.days_used).to eq(0)
  end
end
Sign up to request clarification or add additional context in comments.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.