rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
805 stars 277 forks source link

RSpec/RepeatedSubjectCall flags a false positive #1821

Closed luizkowalski closed 7 months ago

luizkowalski commented 7 months ago

Hey πŸ‘‹πŸ»

I have a test like this:

subject { described_class.new(url) }
...
context "remaining is exhausted" do
  let(:remaining) { 0 }

  it "fails on the next call" do
    expect { subject.call }.not_to raise_error
    expect { subject.call }.to raise_error(RateLimiter::LimitedError)
  end
end
...

from what I understand, subject is memorized; in this case, described_class.new(url) but the .call is not memoized. I know that because this test is currently passing.

I don't think this cop should flag such cases (when the subject invokes another method).

franzliedke commented 7 months ago

Ha, I just came here to report the same problem.

Another example with false positives:

expect { subject.method1 }.to raise_error(SomeException)
expect { subject.method2 }.to raise_error(SomeException)
expect { subject.method3 }.to raise_error(SomeException)
tobiasamft commented 7 months ago

Hello πŸ‘‹ , I just wanted to add another example which could be worth mentioning:

class Something
  def work
    private_method
  end

  private

  def private_method
    # some call that may raise an error
  end
end
describe Something do
  subject { described_class.new }

  describe '#work' do
    it 'raises' do
      allow(subject).to receive(:private_method).and_raise(
        StandardError,
        'error message',
      )
      expect { subject.work }.to raise_error(StandardError, 'error message')
    end
  end
end
sgessa commented 1 month ago

Not sure if this case is already covered but seems a false positive to me. The following example is called from a shared example.

it 'raises an error if country GB' do
  if country_id == 'GB'
    expect { action }.to raise_error(ArgumentError)
  else
    expect { action }.not_to raise_error
  end
end
pirj commented 1 month ago

The expectation inside the "else" clause doesn’t match the docstring. It deserves to be extracted to a separate co text or example. Thus, no need for an if and no false positive.