From 2af03164cbf0c3e86b551d9be338780c204ef9fa Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Thu, 10 Feb 2022 19:42:45 +0100
Subject: [PATCH] Improve tests involving push_bulk (#17508)

sidekiq-bulk's push_bulk can either accept arguments directly or run them
through a block.

Setting expectations on the result of evaluating the blocks allows testing
more code (the block itself) and the test is moved closer to the *interface*
of the tested code than its precise implementation.
---
 .../activitypub/fetch_replies_service_spec.rb  | 18 ++++++------------
 spec/spec_helper.rb                            |  7 +++++++
 .../distribute_poll_update_worker_spec.rb      |  3 +--
 .../activitypub/distribution_worker_spec.rb    |  7 +++----
 .../move_distribution_worker_spec.rb           |  6 ++++--
 .../status_update_distribution_worker_spec.rb  |  8 ++------
 .../update_distribution_worker_spec.rb         |  3 +--
 spec/workers/move_worker_spec.rb               |  5 ++---
 8 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/spec/services/activitypub/fetch_replies_service_spec.rb b/spec/services/activitypub/fetch_replies_service_spec.rb
index 65c453341..fe49b18c1 100644
--- a/spec/services/activitypub/fetch_replies_service_spec.rb
+++ b/spec/services/activitypub/fetch_replies_service_spec.rb
@@ -34,9 +34,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
     context 'when the payload is a Collection with inlined replies' do
       context 'when passing the collection itself' do
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, payload)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
 
@@ -46,9 +45,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
         end
 
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, collection_uri)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
     end
@@ -65,9 +63,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
 
       context 'when passing the collection itself' do
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, payload)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
 
@@ -77,9 +74,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
         end
 
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, collection_uri)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
     end
@@ -100,9 +96,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
 
       context 'when passing the collection itself' do
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, payload)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
 
@@ -112,9 +107,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
         end
 
         it 'spawns workers for up to 5 replies on the same server' do
-          allow(FetchReplyWorker).to receive(:push_bulk)
+          expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
           subject.call(status, collection_uri)
-          expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
         end
       end
     end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index b6d127a08..0414ba9ed 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -57,3 +57,10 @@ end
 def json_str_to_hash(str)
   JSON.parse(str, symbolize_names: true)
 end
+
+def expect_push_bulk_to_match(klass, matcher)
+  expect(Sidekiq::Client).to receive(:push_bulk).with(hash_including({
+    "class" => klass,
+    "args" => matcher
+  }))
+end
diff --git a/spec/workers/activitypub/distribute_poll_update_worker_spec.rb b/spec/workers/activitypub/distribute_poll_update_worker_spec.rb
index 7eb6119fd..d68a695b7 100644
--- a/spec/workers/activitypub/distribute_poll_update_worker_spec.rb
+++ b/spec/workers/activitypub/distribute_poll_update_worker_spec.rb
@@ -10,13 +10,12 @@ describe ActivityPub::DistributePollUpdateWorker do
 
   describe '#perform' do
     before do
-      allow(ActivityPub::DeliveryWorker).to receive(:push_bulk)
       follower.follow!(account)
     end
 
     it 'delivers to followers' do
+      expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), account.id, 'http://example.com']])
       subject.perform(status.id)
-      expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['http://example.com'])
     end
   end
 end
diff --git a/spec/workers/activitypub/distribution_worker_spec.rb b/spec/workers/activitypub/distribution_worker_spec.rb
index c017b4da1..3a5900d9b 100644
--- a/spec/workers/activitypub/distribution_worker_spec.rb
+++ b/spec/workers/activitypub/distribution_worker_spec.rb
@@ -8,7 +8,6 @@ describe ActivityPub::DistributionWorker do
 
   describe '#perform' do
     before do
-      allow(ActivityPub::DeliveryWorker).to receive(:push_bulk)
       follower.follow!(status.account)
     end
 
@@ -18,8 +17,8 @@ describe ActivityPub::DistributionWorker do
       end
 
       it 'delivers to followers' do
+        expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), status.account.id, 'http://example.com', anything]])
         subject.perform(status.id)
-        expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['http://example.com'])
       end
     end
 
@@ -29,8 +28,8 @@ describe ActivityPub::DistributionWorker do
       end
 
       it 'delivers to followers' do
+        expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), status.account.id, 'http://example.com', anything]])
         subject.perform(status.id)
-        expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['http://example.com'])
       end
     end
 
@@ -43,8 +42,8 @@ describe ActivityPub::DistributionWorker do
       end
 
       it 'delivers to mentioned accounts' do
+        expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), status.account.id, 'https://foo.bar/inbox', anything]])
         subject.perform(status.id)
