Merge pull request #1 from babylist/issue-31-fix-periodic-loop-option…

…s-serialization

Issue 31 fix periodic loop options serialization

lib/sidekiq/cronitor.rb

@@ -52,7 +52,11 @@ def periodic_job_key(worker)
         lop.history.find { |j| j[0] == worker.jid }
       end
 
-      periodic_job.present? && periodic_job.options.fetch('cronitor_key', nil)
+      if periodic_job.present?
+        options = periodic_job.options
+        options = JSON.parse(options) if options.is_a?(String)
+        options.fetch('cronitor_key', nil)
+      end
     end
 
     def options(worker)

lib/sidekiq/cronitor/periodic_jobs.rb

@@ -6,7 +6,7 @@ def self.sync_schedule!
       monitors_payload = []
       loops = Sidekiq::Periodic::LoopSet.new
       loops.each do |lop|
-        if lop.options.has_key?('cronitor_enabled') || lop.klass.constantize.sidekiq_options.has_key?('cronitor_enabled')
+        if parsed_options(lop).has_key?('cronitor_enabled') || Object.const_get(lop.klass).sidekiq_options.has_key?('cronitor_enabled')
           next unless fetch_option(lop, 'cronitor_enabled', Cronitor.auto_discover_sidekiq)
         else
           next if fetch_option(lop, 'cronitor_disabled', !Cronitor.auto_discover_sidekiq)
@@ -29,9 +29,17 @@ def self.sync_schedule!
       Sidekiq.logger.error("[cronitor] error during #{name}.#{__method__}: #{e}")
     end
 
+    def self.parsed_options(lop)
+      if lop.options.is_a?(String)
+        JSON.parse(lop.options)
+      else
+        lop.options
+      end
+    end
+
     def self.fetch_option(lop, key, default = nil)
-      lop.options.fetch(key, default) ||
-        lop.klass.constantize.sidekiq_options.fetch(key, default)
+      parsed_options(lop).fetch(key, default) ||
+        Object.const_get(lop.klass).sidekiq_options.fetch(key, default)
     end
   end
 end

spec/sidekiq/cronitor/periodic_jobs_spec.rb

@@ -2,12 +2,70 @@
 
 require 'sidekiq/cronitor/periodic_jobs'
 
+class DummyWorker
+  def self.sidekiq_options=(options)
+    @sidekiq_options ||= {}
+    @sidekiq_options.merge!(options)
+  end
+  def self.sidekiq_options
+    @sidekiq_options ||= {}
+  end
+  def self.reset_options
+    @sidekiq_options = {}
+  end
+end
+
 RSpec.describe Sidekiq::Cronitor::PeriodicJobs do
+  let(:cronitor_key) { "dummy-worker" }
+  let(:loops) { [job] }
+
   before do
-    allow(Sidekiq::Periodic::LoopSet).to receive(:new).and_return([])
+    class_double("Sidekiq::Periodic::LoopSet").as_stubbed_const
+    allow(Sidekiq::Periodic::LoopSet).to receive(:new).and_return(loops)
   end
 
   describe '.sync_schedule!' do
-    xit { expect { described_class.sync_schedule! }.not_to raise_error }
+    context "when no loops" do
+      let(:loops) { [] }
+      it { expect { described_class.sync_schedule! }.not_to raise_error }
+    end
+
+    context "when job options are stringified JSON" do
+      let(:job) {
+        instance_double(
+          "Sidekiq::Periodic::Loop",
+          klass: DummyWorker.name,
+          schedule: "* * * * *",
+          tz_name: "Etc/UTC",
+          options: {
+            "cronitor_key" => cronitor_key,
+            "cronitor_group" => "dummy-team"
+          }.to_json
+        )
+      }
+      it "updates monitors" do
+        expect(Cronitor::Monitor).to receive(:put).with(monitors: [hash_including(key: cronitor_key)])
+        described_class.sync_schedule!
+      end
+    end
+
+    context "when job options are a hash" do
+      let(:job) {
+        instance_double(
+          "Sidekiq::Periodic::Loop",
+          klass: DummyWorker.name,
+          schedule: "* * * * *",
+          tz_name: "Etc/UTC",
+          options: {
+            "cronitor_key" => cronitor_key,
+            "cronitor_group" => "dummy-team"
+          }
+        )
+      }
+      it "updates monitors" do
+        expect(Cronitor::Monitor).to receive(:put).with(monitors: [hash_including(key: cronitor_key)])
+        described_class.sync_schedule!
+      end
+    end
   end
 end