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

[nevinera]

Found this while adding coverage - there was a test that ought to have covered the line in question, but didn't because it was actually slightly too loose. Tightening it up shows that it's getting the wrong error message, because the RaiseError matcher is handling expected_error_or_message by checking the type, and it's looking for String, but not Regexp there.

Figured it'd make more sense as its own PR, since it's not just test coverage. (The line is covered afterwards though!)

Failure with just the test change:

  1) expect { ... }.not_to raise_error(/message/) issues a warning
     Failure/Error: expect { raise 'blarg' }.not_to raise_error(/blah/)

       Kernel received :warn with unexpected arguments
         expected: (match /not_to raise_error\(message\)` risks false positives/ and match /Called from \/Users\/ericmueller\/src\/rspec-expectations\/spec\/rspec\/matchers\/built_in\/raise_error_spec.rb:309/)
              got: ("WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)` risks false positives, since lite...ons/spec/rspec/matchers/built_in/raise_error_spec.rb:309:in `block (2 levels) in <top (required)>'.")
     # ./spec/rspec/matchers/built_in/raise_error_spec.rb:309:in `block (2 levels) in <top (required)>'

[pirj]

No objections on the changes, just a few notes to spark a discussion on if this can be further improved. Partially caused by the lack of morning coffee.

[pirj]

Wondering why ‘warn_about_potential_false_positives’ metadata is needed here to warn, but it warns anyway in other examples? Is it to restore the setting to the default? It just reads confusingly when used this way.

[pirj]

I see that we do the same in other examples in this file.

[nevinera]

I didn't know about this until you asked (and I wasn't sure until I tested it out), but adding a shared context like this one (with a tag specified) causes that context to be automatically included in any context that supplies that same tag. So including this metadata causes an automatic cleanup to occur. I, of course, had just copied it from other tests without thinking :-)

[pirj]

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)

[nevinera]

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.

[pirj]

Thank you!

[pirj]

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.