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

Sketch of fix for delegate private method stubbing #63

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 36 additions & 1 deletion lib/rspec/support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,42 @@ def self.define_optimized_require_for_rspec(lib, &require_relative)
# - BasicObject subclasses that mixin a Kernel dup (e.g. SimpleDelegator)
if RUBY_VERSION.to_i >= 2 && RUBY_ENGINE != 'rbx'
def self.method_handle_for(object, method_name)
KERNEL_METHOD_METHOD.bind(object).call(method_name)
if object_is_a_delegator?(object) && kernel_has_private_method?(method_name) && !object_has_own_method?(object, method_name)
Copy link
Member

Choose a reason for hiding this comment

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

Long line is long and this makes me sad :(

::Kernel.method(method_name)
else
KERNEL_METHOD_METHOD.bind(object).call(method_name)
end
end

# I came up with two implementations of this, one that does ancestor
# checking and one that does respond_to __getobj__ checking. I'm not
# sure which is more appropriate
Copy link
Member

Choose a reason for hiding this comment

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

If it's specifically an issue with ruby's delegator classes I'd favour the ancestor checking.

def self.object_is_a_delegator?(object)
object_has_getobj_method?(object)
end

def self.object_is_a_delegator?(object)
object_has_class_method?(object) && object_has_delegator_as_an_ancestor?(object)
end

def self.object_has_getobj_method?(object)
::Kernel.instance_method(:methods).bind(object).call.include?(:__getobj__)
end

def self.object_has_delegator_as_an_ancestor?(object)
object.class.ancestors.map(&:name).include?("Delegator")
Copy link
Member

Choose a reason for hiding this comment

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

Why map to a string? Just to handle the case of Delegator not being an available const?

end

def self.object_has_class_method?(object)
::Kernel.instance_method(:methods).bind(object).call.include?(:class)
end

def self.kernel_has_private_method?(method_name)
::Kernel.private_instance_methods.include?(method_name)
end

def self.object_has_own_method?(object, method_name)
object.methods.include?(method_name)
end
else
def self.method_handle_for(object, method_name)
Expand Down
79 changes: 79 additions & 0 deletions spec/rspec/support_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'rspec/support'
require "delegate"

module RSpec
describe Support do
Expand All @@ -21,6 +22,84 @@ def foo
expect(Support.method_handle_for(request, :uri).call).to eq "http://foo.com/"
end

context "with a delegator that does not implement a private kernel method" do
subject {
Copy link
Member

Choose a reason for hiding this comment

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

I know we differ on opinion about subject use, but can we at least make this a named subject. (I'd suggest delegator :P). Also I thought we standardised on do ... end for multiline blocks, { / } over multiple lines makes me sad :(

Class.new(SimpleDelegator) do
def foo
rand(2)
end
end
}

it "does not issue a warning when stubbing the private kernel method" do
hash = subject.new({})
expect(hash).not_to receive(:warn)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather see this use the new output matcher rather than mocking:

expect {
  allow(hash).to receive(:rand).and_call_original
}.not_to output.to_stderr

The mock expectation is more brittle as it assumes a warning implementation that calls warn on the object itself when in fact warn can be called on any object (or even directly on Kernel) in order to produce a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering about that, I knew the matcher existed but couldn't remember what it was called and asked a few other maintainers. That's definitely what I had intended.


allow(hash).to receive(:rand).and_call_original
hash.foo
end

it "replaces the rand method" do
hash = subject.new({})
expect(hash).not_to receive(:warn)

allow(hash).to receive(:rand).and_return(3)
expect(hash.foo).to eq(3)
end

it "puts the correct rand method back" do
hash = subject.new({})

allow(hash).to receive(:rand).and_call_original
RSpec::Mocks.space.reset_all
expect(hash.send(:rand)).to be_between(0,1)
end
end
context "with a delegator that does implement a private kernel method" do
subject {
Class.new(SimpleDelegator) do
def rand(n)
n
end

def foo
rand(2)
end
end
}

it "does not issue a warning when stubbing the private kernel method" do
hash = subject.new({})
expect(hash).not_to receive(:warn)

allow(hash).to receive(:rand).and_call_original
hash.foo
end

it "gets the correct method with and_call_original" do
hash = subject.new({})

allow(hash).to receive(:rand).and_call_original
expect(hash.foo).to eq(2)
end

it "it gets the correct stub implementation" do
hash = subject.new({})

allow(hash).to receive(:rand).and_return(3)
expect(hash.foo).to eq(3)
end

it "puts the correct method back" do
hash = subject.new({})

allow(hash).to receive(:rand).and_return(3)
RSpec::Mocks.space.reset_all
expect(hash.foo).to eq(2)
end
end


Copy link
Member

Choose a reason for hiding this comment

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

These specs seem to be specifying rspec-mocks behaviors from within the rspec-support test suite. My suggestion is to move these specs there and keep only specs here that are directly about the behavior of the support method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I took some specs I had in a scratch pad and ported them in before making this change. I'll totally move them though.

context "for a BasicObject subclass", :if => RUBY_VERSION.to_f > 1.8 do
let(:basic_class) do
Class.new(BasicObject) do
Expand Down