
Posted on • Originally published atramimassoud.com on
Instance Doubles: Rspec Anti-Pattern?
Over twelve years in software, and I still find it incredibly difficult to write good tests for my code. Almost as difficult as getting a group of programmers to agree on what's a good test.
More than half of my career has now been working in Rails applications, and I’ve often seen specs using instance doubles as a tool to avoid the database. While I admire avoiding the database when possible in tests, the drive seems to lead to situations where the specs
- Don’t actually test anything other than if you stubbed out methods properly
- Are so brittle that any change to the structure of the code under test breaks them
- Are so brittle that they are broken by changes to the structure of code that shouldn’t even be related 😱
It’s come to the point where if I run into a file that heavily relies on instance doubles, it’s almost a guarantee that I'll have to rewrite the specs from scratch.
I don’t have enough experience to say definitively, but I have a strong suspicion that the real issue in these cases is code that is too tightly coupled and/or some leaky abstractions are in use. In a newer codebase, you might be able to untangle these issues on the code side. Yet, in a long-lived project, you’re likely stuck making the compromises in your specs unless you want a 10,000 line PR untangling code that spans 5 years and 10 different pairs of hands, all with different ideas on How It Should Be™.
When I say compromises, I specifically mean test setup code. You might have to build up tons of entities/records to test the logic of a block of code successfully, but this is by far the lesser evil. The best way to explain is with a code example.
To start, here are a few Rails models, along with the specs for an order method. One spec in the style I suspect to be an anti-pattern, and another using my preferred way.
classPaymentMethod<ApplicationRecordbelongs_to:accountendclassAccount<ApplicationRecordhas_many:payment_methodshas_one:primary_payment_method,->{where(is_primary:true)},class_name:'PaymentMethod'endclassProduct<ApplicationRecordendclassOrderProduct<ApplicationRecordbelongs_to:productbelongs_to:orderendclassOrder<ApplicationRecordbelongs_to:accounthas_many:order_productshas_many:products,through: :order_productsdefchargeable?returnfalseunlessproducts.size.positive?returnfalseunlessaccount.primary_payment_method.present?trueendendRSpec.describeOrder,type: :modeldodescribe'bad? #chargeable?'dosubject{order.chargeable?}let(:order){Order.new}let(:account){instance_double(Account,primary_payment_method:double)}let(:product){instance_double(Product)}beforedoallow(order).toreceive(:account){account}allow(order).toreceive(:products){[product]}endit{is_expected.toeq(true)}enddescribe'good? #chargeable?'dosubject{order.chargeable?}let(:order){build_stubbed(:order,user:account)}let(:account){build_stubbed(:account)}let(:product){create(:product)}let!(:order_product){create(:order_product,order:order,product:product)}let!(:payment_method){create(:payment_method,account:account,is_primary:true)}it{is_expected.toeq(true)}endend
If there is a#chargeable?
, there is likely going to be a#charge!
too. For the sake of not confusing the issue, I'll only show a simple version that won't actually hit a payment provider.
classOrder<ApplicationRecorddefcharge!returnfalseunlesschargeable?# Call payment SDKtrueendendRSpec.describeOrder,type: :modeldodescribe'bad? #charge!'dosubject{order.charge!}let(:order){Order.new}let(:account){instance_double(Account,primary_payment_method:double)}let(:product){instance_double(Product)}beforedoallow(order).toreceive(:account){account}allow(order).toreceive(:products){[product]}endit{is_expected.toeq(true)}enddescribe'good? #charge!'dosubject{order.charge!}let(:order){create(:order,account:account)}let(:account){create(:account)}let(:product){create(:product)}let!(:order_product){create(:order_product,order:order,product:product)}let!(:payment_method){create(:payment_method,account:account,is_primary:true)}it{is_expected.toeq(true)}endend
All of the specs for this hypothetical charging workflow pass! 🎉For now. 😈
Here is where things get tricky. What happens if a check needs to be added for if theprimary_payment_method
is expired? I think ideally, the only spec update required would be for#chargeable?
with#charge!
being left alone. If the default for#expired?
isfalse
,bad? #chargeable?
is the only spec that has to change. But both should change to cover the new logic branch.
classPaymentMethod<ApplicationRecorddefexpired?falseendendclassOrder<ApplicationRecorddefchargeable?returnfalseunlessproducts.size.positive?returnfalseunlessaccount.primary_payment_method.present?returnfalseifaccount.primary_payment_method.expired?trueendendRSpec.describeOrder,type: :modeldodescribe'bad? #chargeable?'dosubject{order.chargeable?}let(:order){Order.new}let(:account){instance_double(Account,primary_payment_method:payment_method)}let(:product){instance_double(Product)}let(:payment_method){instance_double(PaymentMethod,expired?:false)}beforedoallow(order).toreceive(:account){account}allow(order).toreceive(:products){[product]}endit{is_expected.toeq(true)}endend
Unfortunately, re-running the specs results in an error onbad? #charge!
.
$bundleexecrspecspec/models/order_spec.rb..F.Failures:1)Orderbad?#charge!Failure/Error:returnfalseifaccount.primary_payment_method.expired?# received unexpected message :expired? with (no args)# ./app/models/order.rb:10:in `chargeable?'# ./app/models/order.rb:16:in `charge!'# ./spec/models/order_spec.rb:35:in `block (3 levels) in '# ./spec/models/order_spec.rb:47:in `block (3 levels) in 'Finishedin0.08436seconds(filestook0.99secondstoload)4examples,1failureFailedexamples:rspec./spec/models/order_spec.rb:47# Order bad? #charge!
The#expired?
method has not been defined on the returneddouble
, so the spec "erroneously" fails even though the code that's supposed to be under test hasn't changed. On the other hand, the spec forgood? #charge!
ends up passing since the default for the expired state isfalse
.
This is a brief, semi-synthetic example of the issues I end up running into with theinstance_double
testing style all the time. This specific example could be easily adjusted to be less brittle, but as more time, hands, and complexity are added, things quickly spin out of control. There are few things worse than when I'm trying to make a small, targeted change and end up having to chase down why dozens of mostly unrelated specs are failing.
So that's my case against using instance doubles! Feel free to leave a comment or@ me on Twitter with feedback, especially if you've found a better way to manage complex specs 🙏
Top comments(0)
For further actions, you may consider blocking this person and/orreporting abuse