From 31d5bc89d12cd2c5c9b148e4abaa37b2f553a8d5 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 14 Jun 2023 03:56:11 -0400 Subject: [PATCH] Speed improvement for `AccountsStatusesCleanupScheduler` spec (#25406) --- .rubocop_todo.yml | 2 - ...ccounts_statuses_cleanup_scheduler_spec.rb | 83 ++++++++++--------- 2 files changed, 43 insertions(+), 42 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c82cc2a4b7..4cf238595b 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -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: diff --git a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb index fb626596fe..62c353bfe1 100644 --- a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb @@ -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 + 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 + + # 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 as many statuses as the given budget' do - expect { subject.perform }.to change(Status, :count).by(-subject.compute_budget) - end + it 'deletes the appropriate statuses' do + expect(Status.count).to be > (subject.compute_budget) # Data check - it 'does not delete from accounts with no cleanup policy' do - expect { subject.perform }.to_not change { account2.statuses.count } - end - - 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