Skip to content

Loading…

Altered Delayed::Command to work with or without Rails #634

Merged
merged 6 commits into from

5 participants

@bethesque

When running the delayed_job daemon in a codebase without Rails, we've had to create a fake Rails module to provide the logger and root methods that Delayed::Command expects. Given the rest of the delayed job code can run without Rails, it would be nice if the Command behaved the same way.

Please let me know if the tests are sufficient or not. Given the entire class was lacking a test, I have only added tests for the code that I altered.

@col

This has always bugged me. Nice work!
:+1:

@betesh

This PR assumes that if Rails is defined, the entire Rails stack is loaded. This is not necessarily the case. See rails/rails-html-sanitizer#25 / rails/rails-html-sanitizer#26

A more robust approach would use defined?(Rails.root) and defined?(Rails.logger) individually.

@paulspringett

@bethesque any progress with this? I'm keen on seeing this merged so happy to take a look at sorting the specs.

bethesque added some commits
@bethesque bethesque Merge branch 'master' of github.com:collectiveidea/delayed_job into c…
…ommand_without_rails

Conflicts:
	lib/delayed/command.rb
	spec/delayed/command_spec.rb
5da2c59
@bethesque bethesque Check for existance of Rails.root and Rails.logger separately.
@betesh pointed out that one may be defined without the other, and the existance of
both cannot be implied from the existance of the Rails module.
93e9c36
@bethesque

I have modified the code as per @betesh's suggestion, to check for the existence of Rails.logger and Rails.root separately. It should be good to pull now, let me know if there are any changes you would like.

@bethesque

Damn you Rubocop! Double quotes are fine! http://viget.com/extend/just-use-double-quoted-ruby-strings

The merge build is failing because the double logger is leaking across example intermittently. I've noticed other intermittent failures as I've been running the tests. Is this normal?

@bethesque

Fixed.

@paulspringett

@bethesque thank you! @albus522 @sferik could this be merged please?

@sferik

Looks great. Thanks!

@sferik sferik merged commit 6ade20c into collectiveidea:master

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 17, 2014
  1. @bethesque
Commits on Feb 13, 2015
  1. @bethesque

    Merge branch 'master' of github.com:collectiveidea/delayed_job into c…

    bethesque committed
    …ommand_without_rails
    
    Conflicts:
    	lib/delayed/command.rb
    	spec/delayed/command_spec.rb
  2. @bethesque

    Check for existance of Rails.root and Rails.logger separately.

    bethesque committed
    @betesh pointed out that one may be defined without the other, and the existance of
    both cannot be implied from the existance of the Rails module.
  3. @bethesque

    Making Rubocop happy

    bethesque committed
  4. @bethesque

    Making Rubocop happy about extending Struct.new

    bethesque committed
    What's so wrong about that anyway????
  5. @bethesque

    Stubbed the calls to set the logger on Delayed::Worker module.

    bethesque committed
    The class variable was leaking into other tests.
Showing with 169 additions and 16 deletions.
  1. +29 −5 lib/delayed/command.rb
  2. +2 −1 spec/autoloaded/instance_struct.rb
  3. +2 −1 spec/autoloaded/struct.rb
  4. +2 −0 spec/daemons.rb
  5. +130 −7 spec/delayed/command_spec.rb
  6. +4 −2 spec/sample_jobs.rb
