rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
810 stars 276 forks source link

An option to configure `RSpec/NamedSubject` to make it allow `expect { subject }` #1259

Closed j-miyake closed 2 years ago

j-miyake commented 2 years ago

It would be very nice if there is an option to configure RSpec/NamedSubject cop so that it allows expect { subject }.

RSpec/NamedSubject works great when we test against an object returned by a method; however, I think it is a little bit difficult to handle when simultaneously testing side effects in the same describe group or context. (I found a discussion on the same topic, but seems there is no definite solution yet, except disabling the cop.)

For example, the cop warns against this code because it uses subject for expectations.

RSpec.describe Car do
  describe '.build' do
    subject { Car.build({ color: color }) }

    context 'with an available color' do
      let(:color) { 'red' }

      it { expect(subject.color).to eq 'red' }
      it { expect { subject }.to change(BuildLog, :count).by(1) }
    end

    context 'with an unavailable color' do
      let(:color) { 'lightblue' }

      it { expect { subject }.to raise_error(CarError).with_message('Unavailable color.') }
    end
  end
end

Following the cop's suggestion, the code will probably be like the below;

RSpec.describe Car do
  describe '.build' do
    subject(:car) { Car.build({ color: color }) }

    context 'with an available color' do
      let(:color) { 'red' }

      it { expect(car.color).to eq 'red' }

      # "It except car to change BuildLog count" ? :(
      it { expect { car }.to change(BuildLog, :count).by(1) }
    end

    context 'with an unavailable color' do
      let(:color) { 'lightblue' }

      # "It expect `car` to raise error" ? :(
      it { expect { car }.to raise_error(CarError).with_message('Unavailable color.') }
    end
  end
end

Otherwise;

RSpec.describe Car do
  describe '.build' do
    subject(:build_car) { Car.build({ color: color }) }

    context 'with an available color' do
      let(:color) { 'red' }

      # "It expect build_car's color to be red" ? :(
      it { expect(build_car.color).to eq 'red' }
      it { expect { build_car }.to change(BuildLog, :count).by(1) }
    end

    context 'with an unavailable color' do
      let(:color) { 'lightblue' }

      it { expect { build_car }.to raise_error(CarError).with_message('Unavailable color.') }
    end
  end
end

The code above smells, so I think we need to have something like an option to configure the cop to make it more flexible. If this idea sounds reasonable to others, I would love to make a PR. Any opinions?

pirj commented 2 years ago

How do you feel about:

RSpec.describe Car do
  describe '.build' do
    def build_car!
      Car.build(color: color)
    end

    subject(:car_color) { build_car!(color: color).color }

    context 'with an available color' do
      it { expect(car_color).to eq 'red' }
      it { expect { build_car! }.to change(BuildLog, :count).by(1) }
    end

    context 'with an unavailable color' do
      let(:color) { 'lightblue' }

      it { expect { build_car! }.to raise_error(CarError).with_message('Unavailable color.') }
    end
  end
end

From RSpec docs:

we recommend that you reserve it (subject helper) for support of custom matchers and/or extension libraries that hide its use from examples. We recommend using the named helper method over subject in examples.

You might not have the best example, but the previously mentioned discussion seems to come to a conclusion that named subjects facilitate reading specs:

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

j-miyake commented 2 years ago

Thanks for the suggestion and some useful links. I was mistaken about the use of subject :)

Just for additional reference, I found an old discussion on a similar topic. https://github.com/betterspecs/betterspecs/issues/7#issuecomment-150391098