Skip to content
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

Use after hook #10

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
17 changes: 0 additions & 17 deletions lib/resque_ext/resque.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,5 @@
module Resque
class << self
def enqueue_to(queue, klass, *args)
# Perform before_enqueue hooks. Don't perform enqueue if any hook returns false
before_hooks = Plugin.before_enqueue_hooks(klass).collect do |hook|
klass.send(hook, *args)
end
return nil if before_hooks.any? { |result| result == false }

result = Job.create(queue, klass, *args)
return nil if result == "EXISTED"

Plugin.after_enqueue_hooks(klass).each do |hook|
klass.send(hook, *args)
end

true
end

def enqueued?(klass, *args)
enqueued_in?(queue_from_class(klass), klass, *args)
end
Expand Down
8 changes: 8 additions & 0 deletions lib/resque_solo/unique_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ def ttl
def lock_after_execution_period
@lock_after_execution_period ||= 0
end

# We want this to run first in before_enqueue_hooks (which are alpha sorted), so name appropriately
def before_enqueue_001_solo_job(*args)

Choose a reason for hiding this comment

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

Really excited to find resque_solo, the unreleased changes to make it not call after_enqueue_hooks if the job was not created, and this PR which makes the implementation hook based.

Why does the order of execution of before_enqueue hooks matter?

All before_enqueue_hooks will be invoked by Resque.enqueue_to, regardless of the result of any individual hook. So, it isn't clear to me why the order the hooks are invoked would matter.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, you're totally right. I just removed the misleading comment and name.

queue_name = self.instance_variable_get(:@queue)
item = { class: self.to_s, args: args }
!ResqueSolo::Queue.queued?(queue_name, item)
Copy link
Member

Choose a reason for hiding this comment

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

Can we write this all as one line?

!ResqueSolo::Queue.queued?(@queue, class: to_s, args: args)

end

end
end
end
Expand Down
2 changes: 2 additions & 0 deletions resque_solo.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,7 @@ Gem::Specification.new do |spec|
spec.add_development_dependency "bundler", "~> 1.6"
spec.add_development_dependency "fakeredis", "~> 0.4"
spec.add_development_dependency "minitest", "~> 5.8"
spec.add_development_dependency "minitest-reporters", "~> 1.1"
spec.add_development_dependency "rake", "~> 11.2"
spec.add_development_dependency "m", "~> 1.5"
end
19 changes: 13 additions & 6 deletions test/resque_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class ResqueTest < MiniTest::Spec
it "enqueues normal jobs" do
Resque.enqueue FakeJob, "x"
Resque.enqueue FakeJob, "x"
assert_equal 2, Resque.size(:normal)
queue_name = FakeJob.instance_variable_get(:@queue)
Copy link
Member

Choose a reason for hiding this comment

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

In all these tests, I prefer to simply hard-code the queue name rather than accessing an internal instance variable. So don't change the tests to use instance_variable_get. There's no reason to change any of these tests except line 47.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, should be good now I think... I haven't yet figured out the workflow to rebase and squash commits yet so let me know if I need to figure out how to make that happen.

assert_equal 2, Resque.size(queue_name)
end

it "is not able to report if a non-unique job was enqueued" do
Expand All @@ -26,19 +27,25 @@ class ResqueTest < MiniTest::Spec
describe "#enqueue_to" do
describe "non-unique job" do
it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeJob)
assert Resque.enqueue_to(:normal, FakeJob)
queue_name = FakeJob.instance_variable_get(:@queue)
assert Resque.enqueue_to(queue_name, FakeJob)
assert Resque.enqueue_to(queue_name, FakeJob)
end
end

describe "unique job" do
before do
@queue_name = FakeUniqueJob.instance_variable_get(:@queue)
end

it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeUniqueJob)
assert Resque.enqueue_to(@queue_name, FakeUniqueJob)
end

it "should return nil if job already existed" do
Resque.enqueue_to(:normal, FakeUniqueJob)
assert_nil Resque.enqueue_to(:normal, FakeUniqueJob)
Resque.enqueue_to(@queue_name, FakeUniqueJob)
assert Resque.enqueued?(FakeUniqueJob)
assert_nil Resque.enqueue_to(@queue_name, FakeUniqueJob)
end
end
end
Expand Down
5 changes: 4 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@
end

require "minitest/autorun"
require "minitest/reporters"
require "resque_solo"
require "fake_jobs"
require "fakeredis"
require "fakeredis/minitest"
begin
require "pry-byebug"
rescue LoadError
# ignore
end

Minitest::Reporters.use! [Minitest::Reporters::DefaultReporter.new({ color: true })]