View
34 lib/delayed/command.rb
@@ -6,15 +6,19 @@
end
end
require 'optparse'
+require 'pathname'
module Delayed
class Command # rubocop:disable ClassLength
attr_accessor :worker_count, :worker_pools
+ DIR_PWD = Pathname.new Dir.pwd
+
def initialize(args) # rubocop:disable MethodLength
@options = {
:quiet => true,
- :pid_dir => "#{Rails.root}/tmp/pids"
+ :pid_dir => "#{root}/tmp/pids",
+ :log_dir => "#{root}/log"
}
@worker_count = 1
@@ -42,6 +46,9 @@ def initialize(args) # rubocop:disable MethodLength
opt.on('--pid-dir=DIR', 'Specifies an alternate directory in which to store the process ids.') do |dir|
@options[:pid_dir] = dir
end
+ opt.on('--log-dir=DIR', 'Specifies an alternate directory in which to store the delayed_job log.') do |dir|
+ @options[:log_dir] = dir
+ end
opt.on('-i', '--identifier=n', 'A numeric identifier for the worker.') do |n|
@options[:identifier] = n
end
@@ -114,18 +121,19 @@ def run_process(process_name, options = {})
end
def run(worker_name = nil, options = {})
- Dir.chdir(Rails.root)
+ Dir.chdir(root)
Delayed::Worker.after_fork
- Delayed::Worker.logger ||= Logger.new(File.join(Rails.root, 'log', 'delayed_job.log'))
+ Delayed::Worker.logger ||= Logger.new(File.join(@options[:log_dir], 'delayed_job.log'))
worker = Delayed::Worker.new(options)
worker.name_prefix = "#{worker_name} "
worker.start
rescue => e
- Rails.logger.fatal e
STDERR.puts e.message
- exit 1
+ STDERR.puts e.backtrace
+ ::Rails.logger.fatal(e) if rails_logger_defined?
+ exit_with_error_status
end
private
@@ -142,5 +150,21 @@ def parse_worker_pool(pool)
worker_count = (worker_count || 1).to_i rescue 1
@worker_pools << [queues, worker_count]
end
+
+ def root
+ @root ||= rails_root_defined? ? ::Rails.root : DIR_PWD
+ end
+
+ def rails_root_defined?
+ defined?(::Rails.root)
+ end
+
+ def rails_logger_defined?
+ defined?(::Rails.logger)
+ end
+
+ def exit_with_error_status
+ exit 1
+ end
end
end
View
3 spec/autoloaded/instance_struct.rb
@@ -1,5 +1,6 @@
module Autoloaded
- class InstanceStruct < ::Struct.new(nil)
+ InstanceStruct = ::Struct.new(nil)
+ class InstanceStruct
def perform
end
end
View
3 spec/autoloaded/struct.rb
@@ -1,6 +1,7 @@
# Make sure this file does not get required manually
module Autoloaded
- class Struct < ::Struct.new(nil)
+ Struct = ::Struct.new(nil)
+ class Struct
def perform
end
end
View
2 spec/daemons.rb
@@ -0,0 +1,2 @@
+# Fake "daemons" file on the spec load path to allow spec/delayed/command_spec.rb
+# to test the Delayed::Command class without actually adding daemons as a dependency.
View
137 spec/delayed/command_spec.rb
@@ -2,6 +2,129 @@
require 'delayed/command'
describe Delayed::Command do
+ let(:options) { [] }
+ let(:logger) { double('Logger') }
+
+ subject { Delayed::Command.new options }
+
+ before do
+ allow(Delayed::Worker).to receive(:after_fork)
+ allow(Dir).to receive(:chdir)
+ allow(Logger).to receive(:new).and_return(logger)
+ allow_any_instance_of(Delayed::Worker).to receive(:start)
+ allow(Delayed::Worker).to receive(:logger=)
+ allow(Delayed::Worker).to receive(:logger).and_return(nil, logger)
+ end
+
+ shared_examples_for 'uses --log-dir option' do
+ context 'when --log-dir is specified' do
+ let(:options) { ['--log-dir=/custom/log/dir'] }
+
+ it 'creates the delayed_job.log in the specified directory' do
+ expect(Logger).to receive(:new).with('/custom/log/dir/delayed_job.log')
+ subject.run
+ end
+ end
+ end
+
+ describe 'run' do
+ it 'sets the Delayed::Worker logger' do
+ expect(Delayed::Worker).to receive(:logger=).with(logger)
+ subject.run
+ end
+
+ context 'when Rails root is defined' do
+ let(:rails_root) { Pathname.new '/rails/root' }
+ let(:rails) { double('Rails', :root => rails_root) }
+
+ before do
+ stub_const('Rails', rails)
+ end
+
+ it 'runs the Delayed::Worker process in Rails.root' do
+ expect(Dir).to receive(:chdir).with(rails_root)
+ subject.run
+ end
+
+ context 'when --log-dir is not specified' do
+ it 'creates the delayed_job.log in Rails.root/log' do
+ expect(Logger).to receive(:new).with('/rails/root/log/delayed_job.log')
+ subject.run
+ end
+ end
+
+ include_examples 'uses --log-dir option'
+ end
+
+ context 'when Rails root is not defined' do
+ let(:rails_without_root) { double('Rails') }
+
+ before do
+ stub_const('Rails', rails_without_root)
+ end
+
+ it 'runs the Delayed::Worker process in $PWD' do
+ expect(Dir).to receive(:chdir).with(Delayed::Command::DIR_PWD)
+ subject.run
+ end
+
+ context 'when --log-dir is not specified' do
+ it 'creates the delayed_job.log in $PWD/log' do
+ expect(Logger).to receive(:new).with("#{Delayed::Command::DIR_PWD}/log/delayed_job.log")
+ subject.run
+ end
+ end
+
+ include_examples 'uses --log-dir option'
+ end
+
+ context 'when an error is raised' do
+ let(:test_error) { Class.new(StandardError) }
+
+ before do
+ allow(Delayed::Worker).to receive(:new).and_raise(test_error.new('An error'))
+ allow(subject).to receive(:exit_with_error_status)
+ allow(STDERR).to receive(:puts)
+ end
+
+ it 'prints the error message to STDERR' do
+ expect(STDERR).to receive(:puts).with('An error')
+ subject.run
+ end
+
+ it 'exits with an error status' do
+ expect(subject).to receive(:exit_with_error_status)
+ subject.run
+ end
+
+ context 'when Rails logger is not defined' do
+ let(:rails) { double('Rails') }
+
+ before do
+ stub_const('Rails', rails)
+ end
+
+ it 'does not attempt to use the Rails logger' do
+ subject.run
+ end
+ end
+
+ context 'when Rails logger is defined' do
+ let(:rails_logger) { double('Rails logger') }
+ let(:rails) { double('Rails', :logger => rails_logger) }
+
+ before do
+ stub_const('Rails', rails)
+ end
+
+ it 'logs the error to the Rails logger' do
+ expect(rails_logger).to receive(:fatal).with(test_error)
+ subject.run
+ end
+ end
+ end
+ end
+
describe 'parsing --pool argument' do
it 'should parse --pool correctly' do
command = Delayed::Command.new(['--pool=*:1', '--pool=test_queue:4', '--pool=mailers,misc:2'])
@@ -40,13 +163,13 @@
expect(Dir).to receive(:mkdir).with('./tmp/pids').once
[
- ['delayed_job.0', {:quiet => true, :pid_dir => './tmp/pids', :queues => []}],
- ['delayed_job.1', {:quiet => true, :pid_dir => './tmp/pids', :queues => ['test_queue']}],
- ['delayed_job.2', {:quiet => true, :pid_dir => './tmp/pids', :queues => ['test_queue']}],
- ['delayed_job.3', {:quiet => true, :pid_dir => './tmp/pids', :queues => ['test_queue']}],
- ['delayed_job.4', {:quiet => true, :pid_dir => './tmp/pids', :queues => ['test_queue']}],
- ['delayed_job.5', {:quiet => true, :pid_dir => './tmp/pids', :queues => %w[mailers misc]}],
- ['delayed_job.6', {:quiet => true, :pid_dir => './tmp/pids', :queues => %w[mailers misc]}]
+ ['delayed_job.0', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => []}],
+ ['delayed_job.1', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => ['test_queue']}],
+ ['delayed_job.2', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => ['test_queue']}],
+ ['delayed_job.3', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => ['test_queue']}],
+ ['delayed_job.4', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => ['test_queue']}],
+ ['delayed_job.5', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => %w[mailers misc]}],
+ ['delayed_job.6', {:quiet => true, :pid_dir => './tmp/pids', :log_dir => './log', :queues => %w[mailers misc]}]
].each do |args|
expect(command).to receive(:run_process).with(*args).once
end
View
6 spec/sample_jobs.rb
@@ -1,4 +1,5 @@
-class NamedJob < Struct.new(:perform)
+NamedJob = Struct.new(:perform)
+class NamedJob
def display_name
'named_job'
end
@@ -26,7 +27,8 @@ def perform
end
end
-class CustomRescheduleJob < Struct.new(:offset)
+CustomRescheduleJob = Struct.new(:offset)
+class CustomRescheduleJob
cattr_accessor :runs
@runs = 0
def perform
Something went wrong with that request. Please try again.