2 files changed, 42 insertions(+), 41 deletions(-)

M .rubocop_todo.yml
M spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb

@@ 56,7 56,6 @@ Lint/AmbiguousBlockAssociation:
     - 'spec/controllers/settings/two_factor_authentication/otp_authentication_controller_spec.rb'
     - 'spec/services/activitypub/process_status_update_service_spec.rb'
     - 'spec/services/post_status_service_spec.rb'
-    - 'spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb'
 
 # Configuration parameters: AllowComments, AllowEmptyLambdas.
 Lint/EmptyBlock:


@@ 359,7 358,6 @@ RSpec/LetSetup:
     - 'spec/services/suspend_account_service_spec.rb'
     - 'spec/services/unallow_domain_service_spec.rb'
     - 'spec/services/unsuspend_account_service_spec.rb'
-    - 'spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb'
     - 'spec/workers/scheduler/user_cleanup_scheduler_spec.rb'
 
 RSpec/MessageChain:

@@ 12,11 12,6 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
   let!(:account5)  { Fabricate(:account, domain: nil) }
   let!(:remote)    { Fabricate(:account) }
 
-  let!(:policy1)   { Fabricate(:account_statuses_cleanup_policy, account: account1) }
-  let!(:policy2)   { Fabricate(:account_statuses_cleanup_policy, account: account3) }
-  let!(:policy3)   { Fabricate(:account_statuses_cleanup_policy, account: account4, enabled: false) }
-  let!(:policy4)   { Fabricate(:account_statuses_cleanup_policy, account: account5) }
-
   let(:queue_size)       { 0 }
   let(:queue_latency)    { 0 }
   let(:process_set_stub) do


@@ 29,33 24,12 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
   end
 
   before do
-    queue_stub = double
-    allow(queue_stub).to receive(:size).and_return(queue_size)
-    allow(queue_stub).to receive(:latency).and_return(queue_latency)
+    queue_stub = instance_double(Sidekiq::Queue, size: queue_size, latency: queue_latency)
     allow(Sidekiq::Queue).to receive(:new).and_return(queue_stub)
     allow(Sidekiq::ProcessSet).to receive(:new).and_return(process_set_stub)
 
-    sidekiq_stats_stub = double
+    sidekiq_stats_stub = instance_double(Sidekiq::Stats)
     allow(Sidekiq::Stats).to receive(:new).and_return(sidekiq_stats_stub)
-
-    # Create a bunch of old statuses
-    10.times do
-      Fabricate(:status, account: account1, created_at: 3.years.ago)
-      Fabricate(:status, account: account2, created_at: 3.years.ago)
-      Fabricate(:status, account: account3, created_at: 3.years.ago)
-      Fabricate(:status, account: account4, created_at: 3.years.ago)
-      Fabricate(:status, account: account5, created_at: 3.years.ago)
-      Fabricate(:status, account: remote, created_at: 3.years.ago)
-    end
-
-    # Create a bunch of newer statuses
-    5.times do
-      Fabricate(:status, account: account1, created_at: 3.minutes.ago)
-      Fabricate(:status, account: account2, created_at: 3.minutes.ago)
-      Fabricate(:status, account: account3, created_at: 3.minutes.ago)
-      Fabricate(:status, account: account4, created_at: 3.minutes.ago)
-      Fabricate(:status, account: remote, created_at: 3.minutes.ago)
-    end
   end
 
   describe '#under_load?' do


@@ 101,23 75,45 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
   end
 
   describe '#perform' do
-    context 'when the budget is lower than the number of toots to delete' do
-      it 'deletes as many statuses as the given budget' do
-        expect { subject.perform }.to change(Status, :count).by(-subject.compute_budget)
+    before do
+      # Policies for the accounts
+      Fabricate(:account_statuses_cleanup_policy, account: account1)
+      Fabricate(:account_statuses_cleanup_policy, account: account3)
+      Fabricate(:account_statuses_cleanup_policy, account: account4, enabled: false)
+      Fabricate(:account_statuses_cleanup_policy, account: account5)
+
+      # Create a bunch of old statuses
+      4.times do
+        Fabricate(:status, account: account1, created_at: 3.years.ago)
+        Fabricate(:status, account: account2, created_at: 3.years.ago)
+        Fabricate(:status, account: account3, created_at: 3.years.ago)
+        Fabricate(:status, account: account4, created_at: 3.years.ago)
+        Fabricate(:status, account: account5, created_at: 3.years.ago)
+        Fabricate(:status, account: remote, created_at: 3.years.ago)
       end
 
-      it 'does not delete from accounts with no cleanup policy' do
-        expect { subject.perform }.to_not change { account2.statuses.count }
-      end
+      # Create a bunch of newer statuses
+      Fabricate(:status, account: account1, created_at: 3.minutes.ago)
+      Fabricate(:status, account: account2, created_at: 3.minutes.ago)
+      Fabricate(:status, account: account3, created_at: 3.minutes.ago)
+      Fabricate(:status, account: account4, created_at: 3.minutes.ago)
+      Fabricate(:status, account: remote, created_at: 3.minutes.ago)
+    end
+
+    context 'when the budget is lower than the number of toots to delete' do
+      it 'deletes the appropriate statuses' do
+        expect(Status.count).to be > (subject.compute_budget) # Data check
 
-      it 'does not delete from accounts with disabled cleanup policies' do
-        expect { subject.perform }.to_not change { account4.statuses.count }
+        expect { subject.perform }
+          .to change(Status, :count).by(-subject.compute_budget) # Cleanable statuses
+          .and (not_change { account2.statuses.count }) # No cleanup policy for account
+          .and(not_change { account4.statuses.count }) # Disabled cleanup policy
       end
 
       it 'eventually deletes every deletable toot given enough runs' do
         stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 4
 
-        expect { 10.times { subject.perform } }.to change(Status, :count).by(-30)
+        expect { 3.times { subject.perform } }.to change(Status, :count).by(-cleanable_statuses_count)
       end
 
       it 'correctly round-trips between users across several runs' do


@@ 128,7 124,7 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
           .to change(Status, :count).by(-3 * 3)
           .and change { account1.statuses.count }
           .and change { account3.statuses.count }
-          .and change { account5.statuses.count }
+          .and(change { account5.statuses.count })
       end
 
       context 'when given a big budget' do


@@ 140,7 136,7 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
 
         it 'correctly handles looping in a single run' do
           expect(subject.compute_budget).to eq(400)
-          expect { subject.perform }.to change(Status, :count).by(-30)
+          expect { subject.perform }.to change(Status, :count).by(-cleanable_statuses_count)
         end
       end
 


@@ 157,6 153,13 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
           expect { subject.perform }.to_not change(Status, :count)
         end
       end
+
+      def cleanable_statuses_count
+        Status
+          .where(account_id: [account1, account3, account5]) # Accounts with enabled policies
+          .where('created_at < ?', 2.weeks.ago) # Policy defaults is 2.weeks
+          .count
+      end
     end
   end
 end