New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MultipleExpectation and exception matching #379
Comments
You should wrap this test in |
@backus the 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. |
Have you tried writing |
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. |
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 |
* Add chained matchers example to raise_error Being new to rspec, it wasn't clear that I could pass composed matchers to 'raise_error'. This commit adds an example of a chained matcher being passed to 'raise_error'. This is useful for testing custom attributes on an error while still satisfying the one 'expect' statement per test case enforced by rubocop-rspec. See rubocop/rubocop-rspec#379 for more details.
I had a similar problem here, but opted for 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. |
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: 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 |
@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. |
I guess this issue boils down to adding a 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? |
@pirj sorry, my example was not complete, my |
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 |
…ec-expectations#1087) * Add chained matchers example to raise_error Being new to rspec, it wasn't clear that I could pass composed matchers to 'raise_error'. This commit adds an example of a chained matcher being passed to 'raise_error'. This is useful for testing custom attributes on an error while still satisfying the one 'expect' statement per test case enforced by rubocop-rspec. See rubocop/rubocop-rspec#379 for more details. --- This commit was imported from rspec/rspec-expectations@dc0ecfd.
The implementation revealed some more considerations, and wr decided not to proceed with the fix. |
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: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?
The text was updated successfully, but these errors were encountered: