Fix raise error when the expectation target is an error instance. #236

Merged
merged 6 commits into from Apr 15, 2013

Conversation

Projects
None yet
2 participants
Owner

samphippen commented Apr 10, 2013

Not sure if this is the best way to do it, but it certainly seems to work. Thoughts?

Owner

samphippen commented Apr 10, 2013

Related #232

Owner

samphippen commented Apr 10, 2013

Also this gave some errors related to differs when I ran locally, I want to see if it's a local problem or if travis picks it up.

Owner

samphippen commented Apr 10, 2013

I also get these errors on master, any ideas @myronmarston ?

lib/rspec/matchers/built_in/raise_error.rb
+ def expected_error_class
+ (@expected_error.is_a? Class) ? @expected_error : @expected_error.class
+ end
+
@myronmarston

myronmarston Apr 10, 2013

Owner

This is interesting -- I didn't realize 1.9.3- supported rescuring an exception instance but 2.0.0 doesn't. Good find! I noticed something when playing around in irb:

2.0.0p0 :001 > $exc = StandardError.new("boom")
 => #<StandardError: boom>
2.0.0p0 :002 > def try
2.0.0p0 :003?>     raise $exc
2.0.0p0 :004?>   rescue $exc => e
2.0.0p0 :005?>     puts "Rescued #{e}"
2.0.0p0 :006?>   end
 => nil
2.0.0p0 :007 > try
TypeError: class or module required for rescue clause
    from (irb):4:in `rescue in try'
    from (irb):3:in `try'
    from (irb):7
    from /Users/myron/.rvm/rubies/ruby-2.0.0-p0/bin/irb:16:in `<main>'
2.0.0p0 :008 >

As the error indicates, rescue SomeModule is valid, too...and your is_a?(Class) conditional here will skip that case.

On top of that, I was just realizing that this logic will wrongly allow it to pass when you do this:

error = StandardError.new("foo")
expect {
  raise StandardError.new("bar")
}.to raise_error(error)

When a user passes an exception instance to raise_error, I think the expectation is that it will only pass if the raised error is == to the passed exception instance. The change here will allow it to pass if the exception class is the same.

I'm thinking that we may want to do a bit of a larger refactoring of this matcher....rather than having a rescue expected_class and another rescue Exception, maybe we should just have the rescue Exception part, and then the actual and expected exceptions can be compared using == and ===.

Regardless of the approach taken, it would be great to have additional spec coverage of the exception instance case to handle the example I put above.

(BTW, here's the 2.0.0 change: http://bugs.ruby-lang.org/issues/4438 )

@samphippen

samphippen Apr 10, 2013

Owner

Hi @myronmarston. I've updated this with some new specs to actually assert around this behaviour. So what I felt the best way to do this is compare whether actual == expected, or actual.is_a? expected. Thoughts?

samphippen added a commit to samphippen/rspec-expectations that referenced this pull request Apr 10, 2013

Fixes some weird travis failures noted on master and in #236
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
lib/rspec/matchers/built_in/raise_error.rb
- # This clause should be empty, but rcov will not report it as covered
- # unless something (anything) is executed within the clause
- "http://eigenclass.org/hiki.rb?rcov-0.8.0"
+ if @actual_error == @expected_error || @actual_error.is_a?(@expected_error)
@myronmarston

myronmarston Apr 10, 2013

Owner

I believe rescue actually uses === for matching (at least, that's what Exceptional Ruby said), so I think we should use that here:

if @actual_error == @expected_error || @expected_error === @actual_error
lib/rspec/matchers/built_in/raise_error.rb
@@ -76,10 +74,14 @@ def description
private
+ def expected_error_class
+ (@expected_error.is_a? Class) ? @expected_error : @expected_error.class
+ end
@myronmarston

myronmarston Apr 10, 2013

Owner

Looks like this method isn't used anymore...is that right?

@samphippen

samphippen Apr 10, 2013

Owner

Yes, I'll remove it tomorrow.

Owner

samphippen commented Apr 15, 2013

@myronmarston how's this looking now?

Owner

myronmarston commented Apr 15, 2013

@myronmarston how's this looking now?

Closer! But see my comment on === -- once that's fixed, we can merge this.

samphippen added some commits Apr 10, 2013

Fix raise error when the expectation target is an error instance.
This closes #232

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Change error instance implementation to match == instances, or classes
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Add some specs for fixed instance raise_error behaviour
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Remove the now unused expected_error_class method in raise_error
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Use === for type comparison instead of is_a?
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Owner

samphippen commented Apr 15, 2013

pew pew pew. Also rebase against master.

Owner

myronmarston commented Apr 15, 2013

Looks good. Feel free to merge once travis is green.

Owner

samphippen commented Apr 15, 2013

@myronmarston should I make my expectations more lax, or is there a way that I can get 1.8.7, 1.9.2 and ree to give me the strings instead of pointers on that travis backtrace?

Owner

myronmarston commented Apr 15, 2013

Can you interpolate the exception instances into the expected failure string to match what the matcher does?

Interpolate from exception instances in raise_error spec
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Owner

samphippen commented Apr 15, 2013

@myronmarston so I think this is now ready for merging. Anything I've missed?

myronmarston added a commit that referenced this pull request Apr 15, 2013

Merge pull request #236 from samphippen/fix-raise-error-with-instances
Fix raise error when the expectation target is an error instance.

@myronmarston myronmarston merged commit 7f02b50 into rspec:master Apr 15, 2013

1 check passed

default The Travis build passed
Details
Owner

myronmarston commented Apr 15, 2013

nope :). Thanks as always!

Owner

samphippen commented Apr 15, 2013

literally awesome. 🐴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment