Disable Cop `RSpec/NamedSubject`

[FLeinzi]

Hello,

I would appreciate if you could disable the cop RSpec/NamedSubject. I think this Cop do not represent our coding style, as we often use unnamed subjects in our specs. I updated makandra-rubocop in one of my projects and got more than 200 offenses only because of this cop.

# bad
RSpec.describe User do
  subject { described_class.new }

  it 'is valid' do
    expect(subject.valid?).to be(true)
  end
end

# good
RSpec.describe Foo do
  subject(:user) { described_class.new }

  it 'is valid' do
    expect(user.valid?).to be(true)
  end
end

# also good
RSpec.describe Foo do
  subject(:user) { described_class.new }

  it { is_expected.to be_valid }
end

Source: https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecnamedsubject

Please vote :+1: for Disabling and :-1: for Keeping this cop

[denzelem]

I also think enforcing this does not make sense, too. Nevertheless I didn't see the advantage of using unnamed subjects. It's some RSpec thing that makes longer specs hard to read, especially when the subject changes in the describe and context blocks:

RSpec.describe User do
  subject { described_class.new }

  it 'is valid' do
    expect(subject.valid?).to be(true)
  end

  context 'on create' do
     subject {  ActionMailer::Base.deliveries.last }

     it 'sends a welcome email after save' do
       FactoryBot.create(:user, name: 'Peter')

      # It's not about the user anymore
      expect(subject.body).to contain('Welcome Peter')
    end
  end
end

Or is there any advantage I couldn't see?

[codener]

I like about using subject that it clearly marks the test subject, whereas subject(:user) mixes with lets. When you have many lets and many expectations, the subject is harder to track:

...
expect{ user.do_it! }.to change(project, :counter).by(1)
expect(project.things).to be_something
expect(comment).to have_something

... where user could be called subject. I consider subject {...} a let named "subject".

[judithroth]

Contrarian Opinion: I am not a fan of using unnamed subjects. If there is a subject I always have to search what it means in my current context. Especially in large files with many describe / context blocks (maybe even nested ones) this can be a tedious task if there are many different definitions of subject (see Emanuel's example).

What I would wish for is the following:

• Use subject only for that what the whole spec is about (exactly the way rspec defines the subject if you don't define it yourself -> https://relishapp.com/rspec/rspec-core/v/3-9/docs/subject/implicitly-defined-subject)
• if you test other things, give them proper names. We would never accept variables that are just named variable. So why is it okay in tests to name random things subject? Or why is this even considered good code? I really don't understand that :sweat_smile:

To sum it up: Although I never used a named subject (let is my preferred way), I think the cop is doing the right thing.

[FLeinzi]

I totally agree with this one:

• Use subject only for that what the whole spec is about (exactly the way rspec defines the subject if you don't define it yourself -> https://relishapp.com/rspec/rspec-core/v/3-9/docs/subject/implicitly-defined-subject)

That's why I was confused about Emanuel's example, where subject is kind of different classes in each context (user and mail). In my opinion subject should always be an instance of the described class.

But this cop complains about each use of subject.*, even if you use it in the "correct" way. That's why I'm torn between disabling and keeping it.

[codener]

I agree that subject should not assume random values throughout a spec. It should not be a "neat, I can have a variable without naming it", but (usually) an instance of the described class (as @judithroth states in her first bullet). If a subject is redefined in a nested example group, it should only return a modified instance of the described class, e.g. a "locked user" for a group of specs that test locked users.

I consider the subject redefinition in the example by @denzelem bad practice – use let(:welcome_mail) instead.

Still I do not want to be forced to name my subject. Defining subject(:user) and using user throughout the spec is no better than just having subject. After all, one could easily resort to let(:subject).

This is something a bot cannot decide.

[triskweline]

I often use neither named or unnamed subjects, and just make a local variable. I hate it when specs are cluttered with unnecessary example groups, just to use a slightly different subject for one example.

One case where unnamed subject shines is when you can have a shared example group that works on subject, without knowing what subject is. This wouldn't work with named subjects. See code example in this card.

[FLeinzi]

@triskweline As far as I know, this cop won't complain about an unnamed subject in a shared example group.

[triskweline]

As far as I know, this cop won't complain about an unnamed subject in a shared example group.

It would complain about the example group that is using that shared example group, though?

[FLeinzi]

I tried it with a spec like this and got no offenses:

describe Power do
  subject { described_class.new(user) }

  shared_examples 'power' do |role:, result:|
    let(:user) { build(:user, role: role, some_attribute: some_attribute) }
    it { expect(subject.send(method_name)).to eq(result), 'failed for ' + role }
  end

  let(:method_name) do
    self.class.parent.parent.description.gsub('#', '')
  end

  describe '#instance_method' do
    context 'with some_attribute nil' do
      let(:some_attribute) { nil }

      it_behaves_like 'power', role: 'admin', result: true
      it_behaves_like 'power', role: 'contact', result: true
    end

    context 'with some_attribute set' do
      let(:some_attribute) { 'some_value' }

      it_behaves_like 'power', role: 'admin', result: false
      it_behaves_like 'power', role: 'contact', result: false
    end
  end
end

When I convert it_behaves_like 'power', role: 'admin', result: true into an own example within the context, the cop will complain about it:

describe Power do
  subject { described_class.new(user) }

  describe '#instance_method' do
    context 'with some_attribute nil' do
      let(:user) { build(:user, role: 'admin', some_attribute: nil) }

      it { expect(subject.instance_method).to eq(nil) }
    end
  end
end