From a2dd36c6d1d69ed353c9172be07e69949da00c7c Mon Sep 17 00:00:00 2001 From: David Stosik Date: Fri, 20 Feb 2015 09:25:46 +0900 Subject: [PATCH] MyWorker.perform_in(1.month) does not always schedule job in one month. At the moment, `MyWorker.perform_in(1.month)` always schedules a job in 30 days. When added to a date, `1.month` will add 1 to the date's month count, which means that it will add 28, 29, 30, or 31 days depending on the month and year. When I call `MyWorker.perform_in(1.month)`, I would expect the job to be scheduled next month, same day of the month, all the time. At the moment, it is true only four months in the year. My pull request tries to fix this by converting the interval object to a Float at the last possible moment. Plaese note that the test I wrote will fail only during months that do not have 30 days. Ideally, I would add a dependency to Timecop and freeze time to any day in a month of 28, 29 or 31 days. This could also avoid using `#assert_in_delta`, in favour of `#assert_equal`. Feel free to read my blog post [Rails' `1.month` has a variable length](http://dstosik.github.io/rails/2015/02/19/rails-1month-variable-length/) for more details. --- lib/sidekiq/worker.rb | 6 +++--- test/test_scheduling.rb | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/sidekiq/worker.rb b/lib/sidekiq/worker.rb index e41de0f4..1c54efb5 100644 --- a/lib/sidekiq/worker.rb +++ b/lib/sidekiq/worker.rb @@ -42,13 +42,13 @@ module Sidekiq def perform_in(interval, *args) int = interval.to_f - now = Time.now.to_f - ts = (int < 1_000_000_000 ? now + int : int) + now = Time.now + ts = (int < 1_000_000_000 ? (now + interval).to_f : int) item = { 'class' => self, 'args' => args, 'at' => ts } # Optimization to enqueue something now that is scheduled to go out now or in the past - item.delete('at'.freeze) if ts <= now + item.delete('at'.freeze) if ts <= now.to_f client_push(item) end diff --git a/test/test_scheduling.rb b/test/test_scheduling.rb index 80f610db..c2f8757f 100644 --- a/test/test_scheduling.rb +++ b/test/test_scheduling.rb @@ -30,6 +30,15 @@ class TestScheduling < Sidekiq::Test @redis.verify end + it 'schedules a job in one month' do + @redis.expect :zadd, true do |key, args| + assert_equal 'schedule', key + assert_in_delta 1.month.since.to_f, args[0][0].to_f, 1 + end + assert ScheduledWorker.perform_in(1.month, 'mike') + @redis.verify + end + it 'schedules a job via timestamp' do @redis.expect :zadd, true, ['schedule', Array] assert ScheduledWorker.perform_in(5.days.from_now, 'mike')