Proposal: Add cop to prevent mocking Rails find_by

[hlascelles]

This is a feature proposal. Take the following code:

# Code
def show
  @user = User.find_by(params[:idq])
end

# Spec
test_user = User.create
expect(User).to receive(:find_by).and_return(user)
# ... more code

In this case, the developer has made their lives easier in the spec by simply giving themselves the test_user object they want to work on.

However, they have hidden the fact that the params lookup is a dud, using idq and not id.

The correct thing to do is set up the DB correctly with the user, then get the test code to exercise the lookup properly.

Describe the solution you'd like

I would like to prevent receive(:find_by) and its variants, and receive(:where) and similar.

Describe alternatives you've considered

Telling the team not to do this. But, I'd prefer a bot to do it :)

Additional context

I am unsure if this ticket should go in rubocop-rspec or rubocop-rails, so apologies if this is the wrong place!

I can have a go if you agree it might be useful?

[hlascelles]

You may say, "you can add a with to the expectation to check the params". Whilst that is true, the lookup code may perform additional actions like preloading to prevent N+1:

    def find_user(id)
      User.lazy_preload(:orders).find_by!(ref: id)
    end

In this code the object will be materially different to one looked up in a spec and mocked in.

[pirj]

I clearly see problem that you highlight. Not that I see this often in practice, as FactoryBot prevails.

To proceed with a cop, I suggest harvesting real-life usages to ensure we’re not missing some legit use cases. That at least would be necessary to enable the cop by default.

[lovro-bikic]

I (unfortunately) see this often in the codebase I work in, and now even more so with AI tools taking place because current models have a tendency to (over)stub. At first I thought about submitting a cop for forbidding stubs of Rails DB methods, but then I remembered there is usually other code in my codebase that shouldn't be stubbed either (i.e. we have some helper methods to set up other data, but devs stub code rather than use these helpers).

To that end, I've been thinking about an RSpec/ForbiddenStubs cop (disabled by default), which you can configure to forbid stubbing of a method, of a receiver, or some methods on a receiver, e.g.:

RSpec/ForbiddenStubs:
  Enabled: true
  ForbiddenMethods:
    - find_by # forbids stubbing `find_by` on any receiver
  ForbiddenReceivers:
    - User # forbids stubbing any method on `User`
  ForbiddenStubs:
    User:
      - create # forbids stubbing `User.create` and `User#create`
      - create! # forbids stubbing `User.create!` and `User#create!`

Such cop would cover Rails methods, but it would also give the users freedom to configure it any other way they want/need.

[pirj]

How do your specs stub the calls with lazy_preload and alike (scopes, associations, joins, …)?

[lovro-bikic]

They use receive_message_chain. For this example:

def find_user(id)
  User.lazy_preload(:orders).find_by!(ref: id)
end

they would use something like:

allow(User).to receive_message_chain(:lazy_preload, :find_by!).and_return(user)

EDIT: In the context of a supposed RSpec/ForbiddenStubs cop, if e.g. ForbiddenMethods: [find_by!], then the above example would be an offense because there's at least one method in the chain which is forbidden.