Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positives on SubjectStub? #981

Closed
MiralDesai opened this issue Jul 29, 2020 · 5 comments
Closed

False positives on SubjectStub? #981

MiralDesai opened this issue Jul 29, 2020 · 5 comments

Comments

@MiralDesai
Copy link

MiralDesai commented Jul 29, 2020

Using version 1.42.0.

it "validates the new user" do
  subject.call(user, 0)

  expect(subject).to have_received(:valid_user?).and_return(true)
end

This spec is not stubbing subject, it's testing that a method within the subject gets called when the main entry point (call) is called.

This seems like a false positive to me, it shouldn't be raising an offence here. According to the docs this is how it's supposed to work. Shouldn't this cop only look for allow statements though, and not expect?

@pirj
Copy link
Member

pirj commented Jul 29, 2020

Is this a complete example?

First of what comes to mind is an odd usage of have_received with and_return. Please check the don't stub when you are mocking discussion.

Another thing is how you use have_received. It is intended to be used on spies.
But not on a default subject:

class A
  def a
    b
  end
  def b
    1
  end
end

RSpec.describe(A) do
  specify do
    subject.a
    expect(subject).to have_received(:b)
  end
end
     Failure/Error: expect(subject).to have_received(:b)
       #<A:0x00007f8495a26ef0> expected to have received b, but that object is not a spy or method has not been stubbed.

have_received also coincidentally works on stubs, if you add the following to the beginning of this example, it will pass:

    allow(subject).to receive(:b)

Do you happen to stub your subject somewhere above in that spec?

The example you have referenced does not look like a false positive to me at all.

@pirj pirj closed this as completed Jul 29, 2020
@MiralDesai
Copy link
Author

It is a complete example yeah, there's no mocking of subject. The only thing that happens before this is setting up the subject with spies, where dependency injection is used.

 subject do
    described_class.new(
      update_doc: update_doc_spy,
      user_repo: user_repo_spy,
      add_error: add_error_spy
    )
  end

Typically I'm not testing subjects directly and testing spies instead so I agree, however there are a few tests in my project that tests subject methods like this one.

Didn't realise and_return was considered mocking rather than expectation in this case. I've tried running it without the and_return(true) but the same cop is raised. Not sure how to proceed, should this spec not exist? I suppose checking something was called it not exactly a strong spec, and the result of the method call is more important.

@pirj
Copy link
Member

pirj commented Jul 30, 2020

It depends on how you set the boundaries.
It's possible to expect that valid_user? is called if you also have a test for what valid_user? does. But probably it makes sense to combine those two?

Sorry, I can't really reason about this spec without seeing the whole thing.

And I'm still struggling to understand how have_received works on an object that is not stubbed.

@MiralDesai
Copy link
Author

Fair enough, in the case of this spec I can definitely remove it and the other place I noticed this cop being raised could do with some refactoring to check spies instead of the subject itself. Relatively old specs now that I look at it.

Thanks for the info above. Definitely avoidable.

@pirj
Copy link
Member

pirj commented Jul 30, 2020

You're always welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants