From 666fcbac7b0b010170b9cea9b33532a4d666f063 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 5 Feb 2024 16:33:51 +0200 Subject: [PATCH] chore(ci): add test coverage from bropoplpush --- .rubocop.yml | 16 ++- CHANGELOG.md | 8 +- README.md | 2 +- doc/SidekiqUniqueJobs/Script.html | 4 +- doc/file.CHANGELOG.html | 10 +- doc/file.README.html | 4 +- doc/index.html | 4 +- lib/sidekiq_unique_jobs/script/script.rb | 7 +- myapp/config/initializers/sidekiq.rb | 2 +- .../redis_script/client/execute_spec.rb | 134 ++++++++++++++++++ .../redis_script/client_spec.rb | 24 ++++ .../redis_script/config_spec.rb | 59 ++++++++ .../redis_script/logging_spec.rb | 65 +++++++++ .../redis_script/lua_error_spec.rb | 29 ++++ .../redis_script/scripts_spec.rb | 64 +++++++++ .../sidekiq_unique_jobs/script/caller_spec.rb | 2 +- .../script/client/execute_spec.rb | 134 ++++++++++++++++++ .../sidekiq_unique_jobs/script/client_spec.rb | 24 ++++ .../sidekiq_unique_jobs/script/config_spec.rb | 59 ++++++++ .../script/logging_spec.rb | 65 +++++++++ .../script/lua_error_spec.rb | 29 ++++ .../script/scripts_spec.rb | 64 +++++++++ spec/spec_helper.rb | 6 +- spec/support/lua/lock.lua | 14 ++ spec/support/lua/shared/_common.lua | 13 ++ spec/support/lua/shared/_current_time.lua | 8 ++ spec/support/lua/test.lua | 38 +++++ spec/support/shared_contexts.rb | 10 ++ 28 files changed, 869 insertions(+), 29 deletions(-) create mode 100644 spec/sidekiq_unique_jobs/redis_script/client/execute_spec.rb create mode 100644 spec/sidekiq_unique_jobs/redis_script/client_spec.rb create mode 100644 spec/sidekiq_unique_jobs/redis_script/config_spec.rb create mode 100644 spec/sidekiq_unique_jobs/redis_script/logging_spec.rb create mode 100644 spec/sidekiq_unique_jobs/redis_script/lua_error_spec.rb create mode 100644 spec/sidekiq_unique_jobs/redis_script/scripts_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/client/execute_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/client_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/config_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/logging_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/lua_error_spec.rb create mode 100644 spec/sidekiq_unique_jobs/script/scripts_spec.rb create mode 100644 spec/support/lua/lock.lua create mode 100644 spec/support/lua/shared/_common.lua create mode 100644 spec/support/lua/shared/_current_time.lua create mode 100644 spec/support/lua/test.lua create mode 100644 spec/support/shared_contexts.rb diff --git a/.rubocop.yml b/.rubocop.yml index 45cad9213..93ef4249b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -12,12 +12,20 @@ AllCops: TargetRubyVersion: 2.7 Include: - "examples/**/*.rb" + - "*.rb" Exclude: - - "**/*.erb" - - "**/*.lua" - - "**/vendor/**/*" + - "*.erb" + - "*.lua" + - "assets/**/*" + - "bin/**/*" + - "doc/**/*" + - "docs/**/*" + - "lib/tasks/**/*" + - "lib/sidekiq_unique_jobs/lua/**/*" + - "lib/sidekiq_unique_jobs/web/views/**/*" - "myapp/**/*" - - "bin/bench" + - "Sidekiq/**/*" + - "vendor/**/*" Layout/EndAlignment: EnforcedStyleAlignWith: variable diff --git a/CHANGELOG.md b/CHANGELOG.md index 859365ed3..7061b660e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -938,7 +938,7 @@ **Fixed bugs:** - V7 - `on_conflict:` no longer accepts a Hash [\#495](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/495) -- Brpoplpush::RedisScript::LuaError: WRONGTYPE Operation against a key holding the wrong kind of value [\#491](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/491) +- SidekiqUniqueJobs::Script::LuaError: WRONGTYPE Operation against a key holding the wrong kind of value [\#491](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/491) - Lua script bug [\#489](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/489) - Reaper will delete locks for running jobs [\#488](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/488) - Fix access to hash members [\#496](https://github.com/mhenrixon/sidekiq-unique-jobs/pull/496) ([mhenrixon](https://github.com/mhenrixon)) @@ -1691,7 +1691,7 @@ - deprecation warnings with redis-namespace 2.0 [\#212](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/212) - Unclear docs / examples for unique\_args [\#211](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/211) - Jobs Console fails to launch [\#208](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/208) -- Util.del Redis::CommandError: ERR syntax error [\#207](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/207) +- Util.del RedisClient::CommandError: ERR syntax error [\#207](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/207) - version 4.0.19 [\#206](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/206) - Job.delete does not remove lock in all circumstances [\#205](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/205) - disappearing jobs - known issue in conjunction with other extensions? [\#202](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/202) @@ -1881,7 +1881,7 @@ - Rails + Sidekiq will go bezerk after sidekiq-unique-jobs testing check. [\#128](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/128) - NoMethodError: undefined method `to\_sym' for true:TrueClass [\#125](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/125) -- Redis::CommandError: ERR unknown command 'eval' [\#124](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/124) +- RedisClient::CommandError: ERR unknown command 'eval' [\#124](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/124) **Merged pull requests:** @@ -1944,7 +1944,7 @@ **Closed issues:** -- 3.0.14 Error: ERR wrong number of arguments for 'set' command \(Redis::CommandError\) [\#104](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/104) +- 3.0.14 Error: ERR wrong number of arguments for 'set' command \(RedisClient::CommandError\) [\#104](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/104) - Testing [\#103](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/103) - Active Job [\#102](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/102) - Why is SidekiqUnique behaviour applied to regular Workers? [\#100](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/100) diff --git a/README.md b/README.md index f99582042..ff3976fcd 100644 --- a/README.md +++ b/README.md @@ -814,7 +814,7 @@ In my benchmarks deleting 1000 orphaned locks with lua performs around 65% faste On the other hand if I increase it to 10 000 orphaned locks per cleanup (`reaper_count: 10_0000`) then redis starts throwing: -> BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (Redis::CommandError) +> BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (RedisClient::CommandError) If you want to disable the reaper set it to `:none`, `nil` or `false`. Actually, any value that isn't `:ruby` or `:lua` will disable the reaping. diff --git a/doc/SidekiqUniqueJobs/Script.html b/doc/SidekiqUniqueJobs/Script.html index 3306ed079..26f48e06e 100644 --- a/doc/SidekiqUniqueJobs/Script.html +++ b/doc/SidekiqUniqueJobs/Script.html @@ -73,7 +73,7 @@
Includes:
-
Brpoplpush::RedisScript::DSL
+
SidekiqUniqueJobs::Script::DSL
@@ -145,4 +145,4 @@

Overview

- \ No newline at end of file + diff --git a/doc/file.CHANGELOG.html b/doc/file.CHANGELOG.html index 67f59b979..d754fbf13 100644 --- a/doc/file.CHANGELOG.html +++ b/doc/file.CHANGELOG.html @@ -694,7 +694,7 @@

#495 -
  • Brpoplpush::RedisScript::LuaError: WRONGTYPE Operation against a key holding the wrong kind of value #491
    • +
  • SidekiqUniqueJobs::Script::LuaError: WRONGTYPE Operation against a key holding the wrong kind of value #491
  • Lua script bug #489
  • Reaper will delete locks for running jobs #488
  • Fix access to hash members #496 (mhenrixon)
    • @@ -1641,7 +1641,7 @@

    #212
  • Unclear docs / examples for unique_args #211
  • Jobs Console fails to launch #208
    • -
  • Util.del Redis::CommandError: ERR syntax error #207
    • +
  • Util.del RedisClient::CommandError: ERR syntax error #207
  • version 4.0.19 #206
  • Job.delete does not remove lock in all circumstances #205
  • disappearing jobs - known issue in conjunction with other extensions? #202
    • @@ -1871,7 +1871,7 @@

    #128
  • NoMethodError: undefined method `to_sym' for true:TrueClass #125
    • -
  • Redis::CommandError: ERR unknown command 'eval' #124
    • +
  • RedisClient::CommandError: ERR unknown command 'eval' #124

    • Merged pull requests:

    @@ -1948,7 +1948,7 @@

    #104 +
  • 3.0.14 Error: ERR wrong number of arguments for 'set' command (RedisClient::CommandError) #104
  • Testing #103
  • Active Job #102
  • Why is SidekiqUnique behaviour applied to regular Workers? #100
    • @@ -2178,4 +2178,4 @@

    reaper

    On the other hand if I increase it to 10 000 orphaned locks per cleanup (reaper_count: 10_0000) then redis starts throwing:

    -

    BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (Redis::CommandError)

    +

    BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (RedisClient::CommandError)

    If you want to disable the reaper set it to :none, nil or false. Actually, any value that isn't :ruby or :lua will disable the reaping.

    @@ -1088,4 +1088,4 @@

    Contributors

    - \ No newline at end of file + diff --git a/doc/index.html b/doc/index.html index 8e46fcfe0..66c3bcd95 100644 --- a/doc/index.html +++ b/doc/index.html @@ -861,7 +861,7 @@

    reaper

    On the other hand if I increase it to 10 000 orphaned locks per cleanup (reaper_count: 10_0000) then redis starts throwing:

    -

    BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (Redis::CommandError)

    +

    BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. (RedisClient::CommandError)

    If you want to disable the reaper set it to :none, nil or false. Actually, any value that isn't :ruby or :lua will disable the reaping.

    @@ -1088,4 +1088,4 @@

    Contributors

    - \ No newline at end of file + diff --git a/lib/sidekiq_unique_jobs/script/script.rb b/lib/sidekiq_unique_jobs/script/script.rb index afeee7cf7..f3d1c840b 100644 --- a/lib/sidekiq_unique_jobs/script/script.rb +++ b/lib/sidekiq_unique_jobs/script/script.rb @@ -66,12 +66,7 @@ def compiled_sha end def load(conn) - @sha = - if conn.respond_to?(:namespace) - conn.redis.script(:load, source) - else - conn.script(:load, source) - end + @sha = conn.script(:load, source) self end diff --git a/myapp/config/initializers/sidekiq.rb b/myapp/config/initializers/sidekiq.rb index bfb3dde8e..3f8105ad4 100644 --- a/myapp/config/initializers/sidekiq.rb +++ b/myapp/config/initializers/sidekiq.rb @@ -3,7 +3,7 @@ require "sidekiq" require "sidekiq-unique-jobs" -REDIS = Redis.new(url: ENV.fetch("REDIS_URL", nil)) +REDIS = RedisClient.new(url: ENV.fetch("REDIS_URL", nil)) Sidekiq.default_job_options = { backtrace: true, diff --git a/spec/sidekiq_unique_jobs/redis_script/client/execute_spec.rb b/spec/sidekiq_unique_jobs/redis_script/client/execute_spec.rb new file mode 100644 index 000000000..4bb5e184d --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/client/execute_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Client, "#execute" do + subject(:execute) { client.execute(script_name, redis, **arguments) } + + include_context "with test config" + + let(:client) { described_class.new(config) } + let(:script_name) { :test } + let(:redis) { instance_spy(Sidekiq::RedisConnection) } + let(:arguments) { { keys: keys, argv: argv } } + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:exception) { nil } + let(:redis_error_message) { nil } + let(:script) { SidekiqUniqueJobs::Script::Script.new(name: script_name, root_path: SCRIPTS_PATH) } + let(:scripts) { instance_spy(SidekiqUniqueJobs::Script::Scripts) } + + let(:script_source) do + <<~LUA + local key_one = KEYS[1] + local key_two = KEYS[2] + local key_tre = KEYS[3] + local key_for = KEYS[4] + local key_fiv = KEYS[5] + local arg_one = ARGV[1] + local arg_two = ARGV[2] + local arg_tre = ARGV[3] + local arg_for = ARGV[4] + local arg_fiv = ARGV[5] + LUA + end + + context "when error is raised" do + before do + allow(SidekiqUniqueJobs::Script::Scripts).to receive(:fetch).with(config.scripts_path).and_return(scripts) + allow(scripts).to receive(:delete).and_return(true) + allow(scripts).to receive(:kill).and_return(true) + allow(scripts).to receive(:load).and_return(true) + allow(scripts).to receive(:fetch).and_return(script) + + allow(client.logger).to receive(:warn).and_return(true) + allow(client.logger).to receive(:debug).and_return(true) + + call_count = 0 + allow(scripts).to receive(:execute) do + call_count += 1 + call_count.odd? ? raise(RedisClient::CommandError, redis_error_message) : "bogus" + end + + exception + end + + context "when message starts with ERR" do + let(:redis_error_message) do + <<~ERR + ERR Error running script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): [string "func definition"]:7: attempt to compare nil with number + ERR + end + + let(:error_message) do + <<~ERR_MSG + attempt to compare nil with number + + 5: local key_fiv = KEYS[5] + 6: local arg_one = ARGV[1] + => 7: local arg_two = ARGV[2] + 8: local arg_tre = ARGV[3] + 9: local arg_for = ARGV[4] + + ERR_MSG + end + + let(:exception) do + execute + rescue SidekiqUniqueJobs::Script::LuaError => ex + ex + end + + specify do + expect(exception.message).to eq(error_message) + expect(exception.backtrace.first).to match(%r{spec/support/lua/test.lua:7}) + expect(exception.backtrace[1]).to match(/client.rb/) + + expect(scripts).not_to have_received(:delete) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).once + end + end + + context "when message starts with BUSY" do + let(:redis_error_message) do + "BUSY Redis is busy running a script. " \ + "You can only execute SCRIPT KILL or SHUTDOWN NOSAVE." + end + + context "when .script(:kill) raises CommandError" do + before do + allow(scripts).to receive(:kill).and_raise(RedisClient::CommandError, "NOT BUSY") + allow(client.logger).to receive(:warn) + end + + specify do + expect { execute }.not_to raise_error + expect(client.logger).to have_received(:warn).with(kind_of(RedisClient::CommandError)) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + + context "when .script(:kill) is successful" do + before do + allow(scripts).to receive(:kill).and_return(true) + end + + specify do + expect { execute }.not_to raise_error + + expect(scripts).to have_received(:kill).with(redis) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + end + + context "when message starts with NOSCRIPT" do + let(:redis_error_message) { "NOSCRIPT No matching script. Please use EVAL." } + + specify do + expect { execute }.not_to raise_error + + expect(scripts).to have_received(:delete).with(script_name) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + end +end diff --git a/spec/sidekiq_unique_jobs/redis_script/client_spec.rb b/spec/sidekiq_unique_jobs/redis_script/client_spec.rb new file mode 100644 index 000000000..748ba8044 --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/client_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Client do + let(:client) { described_class.new(config) } + + include_context "with test config" + + describe ".execute" do + subject(:execute) { client.execute(script_name, redis, **arguments) } + + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:redis) { Sidekiq.redis { |conn| return conn } } + let(:scriptsha) { "abcdefab" } + let(:arguments) { { keys: keys, argv: argv } } + let(:script_name) { :test } + + before do + allow(client.logger).to receive(:debug).and_return(true) + end + + it { is_expected.to eq("arg_for") } + end +end diff --git a/spec/sidekiq_unique_jobs/redis_script/config_spec.rb b/spec/sidekiq_unique_jobs/redis_script/config_spec.rb new file mode 100644 index 000000000..5bbdf6c6a --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/config_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Config do + let(:config) { described_class.new } + + let(:script_name) { :test } + let(:scripts_path) { SCRIPTS_PATH } + + it { is_expected.to respond_to(:logger).with(0).arguments } + it { is_expected.to respond_to(:logger=).with(1).arguments } + it { is_expected.to respond_to(:scripts_path).with(0).arguments } + it { is_expected.to respond_to(:scripts_path=).with(1).arguments } + + describe "scripts_path=" do + subject(:set_scripts_path) { config.scripts_path = new_path } + + context "when given a Pathname" do + let(:new_path) { SCRIPTS_PATH } + + it { expect { set_scripts_path }.to change { config.scripts_path }.from(nil).to(new_path) } + end + + context "when given a String" do + let(:new_path) { SCRIPTS_PATH.to_s } + + it { expect { set_scripts_path }.to change { config.scripts_path }.from(nil).to(Pathname.new(new_path)) } + end + + context "when given a Class" do + let(:new_path) { Object.new } + + it { expect { set_scripts_path }.to raise_error(ArgumentError, "#{new_path} should be a Pathname or String") } + end + + context "when directory does not exist" do + let(:new_path) { SCRIPTS_PATH.join("non-existing", "path") } + + it do + expect { set_scripts_path }.to raise_error(ArgumentError, "#{new_path} does not exist") + end + end + end + + describe "logger=" do + subject(:set_logger) { config.logger = logger } + + context "when logger is a Logger" do + let(:logger) { Logger.new($stdout) } + + it { expect { set_logger }.to change { config.logger }.to(logger) } + end + + context "when logger isn't a Logger" do + let(:logger) { Object.new } + + it { expect { set_logger }.to raise_error(ArgumentError, "#{logger} should be a Logger") } + end + end +end diff --git a/spec/sidekiq_unique_jobs/redis_script/logging_spec.rb b/spec/sidekiq_unique_jobs/redis_script/logging_spec.rb new file mode 100644 index 000000000..2a1e2ecbe --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/logging_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Logging do + before do + stub_const("LoggingClass", Class.new do + include SidekiqUniqueJobs::Script::Logging + end) + end + + describe "#methods" do + subject(:logging_impl) { LoggingClass.new } + + it { is_expected.to respond_to(:logger) } + it { is_expected.to respond_to(:log_debug).with(1).arguments } + it { is_expected.to respond_to(:log_info).with(1).arguments } + it { is_expected.to respond_to(:log_warn).with(1).arguments } + it { is_expected.to respond_to(:log_error).with(1).arguments } + it { is_expected.to respond_to(:log_fatal).with(1).arguments } + end + + describe ".methods" do + subject(:logging_impl) { LoggingClass } + + it { is_expected.to respond_to(:logger) } + it { is_expected.to respond_to(:log_debug).with(1).arguments } + it { is_expected.to respond_to(:log_info).with(1).arguments } + it { is_expected.to respond_to(:log_warn).with(1).arguments } + it { is_expected.to respond_to(:log_error).with(1).arguments } + it { is_expected.to respond_to(:log_fatal).with(1).arguments } + end + + describe "#logger" do + subject(:logger) { LoggingClass.new.logger } + + it { is_expected.to eq(SidekiqUniqueJobs::Script.logger) } + end + + describe ".logger" do + subject(:logger) { LoggingClass.logger } + + it { is_expected.to eq(SidekiqUniqueJobs::Script.logger) } + end + + shared_examples "delegates to logger" do |level:| + subject(:logging) { LoggingClass.new } + + let(:logger) { logging.logger } + let(:message) { "I am a message" } + + before do + allow(logger).to receive(level).and_return("ROCK n ROLL") + end + + it "delegates to logger" do + expect(logging.send("log_#{level}".to_sym, message)).to be_nil + expect(logger).to have_received(level).with(message) + end + end + + it_behaves_like "delegates to logger", level: :debug + it_behaves_like "delegates to logger", level: :info + it_behaves_like "delegates to logger", level: :warn + it_behaves_like "delegates to logger", level: :error + it_behaves_like "delegates to logger", level: :fatal +end diff --git a/spec/sidekiq_unique_jobs/redis_script/lua_error_spec.rb b/spec/sidekiq_unique_jobs/redis_script/lua_error_spec.rb new file mode 100644 index 000000000..a86878897 --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/lua_error_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::LuaError do + let(:client) { described_class.new(config) } + + describe ".intercepts?" do + subject(:intercepts?) { described_class.intercepts?(error) } + + context "when error contains ERR Error running script" do + let(:error) { RedisClient::CommandError.new(message) } + let(:message) do + 'ERR Error running script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): ' \ + '[string "func definition"]:7: attempt to compare nil with number' + end + + it { is_expected.to be(true) } + end + + context "when error contains ERR Error compiling script" do + let(:error) { RedisClient::CommandError.new(message) } + let(:message) do + 'ERR Error compiling script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): ' \ + '[string "func definition"]:7: attempt to compare nil with number' + end + + it { is_expected.to be(true) } + end + end +end diff --git a/spec/sidekiq_unique_jobs/redis_script/scripts_spec.rb b/spec/sidekiq_unique_jobs/redis_script/scripts_spec.rb new file mode 100644 index 000000000..f9c679eab --- /dev/null +++ b/spec/sidekiq_unique_jobs/redis_script/scripts_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Scripts do + let(:scripts) { described_class.new(scripts_path) } + + let(:redis) { Sidekiq.redis { |conn| return conn } } + let(:script_name) { :test } + let(:script_path) { SCRIPTS_PATH.join("#{script_name}.lua").to_s } + let(:script_sha) { "63af434656e22ce2cd7f2d3b2e937d3e249cb309" } + let(:scripts_path) { SCRIPTS_PATH } + + describe "#fetch" do + subject(:fetch) { scripts.fetch(script_name, redis) } + + context "when script was loaded" do + before do + scripts.load(script_name, redis) + end + + it "loads script to redis" do + expect { fetch }.not_to change { scripts.count }.from(1) + end + + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + + context "when script wasn't loaded" do + it "loads script to redis" do + expect { fetch }.to change { scripts.count }.by(1) + end + + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + end + + describe "#execute" do + subject(:execute) { scripts.execute(script_name, redis, **arguments) } + + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:arguments) { { keys: keys, argv: argv } } + + it { is_expected.to eq("arg_for") } + end + + describe "#load" do + subject(:load) { scripts.load(script_name, redis) } + + its(:class) { is_expected.to eq(SidekiqUniqueJobs::Script::Script) } + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + + describe "#kill" do + subject(:kill) { scripts.kill(redis) } + + specify { expect { kill }.to raise_error(RedisClient::CommandError, "NOTBUSY No scripts in execution right now.") } + end +end diff --git a/spec/sidekiq_unique_jobs/script/caller_spec.rb b/spec/sidekiq_unique_jobs/script/caller_spec.rb index 1fa45f32d..a91b9c03c 100644 --- a/spec/sidekiq_unique_jobs/script/caller_spec.rb +++ b/spec/sidekiq_unique_jobs/script/caller_spec.rb @@ -14,7 +14,7 @@ let(:keys) { [unique_key] } let(:argv) { [jid, max_lock_time] } let(:script_arguments) { [keys, argv, redis] } - let(:redis) { Redis.new } + let(:redis) { RedisClient.new } let(:scriptsha) { "abcdefab" } let(:script_name) { :acquire_lock } let(:error_message) { "Some interesting error" } diff --git a/spec/sidekiq_unique_jobs/script/client/execute_spec.rb b/spec/sidekiq_unique_jobs/script/client/execute_spec.rb new file mode 100644 index 000000000..6224bc36c --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/client/execute_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Client, "#execute" do + subject(:execute) { client.execute(script_name, redis, **arguments) } + + include_context "with test config" + + let(:client) { described_class.new(config) } + let(:script_name) { :test } + let(:redis) { instance_spy(RedisClient) } + let(:arguments) { { keys: keys, argv: argv } } + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:exception) { nil } + let(:redis_error_message) { nil } + let(:script) { SidekiqUniqueJobs::Script::Script.new(name: script_name, root_path: SCRIPTS_PATH) } + let(:scripts) { instance_spy(SidekiqUniqueJobs::Script::Scripts) } + + let(:script_source) do + <<~LUA + local key_one = KEYS[1] + local key_two = KEYS[2] + local key_tre = KEYS[3] + local key_for = KEYS[4] + local key_fiv = KEYS[5] + local arg_one = ARGV[1] + local arg_two = ARGV[2] + local arg_tre = ARGV[3] + local arg_for = ARGV[4] + local arg_fiv = ARGV[5] + LUA + end + + context "when error is raised" do + before do + allow(SidekiqUniqueJobs::Script::Scripts).to receive(:fetch).with(config.scripts_path).and_return(scripts) + allow(scripts).to receive(:delete).and_return(true) + allow(scripts).to receive(:kill).and_return(true) + allow(scripts).to receive(:load).and_return(true) + allow(scripts).to receive(:fetch).and_return(script) + + allow(client.logger).to receive(:warn).and_return(true) + allow(client.logger).to receive(:debug).and_return(true) + + call_count = 0 + allow(scripts).to receive(:execute) do + call_count += 1 + call_count.odd? ? raise(RedisClient::CommandError, redis_error_message) : "bogus" + end + + exception + end + + context "when message starts with ERR" do + let(:redis_error_message) do + <<~ERR + ERR Error running script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): [string "func definition"]:7: attempt to compare nil with number + ERR + end + + let(:error_message) do + <<~ERR_MSG + attempt to compare nil with number + + 5: local key_fiv = KEYS[5] + 6: local arg_one = ARGV[1] + => 7: local arg_two = ARGV[2] + 8: local arg_tre = ARGV[3] + 9: local arg_for = ARGV[4] + + ERR_MSG + end + + let(:exception) do + execute + rescue SidekiqUniqueJobs::Script::LuaError => ex + ex + end + + specify do + expect(exception.message).to eq(error_message) + expect(exception.backtrace.first).to match(%r{spec/support/lua/test.lua:7}) + expect(exception.backtrace[1]).to match(/client.rb/) + + expect(scripts).not_to have_received(:delete) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).once + end + end + + context "when message starts with BUSY" do + let(:redis_error_message) do + "BUSY Redis is busy running a script. " \ + "You can only execute SCRIPT KILL or SHUTDOWN NOSAVE." + end + + context "when .script(:kill) raises CommandError" do + before do + allow(scripts).to receive(:kill).and_raise(RedisClient::CommandError, "NOT BUSY") + allow(client.logger).to receive(:warn) + end + + specify do + expect { execute }.not_to raise_error + expect(client.logger).to have_received(:warn).with(kind_of(RedisClient::CommandError)) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + + context "when .script(:kill) is successful" do + before do + allow(scripts).to receive(:kill).and_return(true) + end + + specify do + expect { execute }.not_to raise_error + + expect(scripts).to have_received(:kill).with(redis) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + end + + context "when message starts with NOSCRIPT" do + let(:redis_error_message) { "NOSCRIPT No matching script. Please use EVAL." } + + specify do + expect { execute }.not_to raise_error + + expect(scripts).to have_received(:delete).with(script_name) + expect(scripts).to have_received(:execute).with(script_name, redis, keys: keys, argv: argv).twice + end + end + end +end diff --git a/spec/sidekiq_unique_jobs/script/client_spec.rb b/spec/sidekiq_unique_jobs/script/client_spec.rb new file mode 100644 index 000000000..748ba8044 --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/client_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Client do + let(:client) { described_class.new(config) } + + include_context "with test config" + + describe ".execute" do + subject(:execute) { client.execute(script_name, redis, **arguments) } + + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:redis) { Sidekiq.redis { |conn| return conn } } + let(:scriptsha) { "abcdefab" } + let(:arguments) { { keys: keys, argv: argv } } + let(:script_name) { :test } + + before do + allow(client.logger).to receive(:debug).and_return(true) + end + + it { is_expected.to eq("arg_for") } + end +end diff --git a/spec/sidekiq_unique_jobs/script/config_spec.rb b/spec/sidekiq_unique_jobs/script/config_spec.rb new file mode 100644 index 000000000..5bbdf6c6a --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/config_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Config do + let(:config) { described_class.new } + + let(:script_name) { :test } + let(:scripts_path) { SCRIPTS_PATH } + + it { is_expected.to respond_to(:logger).with(0).arguments } + it { is_expected.to respond_to(:logger=).with(1).arguments } + it { is_expected.to respond_to(:scripts_path).with(0).arguments } + it { is_expected.to respond_to(:scripts_path=).with(1).arguments } + + describe "scripts_path=" do + subject(:set_scripts_path) { config.scripts_path = new_path } + + context "when given a Pathname" do + let(:new_path) { SCRIPTS_PATH } + + it { expect { set_scripts_path }.to change { config.scripts_path }.from(nil).to(new_path) } + end + + context "when given a String" do + let(:new_path) { SCRIPTS_PATH.to_s } + + it { expect { set_scripts_path }.to change { config.scripts_path }.from(nil).to(Pathname.new(new_path)) } + end + + context "when given a Class" do + let(:new_path) { Object.new } + + it { expect { set_scripts_path }.to raise_error(ArgumentError, "#{new_path} should be a Pathname or String") } + end + + context "when directory does not exist" do + let(:new_path) { SCRIPTS_PATH.join("non-existing", "path") } + + it do + expect { set_scripts_path }.to raise_error(ArgumentError, "#{new_path} does not exist") + end + end + end + + describe "logger=" do + subject(:set_logger) { config.logger = logger } + + context "when logger is a Logger" do + let(:logger) { Logger.new($stdout) } + + it { expect { set_logger }.to change { config.logger }.to(logger) } + end + + context "when logger isn't a Logger" do + let(:logger) { Object.new } + + it { expect { set_logger }.to raise_error(ArgumentError, "#{logger} should be a Logger") } + end + end +end diff --git a/spec/sidekiq_unique_jobs/script/logging_spec.rb b/spec/sidekiq_unique_jobs/script/logging_spec.rb new file mode 100644 index 000000000..2a1e2ecbe --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/logging_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Logging do + before do + stub_const("LoggingClass", Class.new do + include SidekiqUniqueJobs::Script::Logging + end) + end + + describe "#methods" do + subject(:logging_impl) { LoggingClass.new } + + it { is_expected.to respond_to(:logger) } + it { is_expected.to respond_to(:log_debug).with(1).arguments } + it { is_expected.to respond_to(:log_info).with(1).arguments } + it { is_expected.to respond_to(:log_warn).with(1).arguments } + it { is_expected.to respond_to(:log_error).with(1).arguments } + it { is_expected.to respond_to(:log_fatal).with(1).arguments } + end + + describe ".methods" do + subject(:logging_impl) { LoggingClass } + + it { is_expected.to respond_to(:logger) } + it { is_expected.to respond_to(:log_debug).with(1).arguments } + it { is_expected.to respond_to(:log_info).with(1).arguments } + it { is_expected.to respond_to(:log_warn).with(1).arguments } + it { is_expected.to respond_to(:log_error).with(1).arguments } + it { is_expected.to respond_to(:log_fatal).with(1).arguments } + end + + describe "#logger" do + subject(:logger) { LoggingClass.new.logger } + + it { is_expected.to eq(SidekiqUniqueJobs::Script.logger) } + end + + describe ".logger" do + subject(:logger) { LoggingClass.logger } + + it { is_expected.to eq(SidekiqUniqueJobs::Script.logger) } + end + + shared_examples "delegates to logger" do |level:| + subject(:logging) { LoggingClass.new } + + let(:logger) { logging.logger } + let(:message) { "I am a message" } + + before do + allow(logger).to receive(level).and_return("ROCK n ROLL") + end + + it "delegates to logger" do + expect(logging.send("log_#{level}".to_sym, message)).to be_nil + expect(logger).to have_received(level).with(message) + end + end + + it_behaves_like "delegates to logger", level: :debug + it_behaves_like "delegates to logger", level: :info + it_behaves_like "delegates to logger", level: :warn + it_behaves_like "delegates to logger", level: :error + it_behaves_like "delegates to logger", level: :fatal +end diff --git a/spec/sidekiq_unique_jobs/script/lua_error_spec.rb b/spec/sidekiq_unique_jobs/script/lua_error_spec.rb new file mode 100644 index 000000000..a86878897 --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/lua_error_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::LuaError do + let(:client) { described_class.new(config) } + + describe ".intercepts?" do + subject(:intercepts?) { described_class.intercepts?(error) } + + context "when error contains ERR Error running script" do + let(:error) { RedisClient::CommandError.new(message) } + let(:message) do + 'ERR Error running script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): ' \ + '[string "func definition"]:7: attempt to compare nil with number' + end + + it { is_expected.to be(true) } + end + + context "when error contains ERR Error compiling script" do + let(:error) { RedisClient::CommandError.new(message) } + let(:message) do + 'ERR Error compiling script (execute to f_178d75adaa46af3d8237cfd067c9fdff7b9d504f): ' \ + '[string "func definition"]:7: attempt to compare nil with number' + end + + it { is_expected.to be(true) } + end + end +end diff --git a/spec/sidekiq_unique_jobs/script/scripts_spec.rb b/spec/sidekiq_unique_jobs/script/scripts_spec.rb new file mode 100644 index 000000000..f9c679eab --- /dev/null +++ b/spec/sidekiq_unique_jobs/script/scripts_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +RSpec.describe SidekiqUniqueJobs::Script::Scripts do + let(:scripts) { described_class.new(scripts_path) } + + let(:redis) { Sidekiq.redis { |conn| return conn } } + let(:script_name) { :test } + let(:script_path) { SCRIPTS_PATH.join("#{script_name}.lua").to_s } + let(:script_sha) { "63af434656e22ce2cd7f2d3b2e937d3e249cb309" } + let(:scripts_path) { SCRIPTS_PATH } + + describe "#fetch" do + subject(:fetch) { scripts.fetch(script_name, redis) } + + context "when script was loaded" do + before do + scripts.load(script_name, redis) + end + + it "loads script to redis" do + expect { fetch }.not_to change { scripts.count }.from(1) + end + + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + + context "when script wasn't loaded" do + it "loads script to redis" do + expect { fetch }.to change { scripts.count }.by(1) + end + + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + end + + describe "#execute" do + subject(:execute) { scripts.execute(script_name, redis, **arguments) } + + let(:keys) { %w[key_one key_two key_tre key_for key_fav] } + let(:argv) { %w[arg_one arg_two arg_tre arg_for arg_fav] } + let(:arguments) { { keys: keys, argv: argv } } + + it { is_expected.to eq("arg_for") } + end + + describe "#load" do + subject(:load) { scripts.load(script_name, redis) } + + its(:class) { is_expected.to eq(SidekiqUniqueJobs::Script::Script) } + its(:name) { is_expected.to eq(script_name) } + its(:path) { is_expected.to eq(script_path) } + its(:sha) { is_expected.to eq(script_sha) } + end + + describe "#kill" do + subject(:kill) { scripts.kill(redis) } + + specify { expect { kill }.to raise_error(RedisClient::CommandError, "NOTBUSY No scripts in execution right now.") } + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index bf8128d83..80fe1f7fe 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,7 +23,11 @@ require "sidekiq_unique_jobs/testing" Sidekiq.log_format = :json if Sidekiq.respond_to?(:log_format) -LOGLEVEL = ENV.fetch("LOGLEVEL", "ERROR").upcase +LOGLEVEL = ENV.fetch("LOGLEVEL", "ERROR").upcase +SUPPORT_DIR = Pathname.new(File.join(File.dirname(__FILE__), "support")) +SCRIPTS_PATH = SUPPORT_DIR.join("lua") + +Dir[SUPPORT_DIR.join("**", "*.rb")].sort.each { |f| require f } if Sidekiq.respond_to?(:default_job_options) ORIGINAL_SIDEKIQ_OPTIONS = Sidekiq.default_job_options diff --git a/spec/support/lua/lock.lua b/spec/support/lua/lock.lua new file mode 100644 index 000000000..c565ad59d --- /dev/null +++ b/spec/support/lua/lock.lua @@ -0,0 +1,14 @@ +local key_one = KEYS[1] +local key_two = KEYS[2] + +local locked_val = ARGV[1] + +redis.log(redis.LOG_DEBUG, key_one .. " - " .. key_two .. " - " .. locked_val) + +if key_one == key_two then + return -1 +end + +if redis.call("SET", key_two, locked_val, "NX") then + return 1 +end diff --git a/spec/support/lua/shared/_common.lua b/spec/support/lua/shared/_common.lua new file mode 100644 index 000000000..be7ed8e83 --- /dev/null +++ b/spec/support/lua/shared/_common.lua @@ -0,0 +1,13 @@ +local function toboolean(val) + val = tostring(val) + return val == "1" or val == "true" +end + +local function log_debug( ... ) + local result = "" + for _,v in ipairs(arg) do + result = result .. " " .. tostring(v) + end + redis.log(redis.LOG_DEBUG, " - " .. result) +end + diff --git a/spec/support/lua/shared/_current_time.lua b/spec/support/lua/shared/_current_time.lua new file mode 100644 index 000000000..249786d56 --- /dev/null +++ b/spec/support/lua/shared/_current_time.lua @@ -0,0 +1,8 @@ +local function current_time() + local time = redis.call("time") + local s = time[1] + local ms = time[2] + local number = tonumber((s .. "." .. ms)) + + return number +end diff --git a/spec/support/lua/test.lua b/spec/support/lua/test.lua new file mode 100644 index 000000000..a2375130d --- /dev/null +++ b/spec/support/lua/test.lua @@ -0,0 +1,38 @@ +local key_one = KEYS[1] +local key_two = KEYS[2] +local key_tre = KEYS[3] +local key_for = KEYS[4] +local key_fiv = KEYS[5] +local arg_one = ARGV[1] +local arg_two = ARGV[2] +local arg_tre = ARGV[3] +local arg_for = ARGV[4] +local arg_fiv = ARGV[5] + + +-------- BEGIN local functions -------- +<%= include_partial "shared/_common.lua" %> +---------- END local functions ---------- + +--------- BEGIN test.lua --------- +log_debug("BEGIN test key:", key_one, "arg_one:", arg_one) + +log_debug("SET", key_one, arg_one, "ex", 10) +redis.call("SET", key_one, arg_one, "ex", 10) + +log_debug("SET", key_two, arg_two, "ex", 10) +redis.call("SET", key_two, arg_two, "ex", 10) + +log_debug("SET", key_tre, arg_tre, "ex", 10) +redis.call("SET", key_tre, arg_tre, "ex", 10) + +log_debug("SET", key_for, arg_for, "ex", 10) +redis.call("SET", key_for, arg_for, "ex", 10) + +log_debug("SET", key_fiv, arg_fiv, "ex", 10) +redis.call("SET", key_fiv, arg_fiv, "ex", 10) + +log_debug("END test key:", key_one, "arg_one:", arg_one) + +return arg_for +---------- END test.lua ---------- diff --git a/spec/support/shared_contexts.rb b/spec/support/shared_contexts.rb new file mode 100644 index 000000000..e94f20fb0 --- /dev/null +++ b/spec/support/shared_contexts.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +RSpec.shared_context "with test config" do + let(:config) { SidekiqUniqueJobs::Script::Config.new } + let(:scripts_path) { SCRIPTS_PATH } + + before do + config.scripts_path = scripts_path + end +end