Rubocop-rspec: RSpec/NamedSubject: false positive when raising error

[cabello]

Expected behavior

To not trigger when the following construction is used:

it { expect { subject }.to raise_error(ArgumentError, 'problem') }

Actual behavior

Suggests to name subject:

RSpec/NamedSubject: Name your test subject if you need to reference it explicitly.
  it { expect { subject }.to raise_error(ArgumentError, 'problem') }

Steps to reproduce the problem

Enable RSpec/NamedSubject in your .rubocop.yml:

RSpec/NamedSubject:
  Enabled: true

Add a spec that uses subject in a block to check for exception:

RSpec.describe Object do
  subject { raise ArgumentError, 'problem' }

  it { expect { subject }.to raise_error(ArgumentError, 'problem') }
end

RuboCop version

$ bundle exec rubocop -V
0.65.0 (using Parser 2.6.0.0, running on ruby 2.4.5 x86_64-darwin18)

[rrosenblum]

The RSpec cops live in the rubocop-rspec project. Please open up an issue in that project.

[koic]

This issue has been transferred to rubocop-rspec project.

[mikegee]

I don't understand your expectations for this cop. NamedSubject wants you to use subject with a name and your example doesn't have a name, so the cop complains. Do you expect special case behavior for raising errors?

named subject docs

[cabello]

One cannot write it { is_expected.to raise_error(ArgumentError, 'problem') } because the exception must be raised in a block and in this case the exception would be raised before the assertion failing the test.

If I understand correctly the cop was added to prevent developers from doing expect(subject.size).to, subject.errors or accessing an attribute without naming the subject, in this case an attribute is not being accessed and there is no alternative to this idiom.

[mikegee]

there is no alternative to this idiom

RSpec.describe Object do
  subject(:hell_raiser) { raise ArgumentError, 'problem' }

  it { expect { hell_raiser }.to raise_error(ArgumentError, 'problem') }
end

[cabello]

Yes, this works as expected, I am just not sure what do I get from naming the subject in this case, it's a short 2 line spec, it's clear and no attributes are being read from the subject, for example:

RSpec.describe User do
  subject { User.find!(404) }

  it { expect { subject }.to raise(ActiveRecord::RecordNotFound) }
end

vs

RSpec.describe User do
  subject(:user_find) { User.find!(404) }

  it { expect { user_find }.to raise(ActiveRecord::RecordNotFound) }
end

Is mentioning subject as a named variable a code smell or bad pattern in all scenarios?

[bquorning]

I would most often expect subject to be an instance of described_class; in your examples an instance of User.

Following that pattern, calling subject shouldn’t have side effects, except perhaps storing a record in a database.

I would recommend rewriting to e.g.:

RSpec.describe User do
  it { expect { User.find!(404) }.to raise(ActiveRecord::RecordNotFound) }
end

[cabello]

I am having a hard time arguing for not naming the subject on this instance so I will provide a more contrived example that remembers how I usually code & test my code.

class FooService
  def initialize(dependency: Dependency.new)
    @dependency = dependency
  end

  def call
    @dependency.call == 'some awesome data' && 42
  rescue ServiceUnavailable
    # log something
    raise ComputationFailed
  end
end

RSpec.describe FooService do
  let(:dependency) { instance_double('Dependency') }

  subject { described_class.new(dependency: dependency).call }

  context 'happy path' do
    let(:good_data) { 'some awesome data' }

    before { allow(dependency).to receive(:call).and_return(good_data)

    it { is_expected.to eq(42) }
  end

  context 'sad path' do
    before { allow(dependency).to receive(:call).and_raise(ServiceUnavailable) }

    it { expect { subject }.to raise_error(ComputationFailed) }
  end
end

What would I get from naming the subject to something say foo_service? Would my code be clearer in this case?

I like this using described_class to reduce the amount of places I have to change when renaming the class. I like the concept of "test subject".

Ideally I would do it { is_expected.to raise_error(ComputationFailed) } but RSpec does not allow this use case.

I appreciate the feedback from all of you :), at this moment I disabled this specific rule for my project, but I can see myself re-enabling again soon.

