From 1504e0ff2efb4b17bf6bfaf917d593689d335388 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 03:55:39 -0500 Subject: [PATCH 1/7] Define methods normally in `IntegrationBehavior` This changes the `private` helper methods from being defined in the `included` block to being defined as regular methods in the module. `included` should only be used for using class macros. This fixes method redefinition warnings, because due to the `included` approach, we were defining the methods on the host class (instead of this module), which would go on to redefined them. --- test/integration/integration_behaviour.rb | 44 +++++++++++------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/test/integration/integration_behaviour.rb b/test/integration/integration_behaviour.rb index 89dd3602..1fca001d 100644 --- a/test/integration/integration_behaviour.rb +++ b/test/integration/integration_behaviour.rb @@ -50,33 +50,33 @@ module IntegrationBehaviour failed_job_error_class_name, ) end + end - private + private - # Should return the symbol to use when configuring the adapter - # ActiveJob::Base.queue_adapter = adapter - def adapter - raise NotImplemented, "#{self.class.name} must implement #{__method__}" - end + # Should return the symbol to use when configuring the adapter + # ActiveJob::Base.queue_adapter = adapter + def adapter + raise NotImplemented, "#{self.class.name} must implement #{__method__}" + end - # Should start the job worker process and allow it to work the queue - def start_worker_and_wait - raise NotImplemented, "#{self.class.name} must implement #{__method__}" - end + # Should start the job worker process and allow it to work the queue + def start_worker_and_wait + raise NotImplemented, "#{self.class.name} must implement #{__method__}" + end - # Should return the number of jobs currently enqueued for processing - def queue_size - raise NotImplemented, "#{self.class.name} must implement #{__method__}" - end + # Should return the number of jobs currently enqueued for processing + def queue_size + raise NotImplemented, "#{self.class.name} must implement #{__method__}" + end - # Should return the hash of job arguments belonging to the most recently enqueued job - def job_args - raise NotImplemented, "#{self.class.name} must implement #{__method__}" - end + # Should return the hash of job arguments belonging to the most recently enqueued job + def job_args + raise NotImplemented, "#{self.class.name} must implement #{__method__}" + end - # Should return a String matching the name of the error class of the most recently failed job - def failed_job_error_class_name - raise NotImplemented, "#{self.class.name} must implement #{__method__}" - end + # Should return a String matching the name of the error class of the most recently failed job + def failed_job_error_class_name + raise NotImplemented, "#{self.class.name} must implement #{__method__}" end end From d600f142860ab4dc7e577b9bb7d54fde428a5b67 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 03:58:36 -0500 Subject: [PATCH 2/7] Redirect `spawn` output to `/dev/null` Otherwise, we get all the noisy logs from Resque & Sidekiq running, including when we intentionally trigger errors. --- test/integration/resque_test.rb | 8 +++++++- test/integration/sidekiq_test.rb | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/test/integration/resque_test.rb b/test/integration/resque_test.rb index 5881d489..3545fa09 100644 --- a/test/integration/resque_test.rb +++ b/test/integration/resque_test.rb @@ -17,7 +17,13 @@ def queue_adapter def start_worker_and_wait pid = nil Dir.chdir("test/support/resque") do - pid = spawn(resque_env, "bundle exec rake resque:work") + pid = spawn( + resque_env, + "bundle exec rake resque:work", + in: "/dev/null", + out: "/dev/null", + err: "/dev/null", + ) end ensure Process.wait(pid) if pid diff --git a/test/integration/sidekiq_test.rb b/test/integration/sidekiq_test.rb index ca0f0d18..c854b864 100644 --- a/test/integration/sidekiq_test.rb +++ b/test/integration/sidekiq_test.rb @@ -16,7 +16,12 @@ def queue_adapter end def start_worker_and_wait - pid = spawn("bundle exec sidekiq -r ./test/support/sidekiq/init.rb -c 1") + pid = spawn( + "bundle exec sidekiq -r ./test/support/sidekiq/init.rb -c 1", + in: "/dev/null", + out: "/dev/null", + err: "/dev/null", + ) ensure Process.wait(pid) end From 10b6cc8164e956582356d2484af0b4a2e2cb2103 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 03:59:50 -0500 Subject: [PATCH 3/7] Reduce `sleep` duration There doesn't appear to be a reason to sleep for an entire second, so we can reduce it to speed up tests. --- test/support/jobs.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/support/jobs.rb b/test/support/jobs.rb index d56a808e..e38f96f9 100644 --- a/test/support/jobs.rb +++ b/test/support/jobs.rb @@ -11,7 +11,7 @@ def each_iteration(omg) if omg == 0 || omg == 2 Process.kill("TERM", Process.pid) end - sleep(1) + sleep(0.01) end end From 2080c73e4f42b51ed1df49a97ccd03bf942a7289 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 04:05:02 -0500 Subject: [PATCH 4/7] Silence Sidekiq logging --- test/support/sidekiq/init.rb | 1 + test/test_helper.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/test/support/sidekiq/init.rb b/test/support/sidekiq/init.rb index 4f930536..fa2d4d35 100644 --- a/test/support/sidekiq/init.rb +++ b/test/support/sidekiq/init.rb @@ -11,6 +11,7 @@ redis_url = ENV.fetch("REDIS_URL") { "redis://localhost:6379/0" } Sidekiq.configure_server do |config| + config.logger = nil config.redis = { url: redis_url } end diff --git a/test/test_helper.rb b/test/test_helper.rb index 01ef131b..dbc78cf7 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -81,6 +81,7 @@ class Order < ActiveRecord::Base Resque.redis = Redis.current Sidekiq.configure_client do |config| + config.logger = nil config.redis = { url: redis_url } end From 1d3d6d743dcc62fdbc35f866b10eee0e5c7f0445 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 04:06:15 -0500 Subject: [PATCH 5/7] Use `ActiveJob::QueueAdapters::TestAdapter` instead The `TestAdapter` included with `ActiveJob` is sufficient for our purposes, so we don't need to create our own. Using this will simplify upcoming code which dynamically sets the interruption adapter based on the queue adapter name, by not having to handle this custom queue adapter. The `TestAdapter` in Active Job 5.2 doesn't serialize the job properly, but that was fixed in Active Job 6.0, so until we drop support for 5.2, we include a backport monkey patch for tests to work properly. --- ...rs_test_adapter_compatibility_extension.rb | 35 +++++++++++++++++++ test/test_helper.rb | 24 ++----------- test/unit/active_job_iteration_test.rb | 2 +- test/unit/throttle_enumerator_test.rb | 2 +- 4 files changed, 40 insertions(+), 23 deletions(-) create mode 100644 test/support/active_job_5_2_queue_adapters_test_adapter_compatibility_extension.rb diff --git a/test/support/active_job_5_2_queue_adapters_test_adapter_compatibility_extension.rb b/test/support/active_job_5_2_queue_adapters_test_adapter_compatibility_extension.rb new file mode 100644 index 00000000..3d463794 --- /dev/null +++ b/test/support/active_job_5_2_queue_adapters_test_adapter_compatibility_extension.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +active_job_5_2_still_supported = Gem + .loaded_specs["job-iteration"] + .dependencies.find { |gem| gem.name == "activejob" } + .requirement.satisfied_by?(Gem::Version.new("5.2")) + +raise <<~MSG unless active_job_5_2_still_supported + Now that support for Active Job 5.2 has been dropped, this patch is no longer required. + You should: + - Delete this file (`#{Pathname.new(__FILE__).relative_path_from(File.join(__dir__, "../.."))}`) + - Remove the corresponding `require_relative` from `test/test_helper.rb` +MSG + +# Nothing to do if we're using Active Job 6.0 or later +return if Gem.loaded_specs.fetch("activejob").version >= Gem::Version.new("6.0") + +module JobIteration + # This module backports the 6.0 implementation of ActiveJob::QueueAdapters::TestAdapter#job_to_hash, + # which includes the serialized job, plus the fields included in 5.2's version. + # Without this, ActiveJob's deserialization fails when using the TestAdapter, and our tests erroneously fail. + module ActiveJob52QueueAdaptersTestAdapterCompatibilityExtension + private + + def job_to_hash(job, extras = {}) + job.serialize.tap do |job_data| + job_data[:job] = job.class + job_data[:args] = job_data.fetch("arguments") + job_data[:queue] = job_data.fetch("queue_name") + end.merge(extras) + end + end +end + +ActiveJob::QueueAdapters::TestAdapter.prepend(JobIteration::ActiveJob52QueueAdaptersTestAdapterCompatibilityExtension) diff --git a/test/test_helper.rb b/test/test_helper.rb index dbc78cf7..a25a6475 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -16,30 +16,12 @@ require "pry" require "mocha/minitest" +require_relative "support/active_job_5_2_queue_adapters_test_adapter_compatibility_extension" + GlobalID.app = "iteration" ActiveRecord::Base.include(GlobalID::Identification) # https://github.com/rails/globalid/blob/main/lib/global_id/railtie.rb -module ActiveJob - module QueueAdapters - class IterationTestAdapter - attr_writer(:enqueued_jobs) - - def enqueued_jobs - @enqueued_jobs ||= [] - end - - def enqueue(job) - enqueued_jobs << job.serialize - end - - def enqueue_at(job, timestamp) - enqueued_jobs << job.serialize.merge("retry_at" => timestamp) - end - end - end -end - -ActiveJob::Base.queue_adapter = :iteration_test +ActiveJob::Base.queue_adapter = :test class Product < ActiveRecord::Base has_many :comments diff --git a/test/unit/active_job_iteration_test.rb b/test/unit/active_job_iteration_test.rb index 07028312..cb9f7917 100644 --- a/test/unit/active_job_iteration_test.rb +++ b/test/unit/active_job_iteration_test.rb @@ -760,7 +760,7 @@ def test_interrupted_job_uses_default_retry_backoff ActiveRecordIterationJob.perform_now job = ActiveJob::Base.queue_adapter.enqueued_jobs.first - assert_equal(15.seconds.from_now.to_f, job.fetch("retry_at")) + assert_equal(15.seconds.from_now.to_f, job.fetch(:at)) end end end diff --git a/test/unit/throttle_enumerator_test.rb b/test/unit/throttle_enumerator_test.rb index 28eabec8..e47727d0 100644 --- a/test/unit/throttle_enumerator_test.rb +++ b/test/unit/throttle_enumerator_test.rb @@ -117,7 +117,7 @@ def each_iteration(_record, _params); end IterationThrottleJob.perform_now({}) job = ActiveJob::Base.queue_adapter.enqueued_jobs.first - assert_equal(30.seconds.from_now.to_f, job.fetch("retry_at")) + assert_equal(30.seconds.from_now.to_f, job.fetch(:at)) end end end From 6aeeae46729e1c5ba7d50a72451aa66c29d9e96b Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 21 Nov 2023 04:08:55 -0500 Subject: [PATCH 6/7] Suppress `ActiveRecord` logging --- test/test_helper.rb | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index a25a6475..f05bf268 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -67,25 +67,27 @@ class Order < ActiveRecord::Base config.redis = { url: redis_url } end -ActiveRecord::Schema.define do - create_table(:products, force: true) do |t| - t.string(:name) - t.timestamps - end +ActiveRecord::Migration.suppress_messages do + ActiveRecord::Schema.define do + create_table(:products, force: true) do |t| + t.string(:name) + t.timestamps + end - create_table(:comments, force: true) do |t| - t.string(:content) - t.belongs_to(:product) - end + create_table(:comments, force: true) do |t| + t.string(:content) + t.belongs_to(:product) + end - create_table(:travel_routes, force: true, primary_key: [:origin, :destination]) do |t| - t.string(:destination) - t.string(:origin) - end + create_table(:travel_routes, force: true, primary_key: [:origin, :destination]) do |t| + t.string(:destination) + t.string(:origin) + end - create_table(:orders, force: true) do |t| - t.integer(:shop_id) - t.string(:name) + create_table(:orders, force: true) do |t| + t.integer(:shop_id) + t.string(:name) + end end end @@ -107,6 +109,7 @@ def assert_logged(message) end JobIteration.logger = Logger.new(IO::NULL) +ActiveJob::Base.logger = Logger.new(IO::NULL) module ActiveSupport class TestCase From 0d68606de90cecc64f8aa5ba73b5292cd1c9e093 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 1 Feb 2024 17:11:35 -0500 Subject: [PATCH 7/7] Remove `REDIS_{HOST,PORT}` CI environment variables These are unused. --- .github/workflows/ci.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 642f5c92..ec46b389 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,9 +69,6 @@ jobs: mysql -uroot -h localhost -proot -e "CREATE DATABASE job_iteration_test;" - name: Ruby tests run: bundle exec rake test - env: - REDIS_HOST: localhost - REDIS_PORT: ${{ job.services.redis.ports[6379] }} lint: runs-on: ubuntu-latest