MultipleExpectation and exception matching

[lsimoneau]

I really like enforcing one expectation per example. But with RSpec's exception matching, you need two expect calls if you want to assert something about an exception:

expect { my_code }.to raise_error(MyErrorType) do |error|
  expect(error.message).to match /something went wrong/
end

There's no way to match against the error message without first wrapping the call, and RSpec only lets you do that with an expect.

Do you think it would be a good idea to modify the behaviour of the MultipleExpectation cop to account for this? I'm happy to try to implement it if we agree on how it should work. My instinct would be that the default would allow the above code, but would fail if there were extra expectations inside the raise_error block. And maybe have a style option to disallow the above as well?

[backus]

You should wrap this test in aggregate_failures. We just released an improvement that treats aggregated failures as one assertion. Can you let me know if that works for you?

[lsimoneau]

@backus the aggregate_failures is superfluous in this case and would only be there to satisfy the cop. In the above example RSpec will never report multiple failures, it will report either that no exception was raised or that it did but failed the assertion about its contents.

It doesn't seem right to add extra cruft to the test to make the cop happy. The above test is in my opinion correctly implemented, but I'd still like to stop people from doing 2 expectations elsewhere in the codebase.

[backus]

Have you tried writing raise_error(MyErrorType).with_message(/something went wrong/)?

[lsimoneau]

Yes, sorry. The above example was bad in that sense because there's another way of writing it. The actual example in our codebase is a custom exception class which has some additional data fields on it which are not natively supported by RSpec's matchers and require the block syntax to check.

[backus]

I would probably be fine with adding some kind of support like you're proposing here but I'd prefer we do a better job recommending people use composable matchers. Maybe you would be able to do something like this?

module Foo
  class Boom < StandardError
    attr_reader :source

    def initialize(source)
      @source = source
    end
  end

  def self.boom
    raise Boom, :dynamite
  end
end

RSpec.describe Foo do
  it 'with multiple expectations' do
    expect { Foo.boom }.to raise_error(Foo::Boom) do |error|
      expect(error.source).to be(:dynamite)
    end
  end

  it 'with composable matchers' do
    expect { Foo.boom }.to raise_error(an_instance_of(Foo::Boom).and having_attributes(source: :dynamite))
  end
end

[pirj]

I had a similar problem here, but opted for aggregate_failures.

Another option would be to reset the counter for nested expectations (but still count them).

There's nothing wrong with the code @lsimoneau provided, and it not counting a single nested expectation can be the default.

[danielnc]

I have another example that I am not sure how to solve or if this is an issue:

active_record_relation is a: ActiveRecord::Relation, so I couldn't find a way to use something similar to: an_instance_of(ActiveRecord::Relation).and(having_attributes(to_a: [record_1, record_2])

    it "find correct records" do
      expect(described_instance).to have_received(:process_response) do |active_record_relation|
        expect(active_record_relation).to contain_exactly(record_1, record_2)
      end
    end

[pirj]

@danielnc So

expect(described_instance)
  .to have_received(:process_response)
    .with(contain_exactly(record_1, record_2))

doesn't work for you?

Argument matchers shouldn't be counted towards the total number of expectations in the example.

[pirj]

I guess this issue boils down to adding a IgnoreNested configuration option for RSpec/MultipleExpectations cop to skip counting nested expectations, e.g.:

expect { my_code }.to raise_error(MyErrorType) do |error|
  expect(error.cause).to be_a NetworkError
end

I already see how people abuse it :D

expect { my_code }.to raise_error(MyErrorType) do |error|
  expect(something_completely_irrelevant).to be_valid
end

Would anyone like to tackle the addition of such an option?

[danielnc]

@pirj sorry, my example was not complete, my process_response method receives 4 arguments. only the first one is the AR relation, I would like to match them all, with that syntax you just described I am not sure how to accomplish this

[pirj]

Should be trivial enough with any number of arguments, check this out https://relishapp.com/rspec/rspec-mocks/v/3-9/docs/setting-constraints/matching-arguments @danielnc

[pirj]

The implementation revealed some more considerations, and wr decided not to proceed with the fix.