Rubocop-rspec: Cop idea: detect unused (uncalled) spy

[epaew]

I like allow(...).to receive(...) and expect(...).to have_received(...) more than expect(...).to receive(...) because a difference between "preparation" and "expectation" is clear.
However, this style often has the problem of forgetting to erase spies that are no longer needed.
So I want to detect and erase uncalled spies.

e.g.

bad: spy is not called in any example

before do
  allow(File).to receive(:open)
end

it { expect(1 + 1).to eq 2 }

good:

def open_file(filename)
  File.open(filename, "r")
end

describe "open_file" do
  subject { open_file("file") }

  before do
    allow(File).to receive(:open).and_return("text")
  end

  it do
    expect(subject).to eq "text"
    expect(File).to have_received(:open)
  end
end

good: spy is called in at least one example

def open_file(filename)
  File.open(filename, "r")
end

describe "open_file" do
  subject { open_file("file") }

  before do
    allow(File).to receive(:open).and_return("text")
  end

  context do
    it { expect(1 + 1).to eq 2 }
  end

  context do
    it do
      expect(subject).to eq "text"
      expect(File).to have_received(:open)
    end
  end
end

[pirj]

There's one thing I dont' quite understand.
In your good example:

before do
  allow(File).to receive(:open).and_return("text")
end

it { expect(File.open).to eq "text" }

you are calling the stubbed method directly. I guess this is oversimplified to the point that it makes no sense, and you actually meant that the object with a stubbed method is part of the setup, and the method is called somewhere from the inside the SUT. Is my understanding correct?

[epaew]

Yes, I don't think the example pointed out was suitable for actual use.
All spies must be called from within the test target method.

[epaew]

I fixed the example in the issue description.

[pirj]

Aha, so if it's

    allow(File).to receive(:open).and_return("text")

and no have_received expectations have been made on File, it's still ok (because the test is checking for the result, and the allowance is a stub).
But

    allow(File).to receive(:open)

is bad if we don't make a have_received expectation on File in any of the examples, correct?

That sounds reasonable to me.
Will you tackle this cop, provided all necessary guidance/assistance?

[epaew]

Maybe there is a bit of difference in interpretation.
My primary concern is whether the spy method (i.e. File.open) was called during the test.

Of course, I know it can be detected by using expect(...).to receive(...), but

I like allow(...).to receive(...) ... because a difference between "preparation" and "expectation" is clear.

[pirj]

What if it was not called, like in your example, at least not directly?

def open_file(file)
  # it's too hard for RuboCop RSpec to track it up to here
  File.open # ...
end

before do
  allow(File).to receive(:open)
end

it do
 open_file("file")
 expect(...) # `File` is never mentioned here

How about this?

before do
  allow(HTTPParty).to receive(:get) # stub this to prevent HTTP calls in tests
end

subject(:track_visitor) { described_class.new.visit }

it do
  expect { track_visitor }.to change(Visit, :count).by(1)
end

is it bad?

[epaew]

How about this?

In this case, it is OK when HTTPParty.get is called in described_class#visit.

class DescribedClass
  def visit
    ...
    response = HTTPParty.get(anything)
    ...
  end
end

It is also OK in the following case. (I recognize "HTTPParty.get has been called.")

class DescribedClass
  def visit
    some_private_method
  end

  private

  def some_private_method
    HTTPParty.get(anything)
  end
end

It is bad when the HTTPParty.get is never called before the described_class#visit has been completed.

it's too hard for RuboCop RSpec to track it up to here

I found it too hard, so I will give up on this idea. Thanks.

[pirj]

But RuboCop RSpec knows nothing about what is called underneath.

I guess this cop would be too hard to implement, with edge cases causing both false positives.

I'll close it for now until we figure some non-ambiguous and narrow case that we can detect.

[bquorning]

So I want to detect and erase uncalled spies.

I would recommend you take a look at the rspectre gem, created by RuboCop RSpec contributor @dgollahon. It “picks up where static analysis tools like rubocop-rspec leave off by analyzing your test suite as it runs.”

[dgollahon]

Thanks for pointing people my way for things like this, @bquorning 😄 That is definitely the kind of thing rspectre hopes to detect. As a note, I don't currently have spies/stubbing/etc. tracked _yet_, but it is something I hope to support. Maybe I can steal away a few minutes this month or next month at get something basic working--it shouldn't be all too hard.

I'd recommend watching the releases if you're interested in that functionality.