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:
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
# 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.