-        expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['https://foo.bar/inbox'])
       end
     end
   end
diff --git a/spec/workers/activitypub/move_distribution_worker_spec.rb b/spec/workers/activitypub/move_distribution_worker_spec.rb
index b52788e54..af8c44cc0 100644
--- a/spec/workers/activitypub/move_distribution_worker_spec.rb
+++ b/spec/workers/activitypub/move_distribution_worker_spec.rb
@@ -9,14 +9,16 @@ describe ActivityPub::MoveDistributionWorker do
 
   describe '#perform' do
     before do
-      allow(ActivityPub::DeliveryWorker).to receive(:push_bulk)
       follower.follow!(migration.account)
       blocker.block!(migration.account)
     end
 
     it 'delivers to followers and known blockers' do
+      expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [
+        [kind_of(String), migration.account.id, 'http://example.com'],
+        [kind_of(String), migration.account.id, 'http://example2.com']
+      ])
       subject.perform(migration.id)
-        expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['http://example.com', 'http://example2.com'])
     end
   end
 end
diff --git a/spec/workers/activitypub/status_update_distribution_worker_spec.rb b/spec/workers/activitypub/status_update_distribution_worker_spec.rb
index 6633b601f..c014c6790 100644
--- a/spec/workers/activitypub/status_update_distribution_worker_spec.rb
+++ b/spec/workers/activitypub/status_update_distribution_worker_spec.rb
@@ -23,9 +23,7 @@ describe ActivityPub::StatusUpdateDistributionWorker do
       end
 
       it 'delivers to followers' do
-        expect(ActivityPub::DeliveryWorker).to receive(:push_bulk) do |items, &block|
-          expect(items.map(&block)).to match([[kind_of(String), status.account.id, 'http://example.com', anything]])
-        end
+        expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), status.account.id, 'http://example.com', anything]])
 
         subject.perform(status.id)
       end
@@ -37,9 +35,7 @@ describe ActivityPub::StatusUpdateDistributionWorker do
       end
 
       it 'delivers to followers' do
-        expect(ActivityPub::DeliveryWorker).to receive(:push_bulk) do |items, &block|
-          expect(items.map(&block)).to match([[kind_of(String), status.account.id, 'http://example.com', anything]])
-        end
+        expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), status.account.id, 'http://example.com', anything]])
 
         subject.perform(status.id)
       end
diff --git a/spec/workers/activitypub/update_distribution_worker_spec.rb b/spec/workers/activitypub/update_distribution_worker_spec.rb
index 688a424d5..0e057fd0b 100644
--- a/spec/workers/activitypub/update_distribution_worker_spec.rb
+++ b/spec/workers/activitypub/update_distribution_worker_spec.rb
@@ -8,13 +8,12 @@ describe ActivityPub::UpdateDistributionWorker do
 
   describe '#perform' do
     before do
-      allow(ActivityPub::DeliveryWorker).to receive(:push_bulk)
       follower.follow!(account)
     end
 
     it 'delivers to followers' do
+      expect_push_bulk_to_match(ActivityPub::DeliveryWorker, [[kind_of(String), account.id, 'http://example.com', anything]])
       subject.perform(account.id)
-      expect(ActivityPub::DeliveryWorker).to have_received(:push_bulk).with(['http://example.com'])
     end
   end
 end
diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb
index 4db5810f1..be02d3192 100644
--- a/spec/workers/move_worker_spec.rb
+++ b/spec/workers/move_worker_spec.rb
@@ -21,7 +21,6 @@ describe MoveWorker do
     blocking_account.block!(source_account)
     muting_account.mute!(source_account)
 
-    allow(UnfollowFollowWorker).to receive(:push_bulk)
     allow(BlockService).to receive(:new).and_return(block_service)
     allow(block_service).to receive(:call)
   end
@@ -78,8 +77,8 @@ describe MoveWorker do
   context 'both accounts are distant' do
     describe 'perform' do
       it 'calls UnfollowFollowWorker' do
+        expect_push_bulk_to_match(UnfollowFollowWorker, [[local_follower.id, source_account.id, target_account.id, false]])
         subject.perform(source_account.id, target_account.id)
-        expect(UnfollowFollowWorker).to have_received(:push_bulk).with([local_follower.id])
       end
 
       include_examples 'user note handling'
@@ -92,8 +91,8 @@ describe MoveWorker do
 
     describe 'perform' do
       it 'calls UnfollowFollowWorker' do
+        expect_push_bulk_to_match(UnfollowFollowWorker, [[local_follower.id, source_account.id, target_account.id, true]])
         subject.perform(source_account.id, target_account.id)
-        expect(UnfollowFollowWorker).to have_received(:push_bulk).with([local_follower.id])
       end
 
       include_examples 'user note handling'