← Back to Upcase

Why do I feel this style of test is wrong?

Working with some colleagues on an application currently, this application uses fixtures, rather than factories and this particular test involves a state machine inside the model:

Heres the test:

context 'coupon is not set' do
  before { expect(Coupon).to receive(:check).and_return(nil) }

  it 'change from valid coupon to invalid' do
    subject.coupon = coupon
    expect do
      subject.apply_coupon!('invalid coupon')
    end.to change { subject.coupon }.from(coupon).to(nil)
  end
end

Here’s the method in the model that is called via a callback in the state machine:

def apply_coupon!(coupon_code)
  coupon = Coupon.check(coupon_code, customer)
  update(coupon: coupon)
  process_coupon!
  coupon
end

Here’s the issues I (personally) have with this test:

because of the fixtures, we set coupon, which feels a bit messy anyway

Then, we verify, that its changed from the thing in our setup, to our new value.

Whilst I’m ok with the expect do style of testing on the end result of a state machine transition. This just kind of feels off. Can anyone indicate if this is ok or not and why it is or isn’t a good test?

You mention one of the issues in your description:

we verify, that its changed from the thing in our setup, to our new value.

Since you know the initial value of subject.coupon (it is explicitly set on the first line of the test), the expect could be simplified to not check the value again:

it 'change from valid coupon to invalid' do
  subject.coupon = coupon
  subject.apply_coupon!('invalid_coupon')

  expect(subject.coupon).to be_nil
end

Agreed and I had made that argument. The response was because they were changing the value of the fixture and they needed to verify that it changed from a value to nil.

I felt that not only was it testing that you did setup correctly but now it was stretching into testing implementation detail

The test you posted:

# original version using `expect` and `change` blocks

it 'change from valid coupon to invalid' do
  subject.coupon = coupon
  expect do
    subject.apply_coupon!('invalid coupon')
  end.to change { subject.coupon }.from(coupon).to(nil)
end

could be rewritten as:

# multiple explicit `expect` calls

it 'change from valid coupon to invalid' do
  subject.coupon = coupon

  expect(subject.coupon).to eq coupon
  subject.apply_coupon!('invalid coupon')
  expect(subject.coupon).to be_nil
end

Both forms are equivalent, they test that subject.coupon has one value before calling apply_coupon! and that it is nil afterwards. The first (original) form just uses some syntactic sugar that obscures the duplication a bit.

When using the explicit multiple expect form, it becomes more obvious that the first expect is redundant.

Thanks for the detailed response, it’s much appreciated