Movatterモバイル変換


[0]ホーム

URL:


Skip to content
DEV Community
Log in Create account

DEV Community

Cover image for Instance Doubles: Rspec Anti-Pattern?
Rami Massoud
Rami Massoud

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

  1. Don’t actually test anything other than if you stubbed out methods properly
  2. Are so brittle that any change to the structure of the code under test breaks them
  3. 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™.

tangled-web

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
Enter fullscreen modeExit fullscreen mode

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
Enter fullscreen modeExit fullscreen mode

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
Enter fullscreen modeExit fullscreen mode

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!
Enter fullscreen modeExit fullscreen mode

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)

Subscribe
pic
Create template

Templates let you quickly answer FAQs or store snippets for re-use.

Dismiss

Are you sure you want to hide this comment? It will become hidden in your post, but will still be visible via the comment'spermalink.

For further actions, you may consider blocking this person and/orreporting abuse

Cloud Native Staff Engineer @ Blue Apron, powerlifter, big eater, steak aficionado 🥩
  • Location
    Upstate NY
  • Work
    Staff Engineer at Blue Apron
  • Joined

More fromRami Massoud

DEV Community

We're a place where coders share, stay up-to-date and grow their careers.

Log in Create account

[8]ページ先頭

©2009-2025 Movatter.jp