-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
Deprecate expect { }.not_to raise_error(SpecificErrorClass)
#244
Deprecate expect { }.not_to raise_error(SpecificErrorClass)
#244
Conversation
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@@ -14,6 +14,10 @@ def initialize(expected_error_or_message=Exception, expected_message=nil, &block | |||
end | |||
|
|||
def matches?(given_proc, negative_expectation = false) | |||
if negative_expectation == :negative_expectation && @expected_error != Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just check if negative_expectation
-- no need to compare it to the symbol. At the call site it passes the symbol because that's more expressive than passing true
(which doesn't mean a whole lot on its own).
Also, what do you think about extracting the @expected_error != Exception
part into a method that expresses it better? Maybe something like expecting_specific_exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what do you think about extracting the @expected_error != Exception part into a
method that expresses it better? Maybe something like expecting_specific_exception?
👍
Myron these are all great catches as usual. I'm currently facing the last 3 weeks of writing my master's thesis so expect very intermittent stuff from me. |
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
includes breaking out an "expecting_specific_exception?" query. Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@@ -14,6 +14,13 @@ def initialize(expected_error_or_message=Exception, expected_message=nil, &block | |||
end | |||
|
|||
def matches?(given_proc, negative_expectation = false) | |||
if negative_expectation && expecting_specific_exception? | |||
p "warning deprecation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous puts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! This is now gone.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Looks good to me, merge away |
Cool. |
…error-class Deprecate `expect { }.not_to raise_error(SpecificErrorClass)`
I'm updating the spec for this and noticed that |
Great idea. On Thu, May 16, 2013 at 10:26 AM, Sam Phippen notifications@github.comwrote:
|
Building now on a separate branch: https://github.com/rspec/rspec-expectations/compare/deprecate-not-to-raise-error-with-message There are some other issues addressed on that branch, but unless there is objection I'm just going to merge the branch once travis says it's OK to do so. |
Travis failed the build due to a failing cuke, but the same cuke is failing on master - looking into it. |
Two things (I can't figure out how to comment on that view, sadly):
Thanks for taking care of this! On Thu, May 16, 2013 at 10:50 AM, David Chelimsky
|
I'll clarify the failure message. Thx. |
The current travis failure is due to this:
This suggests that the new message expectation syntax is spec'd to disallow any matcher besides |
I know a bit about that. It's generated here: https://github.com/rspec/rspec-mocks/blob/master/lib/rspec/mocks/targets.rb#L11-L21 That error should only be raised if the |
I added this to the cuke and it passed:
Make sense to you, @myronmarston? On Thu, May 16, 2013 at 1:16 PM, Myron Marston notifications@github.comwrote:
|
Yep, thanks for fixing that! On Thu, May 16, 2013 at 11:19 AM, David Chelimsky
|
WDYT? |
Looks great! On Thu, May 16, 2013 at 12:03 PM, David Chelimsky
|
@myronmarston how does one assert that a function raises a certain type of error then? Especially when working on a CLI application, just checking that something raises an error is not enough - I need to check if the error is actually X, not Y. Is there a replacement to this method then? |
@sethvargo You can still do that: expect{}.to raise_error(ArgumentError) What is being deprecated is the negation: expect{}.not_to raise_error(ArgumentError)
|
Ahh okay. That's a bit confusing. Thanks! |
Checking that a specifc error is not raised is deprecated in rspec: rspec/rspec-expectations#244
according to: rspec/rspec-expectations#244, expecting a specific error to not be raised is deprecated and not really a good idea. Indeed, I found two other testcases where a different error was being raised other than the one we specified. I will fix those in later commits.
Related #231.
Deprecates raising with a specific error class and adds specs for this. Also a spec to ensure that the deprecation does not affect using this with a specific error class.