Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Give correct error message for expect {}.not_to raise_error(/foo/) #1456

Merged
merged 5 commits into from
May 11, 2024
Merged

Give correct error message for expect {}.not_to raise_error(/foo/) #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b98096c
Make not_to raise_error tests more specific
nevinera May 8, 2024
ee27f82
Handle Regexp like String when apportioning args
nevinera May 8, 2024
7df981b
Express raise_error test cases more explicitly
nevinera May 9, 2024
c08a500
Add a comment about where UndefinedValue is used
nevinera May 9, 2024
32ea28c
Add changelog entry for #1456
pirj May 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
### Development
[Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.12.3...main)
[Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.13.0...main)

Bug Fixes:

* Fix the false positive warning message for negated raise error with a regexp argument.
(Eric Mueller, #1456)

### 3.13.0 / 2024-02-04
[Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.12.4...v3.13.0)
Expand Down
6 changes: 5 additions & 1 deletion lib/rspec/matchers/built_in/raise_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ class RaiseError
# argument. We can't use `nil` for that because we need to warn when `nil` is
# passed in a different way. It's an Object, not a Module, since Module's `===`
# does not evaluate to true when compared to itself.
#
# Note; this _is_ the default value supplied for expected_error_or_message, but
# because there are two method-calls involved, that default is actually supplied
# in the definition of the _matcher_ method, `RSpec::Matchers#raise_error`
UndefinedValue = Object.new.freeze

def initialize(expected_error_or_message, expected_message, &block)
Expand All @@ -25,7 +29,7 @@ def initialize(expected_error_or_message, expected_message, &block)
when nil, UndefinedValue
pirj marked this conversation as resolved.
Show resolved Hide resolved
@expected_error = Exception
@expected_message = expected_message
when String
when String, Regexp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any change in behaviour?
Is looks like that previously a regexp would have been used as an exception?

This change indeed seems the right thing to do. I’m wondering, why it worked the same way before?

Is it because we still were tolerant to whatever it is an exception or something matcheable with a string message?

if values_match?(@expected_error, @actual_error) ||
               values_match?(@expected_error, actual_error_message)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So.. there is a change in "behavior", but it pretty much just affects what failure messages come out. These two ivars are really only used for that purpose, and aren't exposed externally, thankfully; the only impact I can find is that a slightly wrong failure message gets generated (it acts as though you supplied an exception if you supplied only a regex). Could have had larger impact, but the main place we look at the @expected_error, we only check @expected_error != Exception, which is not true for Regexps either (though I suppose if you supplied to raise_error(Exception, /foo/), you might also be able to get some wrong behavior?).

And the reason the specs didn't mind was just that the case checking this had a test that only checked the part of the failure message the outcomes had in common ("risks false positives"), which was still being produced.

@expected_error = Exception
@expected_message = expected_error_or_message
else
Expand Down
23 changes: 19 additions & 4 deletions spec/rspec/matchers/built_in/raise_error_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,28 @@ def to_s
end
end

RSpec.describe "expect { ... }.not_to raise_error(message)" do
it "issues a warning" do
expect_warning_with_call_site __FILE__, __LINE__+1, /risks false positives/
RSpec.describe "expect { ... }.not_to raise_error('message')" do
it "issues a warning when configured to do so", :warn_about_potential_false_positives do
RSpec::Expectations.configuration.warn_about_potential_false_positives = true
expect_warning_with_call_site __FILE__, __LINE__+1, /not_to raise_error\(message\)` risks false positives/
expect { raise 'blarg' }.not_to raise_error('blah')
end

it "supresses the warning when configured to do so", :warn_about_potential_false_positives do
RSpec::Expectations.configuration.warn_about_potential_false_positives = false
nevinera marked this conversation as resolved.
Show resolved Hide resolved
expect_no_warnings
expect { raise 'blarg' }.not_to raise_error('blah')
end
end

RSpec.describe "expect { ... }.not_to raise_error(/message/)" do
it "issues a warning when configured to do so", :warn_about_potential_false_positives do
RSpec::Expectations.configuration.warn_about_potential_false_positives = true
expect_warning_with_call_site __FILE__, __LINE__+1, /not_to raise_error\(message\)` risks false positives/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to dissect it. Before the change, this was printing a slightly different warning:

Using `expect { }.not_to raise_error(SpecificErrorClass)` risks false positives

and now it's a more appropriate for not_to raise_error(/regexp/):

Using `expect { }.not_to raise_error(message)` risks false positives

My apologies that it took that long to figure out.

expect { raise 'blarg' }.not_to raise_error(/blah/)
end

it "can supresses the warning when configured to do so", :warn_about_potential_false_positives do
it "supresses the warning when configured to do so", :warn_about_potential_false_positives do
RSpec::Expectations.configuration.warn_about_potential_false_positives = false
expect_no_warnings
expect { raise 'blarg' }.not_to raise_error(/blah/)
Expand Down
Loading