Rubocop RSpec/MultipleMemoizedHelpers issue on pundit spec tests

Question

I use pundit for authorization and RSpec for testing in my rails app. Due to this, I had to create specs for the policies.

However, I am having a problem with rubocop throwing an error: RSpec/MultipleMemoizedHelpers. I understand that this means I have too many let and subject calls. My issue is I'm not quite sure how to resolve or refactor my code so it adheres to the proper number of calls I should make.

Another thing, is it okay to disable RSpec/MultipleMemoizedHelpers for spec files?

Here are three of the policy spec files that are an issue.

require "rails_helper"

describe AnswerPolicy do
  subject { described_class }

  let(:user_admin) { build(:user, :admin) }
  let(:consultant) { build(:consultant) }
  let(:user_consultant) { build(:user, :consultant, consultant: consultant) }
  let(:client) { build(:client, consultant: consultant) }
  let(:user_client) { build(:user, :client, client: client) }
  let(:other_client) { build(:client, consultant: build(:consultant)) }
  let(:answer) { build(:answer, client: client) }
  let(:other_answer) { build(:answer, client: other_client) }

  permissions :update? do
    it "allows access to admin" do
      expect(described_class).to permit(user_admin)
    end

    it "prevents consultants to update other non-client answers" do
      expect(described_class).not_to permit(user_consultant, other_answer)
    end

    it "prevents clients to update their answers" do
      expect(described_class).not_to permit(user_client, answer)
    end

    it "allows consultants to update their client's answers" do
      expect(described_class).to permit(user_consultant, answer)
    end
  end
end

describe AssessmentStepPolicy do
  subject { described_class }

  let(:user_admin) { build(:user, :admin) }
  let(:consultant) { build(:consultant) }
  let(:user_consultant) { build(:user, :consultant, consultant: consultant) }
  let(:client) { build(:client, consultant: consultant) }
  let(:user_client) { build(:user, :client, client: client) }
  let(:other_client) { build(:client, consultant: build(:consultant)) }

  permissions :view? do
    it "allows access to admin" do
      expect(described_class).to permit(user_admin)
    end

    it "prevents consultants to view other non-client assessment details" do
      expect(described_class).not_to permit(user_consultant, other_client)
    end

    it "allows clients to view their assessment details" do
      expect(described_class).to permit(user_client, client)
    end

    it "prevents clients to view other client's assessment details" do
      expect(described_class).not_to permit(user_client, other_client)
    end

    it "allows consultants to view their client's answers" do
      expect(described_class).to permit(user_consultant, client)
    end
  end

  permissions :create? do
    it "allows access to any admin" do
      expect(described_class).to permit(user_admin)
    end

    it "prevents consultants to assess other clients" do
      expect(described_class).not_to permit(user_consultant, other_client)
    end

    it "prevents clients to assess themselves" do
      expect(described_class).not_to permit(user_client, client)
    end

    it "allows consultants to assess their clients" do
      expect(described_class).to permit(user_consultant, client)
    end
  end
end

require "rails_helper"

describe ReportPolicy do
  subject { described_class }

  let(:user_admin) { build(:user, :admin) }
  let(:consultant) { build(:consultant) }
  let(:user_consultant) { build(:user, :consultant, consultant: consultant) }
  let(:client) { build(:client, consultant: consultant) }
  let(:user_client) { build(:user, :client, client: client) }
  let(:other_consultant) { build(:consultant) }
  let(:other_client) { build(:client, consultant: other_consultant) }

  permissions :dashboard? do
    it "allows access to admin" do
      expect(described_class).to permit(user_admin)
    end

    it "prevents clients to view other client dashboards" do
      expect(described_class).not_to permit(user_client, other_client)
    end

    it "prevents consultants to view other non-client dashboards" do
      expect(described_class).not_to permit(user_consultant, other_client)
    end

    it "allows clients to view their dashboard" do
      expect(described_class).to permit(user_client, client)
    end

    it "allows consultants to view their client's dashboards" do
      expect(described_class).to permit(user_consultant, client)
    end
  end
end

Kania

Are they in their own spec file? If so, I wouldn't mind, but if you see duplication there you can move them to shared contexts. – Loggerhead

Yes, all three are in their separate file. Moving duplicated calls to shared context makes a lot of sense. I will try that out. Thanks @SebastianPalma – Kania

Answer 1

This RSpec/MultipleMemoizedHelpers cop is controversial. It wants you to limit the number of let to an arbitrary number.

I fought hard against it. To me it is similar to a cop that would raise an offense because you have too many variables. rubocop and rubocop-ast have it disabled, while we typically enable more cops that the default. Note that you could change these let to def and the offenses go away (although you haven't changed anything; let is just syntax sugar for a def).

Sharing your factories seems like a good idea, and I recommend disabling the cop too.

Emlin

Good to hear this recommendation from you! Thanks for your OS work too! – Petta

Answer 2

So I think I can take some amount of credit for starting this debate. I'm a big fan of not using let and subject blocks. They add a lot of indirection in tests that can make it hard to read and extend tests as they get larger. You can read more of my thoughts in the linked thread and its offshoots. Rather than using let, I would start by simply in-lining the dependencies inside the test block:

it "prevents consultants to view other non-client assessment details" do
  consultant = build(:consultant)
  user = build(:user, :consultant, consultant: consultant)
  client = build(:client, consultant: build(:consultant))

  expect(described_class).not_to permit(user, client)
end

That is a bit wordy, but I don't mind some duplication in my tests. My general rule of thumb is that if it's <= 5 lines, then I don't stress too much about it. But still, I might want to pull some of the setup into factories:

it "prevents consultants to view other non-client assessment details" do
  user = build(:user, :consultant)
  client = build(:client, :with_consultant)

  expect(described_class).not_to permit(user, client)
end

The nice thing about this is that it doesn't require you to keep track of the existing tests when writing new ones. It's easier to write variations without risking breaking the let blocks working for all of the other tests.

Rapine

I appreciate your thoughts! I have changed the way I write tests since then and I've actually been doing it the way you explained here. I agree that it is cleaner and a lot easier to read. – Kania

FYI: It may be useful to think of this cop as the equivalent of the core Metrics/Parameterlists cop. If you squint, let statements are the "parameter list" of the test. – Collection