From 59a8d54df2a29a2698bb13fa6b7d276b2fb7031f Mon Sep 17 00:00:00 2001 From: Patricio de Villa Date: Thu, 21 Mar 2024 19:32:27 -0600 Subject: [PATCH] cleaner api, ensure jobs run after failing --- .rubocop.yml | 2 +- .rubocop_todo.yml | 19 +++-- Gemfile | 1 + README.md | 4 +- lib/sidekiq_bouncer/bounceable.rb | 19 +++-- lib/sidekiq_bouncer/bouncer.rb | 16 +++- lib/sidekiq_bouncer/version.rb | 2 +- spec/sidekiq_bouncer/bouncer_spec.rb | 114 +++++++++++++++++---------- spec/spec_helper.rb | 2 + 9 files changed, 112 insertions(+), 67 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index b57774b..ee31ef1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,7 +18,7 @@ require: AllCops: NewCops: enable - # TargetRubyVersion: 2.7.8 + TargetRubyVersion: 3.2.2 # TargetRailsVersion: 6.1.4 # Exclude: # - 'Gemfile.lock' diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index d2c6862..b838b3d 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,24 +1,23 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2024-03-19 18:55:09 UTC using RuboCop version 1.62.1. +# on 2024-03-22 01:08:54 UTC using RuboCop version 1.62.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. +# Offense count: 1 +# Configuration parameters: Severity, Include. +# Include: **/*.gemspec +Gemspec/RequiredRubyVersion: + Exclude: + - 'sidekiq-bouncer.gemspec' + # Offense count: 1 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: Max: 14 -# Offense count: 1 -RSpec/BeforeAfterAll: - Exclude: - - '**/spec/spec_helper.rb' - - '**/spec/rails_helper.rb' - - '**/spec/support/**/*.rb' - - 'spec/sidekiq_bouncer/bouncer_spec.rb' - # Offense count: 2 # Configuration parameters: CountAsOne. RSpec/ExampleLength: @@ -29,7 +28,7 @@ RSpec/ExampleLength: RSpec/NestedGroups: Max: 4 -# Offense count: 1 +# Offense count: 2 RSpec/SubjectStub: Exclude: - 'spec/sidekiq_bouncer/bouncer_spec.rb' diff --git a/Gemfile b/Gemfile index 30ca58b..86f0fb9 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ gemspec gem 'debug', '>= 1.0.0' gem 'rake', '~> 13.1' gem 'rspec', '~> 3.9' +gem 'timecop' # rubocop gem 'rubocop', '~> 1.62' diff --git a/README.md b/README.md index 6496e11..bef8cbe 100644 --- a/README.md +++ b/README.md @@ -61,9 +61,7 @@ class FooWorker # The default delay is 60 seconds. You can optionally override it. register_bouncer(delay: optional_delay_override, delay_buffer: optional_delay_buffer_override) - def perform(param1, param2, debounce_key = nil) - return unless self.class.bouncer.let_in?(debounce_key) # last argument is added as debounce_key - + def perform(param1, param2) # Do your thing. end end diff --git a/lib/sidekiq_bouncer/bounceable.rb b/lib/sidekiq_bouncer/bounceable.rb index a4c94a0..31c3d1e 100644 --- a/lib/sidekiq_bouncer/bounceable.rb +++ b/lib/sidekiq_bouncer/bounceable.rb @@ -4,23 +4,26 @@ module SidekiqBouncer module Bounceable def self.included(base) - base.include InstanceMethods + base.prepend InstanceMethods base.extend ClassMethods end module ClassMethods - # creates and sets a +SidekiqBouncer::Bouncer+ - def register_bouncer(**kwargs) - @bouncer = SidekiqBouncer::Bouncer.new(self, **kwargs) - end - # @retrun [SidekiqBouncer::Bouncer] - def bouncer - @bouncer + attr_reader :bouncer + + # creates and sets a +SidekiqBouncer::Bouncer+ + def register_bouncer(**) + @bouncer = SidekiqBouncer::Bouncer.new(self, **) end end module InstanceMethods + def perform(*, debounce_key, **) + self.class.bouncer.run(debounce_key) do + super(*, **) + end + end end end diff --git a/lib/sidekiq_bouncer/bouncer.rb b/lib/sidekiq_bouncer/bouncer.rb index e5f8e50..c99bde3 100644 --- a/lib/sidekiq_bouncer/bouncer.rb +++ b/lib/sidekiq_bouncer/bouncer.rb @@ -54,6 +54,16 @@ def debounce(*params, key_or_args_indices:) # Checks if job should be excecuted # + # @param [NilClass|String] key + # @return [False|*] true if should be excecuted + def run(key) + return false unless let_in?(key) + + yield.tap do + redis.call('DEL', key) + end + end + # @param [NilClass|String] key # @return [Boolean] true if should be excecuted def let_in?(key) @@ -67,18 +77,16 @@ def let_in?(key) timestamp = redis.call('GET', key) return false if timestamp.nil? || now_i < timestamp.to_i - redis.call('DEL', key) - true end + private + # @return [RedisClient::Pooled] def redis SidekiqBouncer.config.redis end - private - # Builds a key based on arguments # # @param [Array] redis_params diff --git a/lib/sidekiq_bouncer/version.rb b/lib/sidekiq_bouncer/version.rb index 1b2ae35..a915cc3 100644 --- a/lib/sidekiq_bouncer/version.rb +++ b/lib/sidekiq_bouncer/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module SidekiqBouncer - VERSION = '0.3.2' + VERSION = '0.3.3' end diff --git a/spec/sidekiq_bouncer/bouncer_spec.rb b/spec/sidekiq_bouncer/bouncer_spec.rb index 6734489..3a447d9 100644 --- a/spec/sidekiq_bouncer/bouncer_spec.rb +++ b/spec/sidekiq_bouncer/bouncer_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Mock -module RedisMock +class RedisMock # rubocop:disable Lint/EmptyClass end # Mock @@ -9,8 +9,6 @@ class WorkerMock def self.bouncer SidekiqBouncer::Bouncer.new(self) end - - # def self.perform_at(*args); end end # Tests @@ -18,19 +16,18 @@ def self.bouncer # careful, the bouncer instance is generally cached on the worker model subject(:bouncer) { WorkerMock.bouncer } - before :all do + let(:redis) { SidekiqBouncer.config.redis } + let(:worker_klass) { WorkerMock } + let(:now) { Time.now.to_i } + + before do SidekiqBouncer.configure do |config| - config.redis = RedisMock + config.redis = RedisMock.new end - end - let(:redis) { RedisMock } - let(:worker_klass) { WorkerMock } - let(:now) { 100 } + Timecop.freeze(Time.now) - before do # stubbing - allow(bouncer).to receive(:now_i) { now } allow(redis).to receive(:call) allow(worker_klass).to receive(:perform_at) end @@ -43,7 +40,6 @@ def self.bouncer it { expect(bouncer).to respond_to(:delay_buffer=) } it { expect(bouncer).to respond_to(:debounce) } it { expect(bouncer).to respond_to(:let_in?) } - it { expect(bouncer).to respond_to(:redis) } end describe '.new' do @@ -82,7 +78,7 @@ def self.bouncer it 'sets scoped_key to Redis with delayed timestamp' do bouncer.debounce('test_param_1', 'test_param_2', key_or_args_indices: [0, 1]) - expect(SidekiqBouncer.config.redis) + expect(redis) .to have_received(:call) .with('SET', 'WorkerMock:test_param_1,test_param_2', now + bouncer.delay) end @@ -101,7 +97,7 @@ def self.bouncer it 'sets scoped_key to Redis with delayed timestamp' do bouncer.debounce('test_param_1', 'test_param_2', key_or_args_indices: [0]) - expect(SidekiqBouncer.config.redis) + expect(redis) .to have_received(:call) .with('SET', 'WorkerMock:test_param_1', now + bouncer.delay) end @@ -121,7 +117,7 @@ def self.bouncer describe '#let_in?' do context 'when key is nil' do it 'does not call redis' do - expect(SidekiqBouncer.config.redis).not_to have_received(:call) + expect(redis).not_to have_received(:call) end it 'returns true' do @@ -135,7 +131,7 @@ def self.bouncer it 'exec call on redis with GET' do bouncer.let_in?(key) - expect(SidekiqBouncer.config.redis) + expect(redis) .to have_received(:call) .with('GET', key) end @@ -145,14 +141,6 @@ def self.bouncer allow(redis).to receive(:call).with('GET', anything).and_return(now - 10) end - it 'exec call on redis with DEL' do - bouncer.let_in?(key) - - expect(SidekiqBouncer.config.redis) - .to have_received(:call) - .with('DEL', key) - end - it 'returns true' do expect(bouncer.let_in?(key)).to be(true) end @@ -163,14 +151,6 @@ def self.bouncer allow(redis).to receive(:call).with('GET', anything).and_return(Time.now + 10) end - it 'does not exec call on redis with DEL' do - bouncer.let_in?(key) - - expect(SidekiqBouncer.config.redis) - .not_to have_received(:call) - .with('DEL', anything) - end - it 'returns false' do expect(bouncer.let_in?(key)).to be(false) end @@ -181,18 +161,72 @@ def self.bouncer allow(redis).to receive(:call).with('GET', anything).and_return(nil) end - it 'does not exec call on redis with DEL' do - bouncer.let_in?(key) - - expect(SidekiqBouncer.config.redis) - .not_to have_received(:call) - .with('DEL', anything) - end - it 'returns false' do expect(bouncer.let_in?(key)).to be(false) end end end end + + describe '#run' do + before do + # stubbing + allow(bouncer).to receive(:let_in?).with('do').and_return(true) + allow(bouncer).to receive(:let_in?).with('do_not').and_return(false) + end + + context 'when let_in? returns false' do + it 'returns false' do + expect(bouncer.run('do_not')).to be(false) + end + + it 'does not yield' do + expect { |b| bouncer.run('do_not', &b) }.not_to yield_control + end + + it 'does not exec call on redis with DEL' do + bouncer.run('do_not') { '__test__' } + + expect(redis) + .not_to have_received(:call) + .with('DEL', anything) + end + end + + context 'when let_in? returns true' do + it 'returns yield return' do + expect(bouncer.run('do') { '__test__' }).to be('__test__') + end + + it 'yields' do + expect { |b| bouncer.run('do', &b) }.to yield_control + end + + it 'exec call on redis with DEL' do + bouncer.run('do') { '__test__' } + + expect(redis) + .to have_received(:call) + .with('DEL', 'do') + end + end + end + + describe '#now_i' do + it 'returns now as integer' do + expect(bouncer.send(:now_i)).to be(now) + end + end + + describe '#redis' do + it 'returns' do + expect(bouncer.send(:redis)).to be_a(RedisMock) + end + end + + describe '#redis_key' do + it 'returns now as integer' do + expect(bouncer.send(:redis_key, 'test_key')).to eql('WorkerMock:test_key') + end + end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index cf06d3b..61f5801 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true +$LOAD_PATH.unshift File.expand_path('../lib', __dir__) require 'bundler/setup' require 'sidekiq_bouncer' +require 'timecop' require 'debug' RSpec.configure do |config|