Permalink
Browse files

Proof of concept to show that wrapping hook blocks in a proxy support…

…s usin lambdas.
  • Loading branch information...
1 parent 3a83a89 commit 4e735e185356a3c5204bc1241d73c3e926b57f42 @dchelimsky dchelimsky committed Feb 24, 2013
Showing with 29 additions and 4 deletions.
  1. +12 −4 lib/rspec/core/hooks.rb
  2. +17 −0 spec/rspec/core/hooks_spec.rb
View
@@ -20,7 +20,7 @@ module BeforeHookExtension
include HookExtension
def run(example)
- example.instance_eval(&self)
+ example.instance_eval(&block)
end
def display_name
@@ -32,7 +32,7 @@ module AfterHookExtension
include HookExtension
def run(example)
- example.instance_eval_with_rescue("in an after hook", &self)
+ example.instance_eval_with_rescue("in an after hook", &block)
end
def display_name
@@ -90,7 +90,7 @@ def with(example, initial_procsy)
def run
inject(@initial_procsy) do |procsy, around_hook|
Example.procsy(procsy.metadata) do
- @example.instance_eval_with_args(procsy, &around_hook)
+ @example.instance_eval_with_args(procsy, &around_hook.block)
end
end.call
end
@@ -431,6 +431,14 @@ def around_each_hooks_for(example, initial_procsy=nil)
private
+ class HookProxy
+ attr_reader :block
+
+ def initialize(&block)
+ @block = block
+ end
+ end
+
SCOPES = [:each, :all, :suite]
EXTENSIONS = {
@@ -457,7 +465,7 @@ def after_each_hooks_for(example)
def register_hook prepend_or_append, hook, *args, &block
scope, options = scope_and_options_from(*args)
- hooks[hook][scope].send(prepend_or_append, block.extend(EXTENSIONS[hook]).with(options))
+ hooks[hook][scope].send(prepend_or_append, HookProxy.new(&block).extend(EXTENSIONS[hook]).with(options))
@myronmarston

myronmarston Feb 24, 2013

Owner

I think using composition like this is a better approach than modifying the block directly. That said, I think we should take this a step further and do:

HOOK_CLASSES[hook].new(block)

...rather than extending the block with a module. Once we're wrapping the block with our own class, I think we're best off moving the logic directly in there rather than keeping it in an extension module. I would expect such a change to have a positive perf improvement; as I've read elsewhere that extending modules onto objects at runtime tends to blow method caches and introduce performance degredations.

@myronmarston

myronmarston Feb 24, 2013

Owner

Actually, looking at this some more, this approach allows you to do away with the semi-awkward with call: with looks (to me, at least) like it's trying to stand in place of an initializer since the options state really belongs with the hook object. So this entire line becomes:

hooks[hook][scope].send(prepend_or_append, HOOK_CLASSES[hook].new(block, options))
@myronmarston

myronmarston Feb 24, 2013

Owner

Actually, I'm behind...I just noticed you pushed most of what I had suggested here already :). I made the rest of the changes in eb0a45d.

end
def find_hook(hook, scope, example_or_group, initial_procsy)
@@ -246,5 +246,22 @@ def yielder
end
end
end
+
+ describe "lambda" do
+ it "can be used as a hook" do
+ messages = []
+ count = 0
+ hook = lambda {|e| messages << "hook #{count = count + 1}"; e.run }
+
+ RSpec.configure do |c|
+ c.around(:each, &hook)
+ c.around(:each, &hook)
+ end
+
+ group = ExampleGroup.describe { example { messages << "example" } }
+ group.run
+ expect(messages).to eq ["hook 1", "hook 2", "example"]
+ end
+ end
end
end

2 comments on commit 4e735e1

Owner

dchelimsky replied Feb 24, 2013

Like minds think alike, or something like that :)

As the commit message says, this was just a proof of concept to show that using a proxy solves the issue. I've already committed the "real fix" 0c56b9b and it looks very much like what you're suggesting.

Owner

dchelimsky replied Feb 24, 2013

Ah, didn't remove with. I'll do that as well.

Please sign in to comment.