-
Notifications
You must be signed in to change notification settings - Fork 961
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
Ruby 2.0 tests #194
Ruby 2.0 tests #194
Conversation
Works good for me 👍 |
@@ -598,13 +598,17 @@ def self.name | |||
@child1.default_options[:imaginary_option].should be_a Proc | |||
end | |||
|
|||
def is(a) | |||
RubyVM::InstructionSequence.of(a) |
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.
What does this even do? Does it work on 1.8 and 1.9 as well?
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.
When I wrote that I tested briefly the cases where proc equality should be true and false with RubyVM::InstructionSequence#of
. It seemed to be doing what I thought. Now however with a clear mind, it seems it is not, and it makes sense that it couldn't. Proc equality is a very complicated issue.
a = RubyVM::InstructionSequence.of -> { 1 + 2 }
b = RubyVM::InstructionSequence.of -> { 1 + 2 }
puts a == b
If this was functional code that would display true. Sadly it doesn't.
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.
So the main goal is to test that the parent and child do not have the same proc. I suppose we could just make the procs return something that is known, like 1 + 2 for parent and 2 + 2 for child. We could then just test that the procs return the correct value when called, instead of using equality as the test.
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.
Yea exactly what I was thinking.
Quick question for you though. I'm trying to imagine why you are duplicating the procs for every subclass of HTTParty
, is it just a security to prevent someone from hacking into the procs and changing what they do. That's the best I could think of.
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.
Trying to remember. I'm thinking it had to do with memcache and marshaling or something. It was a pull request, not something I invented, as most of httparty is.
Everything is passing except deciding on how to handle backward compaitibility with the |
@@ -599,11 +599,11 @@ def self.name | |||
end | |||
|
|||
it 'should dup the proc on the child class' do | |||
imaginary_option = lambda { "This is a new lambda" } | |||
imaginary_option = lambda { 2 * 3.14 } | |||
@parent.default_options[:imaginary_option] = imaginary_option | |||
@parent.default_options[:imaginary_option].should be_equal imaginary_option |
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.
Should this be testing equality? Seems like it should be the same as below.
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.
I'm not 100% sure what you mean, my change tests the call
value of the proc for equality, and keeps the test for the proc not being the same object.
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.
I thought the point of changing the block and swapping out the equality check for the invocation check was because the equality check didn't work, but the @parent is still using the equality check.
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.
Oh I see what you mean, yea that makes perfect sense.
@@ -22,5 +36,5 @@ | |||
|
|||
Then /it should raise (?:an|a) ([\w:]+) exception/ do |exception| | |||
@exception_from_httparty.should_not be_nil | |||
@exception_from_httparty.class.name.should eql(exception) | |||
@exception_from_httparty.should be_a constantize(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.
This fixes the cucumber issues with subclass exceptions not counting as raised.
Is there anything left outstanding on this I can fix? |
see #193 for information on these changes.