Cop Idea: Recommend `double` instead of anonymous OpenStruct for test setup

[backus]

(I'll add explanation here later, just opening this while I'm in the middle of a code review)

[pirj]

A small benchmark to keep in mind the performance implications of double/instance_double.

    require 'benchmark/ips'
    Benchmark.ips do |x|
      class X
        attr_reader :a, :b
        def initialize(a, b)
          @a = a
          @b = b
        end
      end
      x.report('os') { OpenStruct.new(a: 1, b: 2) }
      x.report('instance_double') { instance_double(X, a: 1, b: 2) }
      x.report('new') { X.new(1, 2) }
    end

                 new      3.144M (±12.4%) i/s -     15.568M in   5.038463s
                  os    438.698k (±39.4%) i/s -      1.658M in   5.025020s
              double     32.803k (±33.0%) i/s -    122.884k in   5.250296s
     instance_double     15.570k (±24.4%) i/s -     71.052k in   5.062904s

double is 100x slower than new (for simple objects), and 10x slower than OpenStruct.

[dgollahon]

That's interesting. I wonder if there are any easy performance optimizations that could be upstreamed? There are several dynamic concepts in rspec that are a little slower than you'd expect. Still, we're talking very, very small amounts of time in absolute numbers and I think this would be a premature optimization for real test environments.

IMO, OpenStruct is very, very, very rarely the right tool for the job. It's extremely permissive and easy to misuse. Compare

it 'helps you not shoot yourself in the foot' do
  service_double = double(:service, version: 1)
  expect(service_double.version).to eql(1)
  expect { service_double.im_not_real }.to raise_error(RSpec::Mocks::MockExpectationError) # oh, whoops
end

with

it 'lets you do anything' do
  service = OpenStruct.new(version: 1)
  expect(service.version).to eql(1)
  expect(service.im_not_real).to be(nil) # uh oh
end

Additionally, you can name doubles to give additional test context and I think it's more communicative that you are intentionally creating a double for testing purposes.

I would always advise using a double.

[pirj]

I've also tried with a named OpenStruct, but it was almost as slow as a named double. I assume double is as fast as it can be.

Agree that OpenStruct is a better replacement for a null object than a verifying double.

I also prefer doubles over OpenStruct, since they are a better fit semantically, and also avoid this undesirable null object behaviour that might not be obvious and can lead to disaster.

[januszm]

I'd be great if support for using double was implemented in FactoryBot first (https://github.com/thoughtbot/factory_bot/issues/1252)