From f71fb8a424d934be7e054a8b27ea7ad4cb875992 Mon Sep 17 00:00:00 2001 From: Jared McFarland Date: Sat, 13 Apr 2013 13:05:01 -0700 Subject: [PATCH 1/2] Refactoring `Resque::JobPerformer#perform` Split the method up into smaller methods, each witha single responsibility. Added testing for the class and `#perform` method. --- lib/resque/job_performer.rb | 99 ++++++++++++++++++++----------- test/resque/job_performer_test.rb | 77 ++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 34 deletions(-) create mode 100644 test/resque/job_performer_test.rb diff --git a/lib/resque/job_performer.rb b/lib/resque/job_performer.rb index 2ec4e3d60..743d360c9 100644 --- a/lib/resque/job_performer.rb +++ b/lib/resque/job_performer.rb @@ -1,52 +1,83 @@ module Resque class JobPerformer + attr_reader :job, :job_args, :hooks + + # This is the actual performer for a single unit of work. It's called + # by Resque::Job#perform + # Args: + # palyoad_class: The class to call ::perform on + # args: An array of args to pass to the payload_class::perform + # hook: A hash with keys :before, :after and :around, all arrays of + # methods to call on the payload class with args def perform(payload_class, args, hooks) - job = payload_class - job_args = args || [] - job_was_performed = false + # Setup instance variables for the helper methods + @job = payload_class + @job_args = args || [] + @hooks = hooks + + # Before hooks can raise a Resque::DontPerform exception + # in which case we exit this method, returning false (because + # the job was never performed) + return false unless call_before_hooks + execute_job + call_hooks(:after) - # Execute before_perform hook. Abort the job gracefully if - # Resque::DontPerform is raised. + # Return whether or not the job was performed + performed? + end + + private + def call_before_hooks begin - hooks[:before].each do |hook| - job.send(hook, *job_args) - end + call_hooks(:before) + true rescue Resque::DontPerform - return false + false end + end + def execute_job # Execute the job. Do it in an around_perform hook if available. if hooks[:around].empty? - job.perform(*job_args) - job_was_performed = true + perform_job else - # We want to nest all around_perform plugins, with the last one - # finally calling perform - stack = hooks[:around].reverse.inject(nil) do |last_hook, hook| - if last_hook - lambda do - job.send(hook, *job_args) { last_hook.call } - end - else - lambda do - job.send(hook, *job_args) do - result = job.perform(*job_args) - job_was_performed = true - result - end - end - end - end - stack.call + call_around_hooks end + end + + def call_around_hooks + nested_around_hooks.call + end - # Execute after_perform hook - hooks[:after].each do |hook| - job.send(hook, *job_args) + # We want to nest all around_perform plugins, with the last one + # finally calling perform + def nested_around_hooks + final_hook = lambda { perform_job } + hooks[:around].reverse.inject(final_hook) do |last_hook, hook| + lambda { perform_hook(hook) { last_hook.call } } end + end + + def call_hooks(hook_type) + hooks[hook_type].each { |hook| perform_hook(hook) } + end + + def perform_hook(hook, &block) + job.__send__(hook, *job_args, &block) + end + + def perform_job + result = job.perform(*job_args) + job_performed + result + end + + def performed? + @performed ||= false + end - # Return true if the job was performed - job_was_performed + def job_performed + @performed = true end end end diff --git a/test/resque/job_performer_test.rb b/test/resque/job_performer_test.rb new file mode 100644 index 000000000..01b522b46 --- /dev/null +++ b/test/resque/job_performer_test.rb @@ -0,0 +1,77 @@ +require 'test_helper' + +describe Resque::JobPerformer do + before do + @mock = MiniTest::Mock.new + @job_performer = Resque::JobPerformer.new + @job_args = [:foo] + end + + describe '#perform' do + before do + @options = { + :before => [ + :before_one, + :before_two + ], + :around => [], + :after => [ + :after_one, + :after_two + ] + } + end + + it 'runs the before hooks' do + @mock.expect :before_one, true, @job_args + @mock.expect :before_two, true, @job_args + @mock.expect :perform, true, @job_args + @mock.expect :after_one, true, @job_args + @mock.expect :after_two, true, @job_args + @job_performer.perform(@mock, @job_args, @options).must_equal true + @mock.verify + end + + it 'returns false when a before mock raises DontPerform' do + @options = { + :before => [:before_one], + :after => [], + :around => [] + } + def @mock.before_one(*args) + raise Resque::DontPerform + end + @mock.expect :perform, nil, @job_args + @job_performer.perform(@mock, @job_args, @options).must_equal false + end + + describe 'when around_perform is present' do + before do + @options = { + :before => [], + :around => [ + :around_one, + :around_two + ], + :after => [] + } + end + + it 'runs the around hooks' do + @mock.expect :perform, true, @job_args + @mock.expect :around_two, true, @job_args + @mock.expect :around_one, true, @job_args + def @mock.around_two(*args) + method_missing(:around_two, *args) + yield + end + def @mock.around_one(*args) + method_missing(:around_one, *args) + yield + end + @job_performer.perform(@mock, @job_args, @options) + @mock.verify + end + end + end +end From 1b76fc176c769902b661768b6e1540d1cbda4631 Mon Sep 17 00:00:00 2001 From: Jared McFarland Date: Sun, 14 Apr 2013 15:29:38 -0700 Subject: [PATCH 2/2] Removing some un-need comments and correcting a typo --- lib/resque/job_performer.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/resque/job_performer.rb b/lib/resque/job_performer.rb index 743d360c9..c613ead68 100644 --- a/lib/resque/job_performer.rb +++ b/lib/resque/job_performer.rb @@ -10,19 +10,17 @@ class JobPerformer # hook: A hash with keys :before, :after and :around, all arrays of # methods to call on the payload class with args def perform(payload_class, args, hooks) - # Setup instance variables for the helper methods @job = payload_class @job_args = args || [] @hooks = hooks - # Before hooks can raise a Resque::DontPerform exception + # before_hooks can raise a Resque::DontPerform exception # in which case we exit this method, returning false (because # the job was never performed) return false unless call_before_hooks execute_job call_hooks(:after) - # Return whether or not the job was performed performed? end