[mikegee]

Assuming you intended to chain a .call in your subject there, I’d write it with the .call in each example and name the FooService instance as the subject. But now I see why you want the subject to be more of a verb than a noun; it seems natural for service objects.

[dgollahon]

I would agree with @bquorning that in general I expect subject to be an instance of described_class. As @mikegee suggests you could potentially have a #call call in each example or something like that. I tend to think this is more clear and helpful.

On some occasions when i'm testing a very imperative-feeling service object with a lot of setup, i might do something like this:

def perform_action
  described_class.new(...).perform
end

it 'explodes' do
  expect { perform_action }.to raise_error(FooError)
end

I find it personally a little odd to have a memoized subject helper represent a result or an action most of the time. If you're testing a side effect through subject that calling subject twice wouldn't cause the behavior twice (since it is memoized) so sometimes a plain old method feels more natural to me.

That said, I think this is all a very grey area of testing. If you write a lot of tests that leave you fighting with this cop, disable it. Or disable it for a particular file. I think the expect block exception is probably too specific of a case to merit a special configuration option here, but I could possibly be convinced otherwise.

On a related note, I don't use the implicit expect syntax at all because I prefer the expanded/explicit form but there are some testing situations where i could be convinced that it fits well.

This is a subjective cop and if you disagree with it that's ok, but i wouldn't consider this a false positive: it's doing its job and asking you to use a descriptive subject name any time you want to explicitly reference your subject.

[dgollahon]

I'm actually a little surprised we don't have a configuration option for forcing subjects to not be named since that's a thing you could reasonably consider enforcing uniformly if you don't like the idea of naming your subjects.

[pirj]

Can't agree more that a helper method fits here very nicely.

You may also be interested in a recent discussion on a very similar topic in rspec-expectations. rspec-has might be exactly what you need to match your testing style.

it { is_expected.to eq(42) }
it { has.to raise_error(ComputationFailed) }

There's an option to put a proc in your subject:

subject(:service_call) { -> { service.something } }

There's an open issue in RSpec Style Guide to recommend against this syntax though. The most important part since it's being discussed here is that this syntax is nearly impossible to detect using static analysis tools like RuboCop.

Anyway, when an unnamed subject is referenced directly in an example, it's hard to understand what subject is. A self-explaining name provides more information to the reader and helps to write your examples in a way that they not only cover the correctness of the behaviour of your code but also document it.

[pirj]

This cop properly detects the usage of an unnamed subject as such with the block syntax.

RSpec Style Guide recommends to "use named subjects when possible".

As per my comments above, there are workarounds for cases provided. Also with the examples provided it was possible to use named subjects and appease the cop.

Even though some prefer not to name subjects, would it be a good practice to let them disallow named subjects completely in their projects just for the sake of consistency?

[pirj]

May I ask a clarifying question, @cabello? What is the nature and semantics of what FooService#call returns? Would you consider naming the subject correspondingly, e.g. if it's "weight" then to go with:

subject(:weight) { described_class.new(dependency: dependency).call }
# ...
    it { expect(weight).to eq(42) }
    it { expect { weight }.to raise_error(ComputationFailed) }

[cabello]

Since opening this ticket I've been experimenting with naming and not naming subjects, my personal feeling is that I would rather not name it and use subject as my safe, go to name. For example, say I am writing tests for NameImporterService, my subject is described_class.new(dependency: dependency).call, should I name this subject result, call_result, names_count?

In my opinion the extra clarity that a named subject could give in this situation defeats the mental energy required to think about a good name for it.

Nonetheless I understand how other people would like this rule and I have the option to disable it, so win/win for everyone. That's the beauty of Rubocop, it's extremely configurable, the cops have good SEOs and most of them have clear explanation why one style is preferred over another. ❤️

[pirj]

Totally valid points @cabello, thank you!

Do you mind if we close this, or do you have something in mind, like adding an additional style for the cop so that it ignores unnamed subjects that are exclusively used in block expectations?