From 69d00e272188b9f06bc0323cda248a4f7d37d0bd Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 8 Nov 2023 10:42:30 -0500 Subject: [PATCH] Fix `RSpec/InstanceVariable` cop (#27766) --- .rubocop_todo.yml | 20 -------- .../api/v1/streaming_controller_spec.rb | 9 +++- .../auth/passwords_controller_spec.rb | 16 +++--- .../export_controller_concern_spec.rb | 4 +- .../statuses_cleanup_controller_spec.rb | 11 +++-- .../concerns/account_finder_concern_spec.rb | 20 +++----- .../concerns/account_interactions_spec.rb | 18 +++---- spec/models/public_feed_spec.rb | 20 ++++---- spec/services/remove_status_service_spec.rb | 49 +++++++++---------- spec/services/search_service_spec.rb | 16 +++--- spec/services/unblock_domain_service_spec.rb | 38 +++++++------- 11 files changed, 97 insertions(+), 124 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 4d753a71a0..a59f65519d 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -71,26 +71,6 @@ RSpec/AnyInstance: RSpec/ExampleLength: Max: 22 -# Configuration parameters: AssignmentOnly. -RSpec/InstanceVariable: - Exclude: - - 'spec/controllers/api/v1/streaming_controller_spec.rb' - - 'spec/controllers/auth/confirmations_controller_spec.rb' - - 'spec/controllers/auth/passwords_controller_spec.rb' - - 'spec/controllers/auth/sessions_controller_spec.rb' - - 'spec/controllers/concerns/export_controller_concern_spec.rb' - - 'spec/controllers/home_controller_spec.rb' - - 'spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb' - - 'spec/controllers/statuses_cleanup_controller_spec.rb' - - 'spec/models/concerns/account_finder_concern_spec.rb' - - 'spec/models/concerns/account_interactions_spec.rb' - - 'spec/models/public_feed_spec.rb' - - 'spec/serializers/activitypub/note_serializer_spec.rb' - - 'spec/serializers/activitypub/update_poll_serializer_spec.rb' - - 'spec/services/remove_status_service_spec.rb' - - 'spec/services/search_service_spec.rb' - - 'spec/services/unblock_domain_service_spec.rb' - RSpec/LetSetup: Exclude: - 'spec/controllers/api/v1/accounts/statuses_controller_spec.rb' diff --git a/spec/controllers/api/v1/streaming_controller_spec.rb b/spec/controllers/api/v1/streaming_controller_spec.rb index 06639a13fd..c3e7153ce8 100644 --- a/spec/controllers/api/v1/streaming_controller_spec.rb +++ b/spec/controllers/api/v1/streaming_controller_spec.rb @@ -26,7 +26,6 @@ describe Api::V1::StreamingController do context 'with streaming api on different host' do before do Rails.configuration.x.streaming_api_base_url = "wss://streaming-#{Rails.configuration.x.web_domain}" - @streaming_host = URI.parse(Rails.configuration.x.streaming_api_base_url).host end describe 'GET #index' do @@ -38,7 +37,13 @@ describe Api::V1::StreamingController do [:scheme, :path, :query, :fragment].each do |part| expect(redirect_to_uri.send(part)).to eq(request_uri.send(part)), "redirect target #{part}" end - expect(redirect_to_uri.host).to eq(@streaming_host), 'redirect target host' + expect(redirect_to_uri.host).to eq(streaming_host), 'redirect target host' + end + + private + + def streaming_host + URI.parse(Rails.configuration.x.streaming_api_base_url).host end end end diff --git a/spec/controllers/auth/passwords_controller_spec.rb b/spec/controllers/auth/passwords_controller_spec.rb index ef5706ee78..e7f7ab4676 100644 --- a/spec/controllers/auth/passwords_controller_spec.rb +++ b/spec/controllers/auth/passwords_controller_spec.rb @@ -18,12 +18,14 @@ describe Auth::PasswordsController do before do request.env['devise.mapping'] = Devise.mappings[:user] - @token = user.send_reset_password_instructions end context 'with valid reset_password_token' do it 'returns http success' do - get :edit, params: { reset_password_token: @token } + token = user.send_reset_password_instructions + + get :edit, params: { reset_password_token: token } + expect(response).to have_http_status(200) end end @@ -38,9 +40,9 @@ describe Auth::PasswordsController do describe 'POST #update' do let(:user) { Fabricate(:user) } + let(:password) { 'reset0password' } before do - @password = 'reset0password' request.env['devise.mapping'] = Devise.mappings[:user] end @@ -50,9 +52,9 @@ describe Auth::PasswordsController do let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) } before do - @token = user.send_reset_password_instructions + token = user.send_reset_password_instructions - post :update, params: { user: { password: @password, password_confirmation: @password, reset_password_token: @token } } + post :update, params: { user: { password: password, password_confirmation: password, reset_password_token: token } } end it 'redirect to sign in' do @@ -63,7 +65,7 @@ describe Auth::PasswordsController do this_user = User.find(user.id) expect(this_user).to_not be_nil - expect(this_user.valid_password?(@password)).to be true + expect(this_user.valid_password?(password)).to be true end it 'deactivates all sessions' do @@ -81,7 +83,7 @@ describe Auth::PasswordsController do context 'with invalid reset_password_token' do before do - post :update, params: { user: { password: @password, password_confirmation: @password, reset_password_token: 'some_invalid_value' } } + post :update, params: { user: { password: password, password_confirmation: password, reset_password_token: 'some_invalid_value' } } end it 'renders reset password' do diff --git a/spec/controllers/concerns/export_controller_concern_spec.rb b/spec/controllers/concerns/export_controller_concern_spec.rb index e253aa5206..7f0a7c5b54 100644 --- a/spec/controllers/concerns/export_controller_concern_spec.rb +++ b/spec/controllers/concerns/export_controller_concern_spec.rb @@ -11,7 +11,7 @@ describe ExportControllerConcern do end def export_data - @export.account.username + 'body data value' end end @@ -24,7 +24,7 @@ describe ExportControllerConcern do expect(response).to have_http_status(200) expect(response.media_type).to eq 'text/csv' expect(response.headers['Content-Disposition']).to start_with 'attachment; filename="anonymous.csv"' - expect(response.body).to eq user.account.username + expect(response.body).to eq 'body data value' end it 'returns unauthorized when not signed in' do diff --git a/spec/controllers/statuses_cleanup_controller_spec.rb b/spec/controllers/statuses_cleanup_controller_spec.rb index e082b69c51..8a72621993 100644 --- a/spec/controllers/statuses_cleanup_controller_spec.rb +++ b/spec/controllers/statuses_cleanup_controller_spec.rb @@ -5,9 +5,10 @@ require 'rails_helper' RSpec.describe StatusesCleanupController do render_views + let!(:user) { Fabricate(:user) } + before do - @user = Fabricate(:user) - sign_in @user, scope: :user + sign_in user, scope: :user end describe 'GET #show' do @@ -30,9 +31,9 @@ RSpec.describe StatusesCleanupController do end it 'updates the account status cleanup policy' do - expect(@user.account.statuses_cleanup_policy.enabled).to be true - expect(@user.account.statuses_cleanup_policy.keep_direct).to be false - expect(@user.account.statuses_cleanup_policy.keep_polls).to be true + expect(user.account.statuses_cleanup_policy.enabled).to be true + expect(user.account.statuses_cleanup_policy.keep_direct).to be false + expect(user.account.statuses_cleanup_policy.keep_polls).to be true end it 'redirects' do diff --git a/spec/models/concerns/account_finder_concern_spec.rb b/spec/models/concerns/account_finder_concern_spec.rb index 25f4fdec4b..3a94c4d545 100644 --- a/spec/models/concerns/account_finder_concern_spec.rb +++ b/spec/models/concerns/account_finder_concern_spec.rb @@ -4,17 +4,15 @@ require 'rails_helper' describe AccountFinderConcern do describe 'local finders' do - before do - @account = Fabricate(:account, username: 'Alice') - end + let!(:account) { Fabricate(:account, username: 'Alice') } describe '.find_local' do it 'returns case-insensitive result' do - expect(Account.find_local('alice')).to eq(@account) + expect(Account.find_local('alice')).to eq(account) end it 'returns correctly cased result' do - expect(Account.find_local('Alice')).to eq(@account) + expect(Account.find_local('Alice')).to eq(account) end it 'returns nil without a match' do @@ -36,7 +34,7 @@ describe AccountFinderConcern do describe '.find_local!' do it 'returns matching result' do - expect(Account.find_local!('alice')).to eq(@account) + expect(Account.find_local!('alice')).to eq(account) end it 'raises on non-matching result' do @@ -54,17 +52,15 @@ describe AccountFinderConcern do end describe 'remote finders' do - before do - @account = Fabricate(:account, username: 'Alice', domain: 'mastodon.social') - end + let!(:account) { Fabricate(:account, username: 'Alice', domain: 'mastodon.social') } describe '.find_remote' do it 'returns exact match result' do - expect(Account.find_remote('alice', 'mastodon.social')).to eq(@account) + expect(Account.find_remote('alice', 'mastodon.social')).to eq(account) end it 'returns case-insensitive result' do - expect(Account.find_remote('ALICE', 'MASTODON.SOCIAL')).to eq(@account) + expect(Account.find_remote('ALICE', 'MASTODON.SOCIAL')).to eq(account) end it 'returns nil when username does not match' do @@ -90,7 +86,7 @@ describe AccountFinderConcern do describe '.find_remote!' do it 'returns matching result' do - expect(Account.find_remote!('alice', 'mastodon.social')).to eq(@account) + expect(Account.find_remote!('alice', 'mastodon.social')).to eq(account) end it 'raises on non-matching result' do diff --git a/spec/models/concerns/account_interactions_spec.rb b/spec/models/concerns/account_interactions_spec.rb index 84e2c91a85..6687c84436 100644 --- a/spec/models/concerns/account_interactions_spec.rb +++ b/spec/models/concerns/account_interactions_spec.rb @@ -648,38 +648,36 @@ describe AccountInteractions do end describe 'ignoring reblogs from an account' do - before do - @me = Fabricate(:account, username: 'Me') - @you = Fabricate(:account, username: 'You') - end + let!(:me) { Fabricate(:account, username: 'Me') } + let!(:you) { Fabricate(:account, username: 'You') } context 'with the reblogs option unspecified' do before do - @me.follow!(@you) + me.follow!(you) end it 'defaults to showing reblogs' do - expect(@me.muting_reblogs?(@you)).to be(false) + expect(me.muting_reblogs?(you)).to be(false) end end context 'with the reblogs option set to false' do before do - @me.follow!(@you, reblogs: false) + me.follow!(you, reblogs: false) end it 'does mute reblogs' do - expect(@me.muting_reblogs?(@you)).to be(true) + expect(me.muting_reblogs?(you)).to be(true) end end context 'with the reblogs option set to true' do before do - @me.follow!(@you, reblogs: true) + me.follow!(you, reblogs: true) end it 'does not mute reblogs' do - expect(@me.muting_reblogs?(@you)).to be(false) + expect(me.muting_reblogs?(you)).to be(false) end end end diff --git a/spec/models/public_feed_spec.rb b/spec/models/public_feed_spec.rb index 53e01cafd3..20fcdb0024 100644 --- a/spec/models/public_feed_spec.rb +++ b/spec/models/public_feed_spec.rb @@ -137,15 +137,13 @@ RSpec.describe PublicFeed do end describe 'with an account passed in' do - subject { described_class.new(@account).get(20).map(&:id) } + subject { described_class.new(account).get(20).map(&:id) } - before do - @account = Fabricate(:account) - end + let!(:account) { Fabricate(:account) } it 'excludes statuses from accounts blocked by the account' do blocked = Fabricate(:account) - @account.block!(blocked) + account.block!(blocked) blocked_status = Fabricate(:status, account: blocked) expect(subject).to_not include(blocked_status.id) @@ -153,7 +151,7 @@ RSpec.describe PublicFeed do it 'excludes statuses from accounts who have blocked the account' do blocker = Fabricate(:account) - blocker.block!(@account) + blocker.block!(account) blocked_status = Fabricate(:status, account: blocker) expect(subject).to_not include(blocked_status.id) @@ -161,7 +159,7 @@ RSpec.describe PublicFeed do it 'excludes statuses from accounts muted by the account' do muted = Fabricate(:account) - @account.mute!(muted) + account.mute!(muted) muted_status = Fabricate(:status, account: muted) expect(subject).to_not include(muted_status.id) @@ -169,7 +167,7 @@ RSpec.describe PublicFeed do it 'excludes statuses from accounts from personally blocked domains' do blocked = Fabricate(:account, domain: 'example.com') - @account.block_domain!(blocked.domain) + account.block_domain!(blocked.domain) blocked_status = Fabricate(:status, account: blocked) expect(subject).to_not include(blocked_status.id) @@ -177,7 +175,7 @@ RSpec.describe PublicFeed do context 'with language preferences' do it 'excludes statuses in languages not allowed by the account user' do - @account.user.update(chosen_languages: [:en, :es]) + account.user.update(chosen_languages: [:en, :es]) en_status = Fabricate(:status, language: 'en') es_status = Fabricate(:status, language: 'es') fr_status = Fabricate(:status, language: 'fr') @@ -188,7 +186,7 @@ RSpec.describe PublicFeed do end it 'includes all languages when user does not have a setting' do - @account.user.update(chosen_languages: nil) + account.user.update(chosen_languages: nil) en_status = Fabricate(:status, language: 'en') es_status = Fabricate(:status, language: 'es') @@ -198,7 +196,7 @@ RSpec.describe PublicFeed do end it 'includes all languages when account does not have a user' do - @account.update(user: nil) + account.update(user: nil) en_status = Fabricate(:status, language: 'en') es_status = Fabricate(:status, language: 'es') diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb index c19b4fac15..7754ae8004 100644 --- a/spec/services/remove_status_service_spec.rb +++ b/spec/services/remove_status_service_spec.rb @@ -20,71 +20,70 @@ RSpec.describe RemoveStatusService, type: :service do end context 'when removed status is not a reblog' do + let!(:status) { PostStatusService.new.call(alice, text: 'Hello @bob@example.com ThisIsASecret') } + before do - @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com ThisIsASecret') - FavouriteService.new.call(jeff, @status) - Fabricate(:status, account: bill, reblog: @status, uri: 'hoge') + FavouriteService.new.call(jeff, status) + Fabricate(:status, account: bill, reblog: status, uri: 'hoge') end it 'removes status from author\'s home feed' do - subject.call(@status) - expect(HomeFeed.new(alice).get(10).pluck(:id)).to_not include(@status.id) + subject.call(status) + expect(HomeFeed.new(alice).get(10).pluck(:id)).to_not include(status.id) end it 'removes status from local follower\'s home feed' do - subject.call(@status) - expect(HomeFeed.new(jeff).get(10).pluck(:id)).to_not include(@status.id) + subject.call(status) + expect(HomeFeed.new(jeff).get(10).pluck(:id)).to_not include(status.id) end it 'sends Delete activity to followers' do - subject.call(@status) + subject.call(status) expect(a_request(:post, 'http://example.com/inbox').with( body: hash_including({ 'type' => 'Delete', 'object' => { 'type' => 'Tombstone', - 'id' => ActivityPub::TagManager.instance.uri_for(@status), - 'atomUri' => OStatus::TagManager.instance.uri_for(@status), + 'id' => ActivityPub::TagManager.instance.uri_for(status), + 'atomUri' => OStatus::TagManager.instance.uri_for(status), }, }) )).to have_been_made.once end it 'sends Delete activity to rebloggers' do - subject.call(@status) + subject.call(status) expect(a_request(:post, 'http://example2.com/inbox').with( body: hash_including({ 'type' => 'Delete', 'object' => { 'type' => 'Tombstone', - 'id' => ActivityPub::TagManager.instance.uri_for(@status), - 'atomUri' => OStatus::TagManager.instance.uri_for(@status), + 'id' => ActivityPub::TagManager.instance.uri_for(status), + 'atomUri' => OStatus::TagManager.instance.uri_for(status), }, }) )).to have_been_made.once end it 'remove status from notifications' do - expect { subject.call(@status) }.to change { + expect { subject.call(status) }.to change { Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count }.from(1).to(0) end end context 'when removed status is a private self-reblog' do - before do - @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :private) - @status = ReblogService.new.call(alice, @original_status) - end + let!(:original_status) { Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :private) } + let!(:status) { ReblogService.new.call(alice, original_status) } it 'sends Undo activity to followers' do - subject.call(@status) + subject.call(status) expect(a_request(:post, 'http://example.com/inbox').with( body: hash_including({ 'type' => 'Undo', 'object' => hash_including({ 'type' => 'Announce', - 'object' => ActivityPub::TagManager.instance.uri_for(@original_status), + 'object' => ActivityPub::TagManager.instance.uri_for(original_status), }), }) )).to have_been_made.once @@ -92,19 +91,17 @@ RSpec.describe RemoveStatusService, type: :service do end context 'when removed status is public self-reblog' do - before do - @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :public) - @status = ReblogService.new.call(alice, @original_status) - end + let!(:original_status) { Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :public) } + let!(:status) { ReblogService.new.call(alice, original_status) } it 'sends Undo activity to followers' do - subject.call(@status) + subject.call(status) expect(a_request(:post, 'http://example.com/inbox').with( body: hash_including({ 'type' => 'Undo', 'object' => hash_including({ 'type' => 'Announce', - 'object' => ActivityPub::TagManager.instance.uri_for(@original_status), + 'object' => ActivityPub::TagManager.instance.uri_for(original_status), }), }) )).to have_been_made.once diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index cb69af5f54..39adf43876 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -19,17 +19,15 @@ describe SearchService, type: :service do end describe 'with an url query' do - before do - @query = 'http://test.host/query' - end + let(:query) { 'http://test.host/query' } context 'when it does not find anything' do it 'returns the empty results' do service = instance_double(ResolveURLService, call: nil) allow(ResolveURLService).to receive(:new).and_return(service) - results = subject.call(@query, nil, 10, resolve: true) + results = subject.call(query, nil, 10, resolve: true) - expect(service).to have_received(:call).with(@query, on_behalf_of: nil) + expect(service).to have_received(:call).with(query, on_behalf_of: nil) expect(results).to eq empty_results end end @@ -40,8 +38,8 @@ describe SearchService, type: :service do service = instance_double(ResolveURLService, call: account) allow(ResolveURLService).to receive(:new).and_return(service) - results = subject.call(@query, nil, 10, resolve: true) - expect(service).to have_received(:call).with(@query, on_behalf_of: nil) + results = subject.call(query, nil, 10, resolve: true) + expect(service).to have_received(:call).with(query, on_behalf_of: nil) expect(results).to eq empty_results.merge(accounts: [account]) end end @@ -52,8 +50,8 @@ describe SearchService, type: :service do service = instance_double(ResolveURLService, call: status) allow(ResolveURLService).to receive(:new).and_return(service) - results = subject.call(@query, nil, 10, resolve: true) - expect(service).to have_received(:call).with(@query, on_behalf_of: nil) + results = subject.call(query, nil, 10, resolve: true) + expect(service).to have_received(:call).with(query, on_behalf_of: nil) expect(results).to eq empty_results.merge(statuses: [status]) end end diff --git a/spec/services/unblock_domain_service_spec.rb b/spec/services/unblock_domain_service_spec.rb index 27dbc92ada..3d6d82ff68 100644 --- a/spec/services/unblock_domain_service_spec.rb +++ b/spec/services/unblock_domain_service_spec.rb @@ -6,38 +6,36 @@ describe UnblockDomainService, type: :service do subject { described_class.new } describe 'call' do - before do - @independently_suspended = Fabricate(:account, domain: 'example.com', suspended_at: 1.hour.ago) - @independently_silenced = Fabricate(:account, domain: 'example.com', silenced_at: 1.hour.ago) - @domain_block = Fabricate(:domain_block, domain: 'example.com') - @silenced = Fabricate(:account, domain: 'example.com', silenced_at: @domain_block.created_at) - @suspended = Fabricate(:account, domain: 'example.com', suspended_at: @domain_block.created_at) - end + let!(:independently_suspended) { Fabricate(:account, domain: 'example.com', suspended_at: 1.hour.ago) } + let!(:independently_silenced) { Fabricate(:account, domain: 'example.com', silenced_at: 1.hour.ago) } + let!(:domain_block) { Fabricate(:domain_block, domain: 'example.com') } + let!(:silenced) { Fabricate(:account, domain: 'example.com', silenced_at: domain_block.created_at) } + let!(:suspended) { Fabricate(:account, domain: 'example.com', suspended_at: domain_block.created_at) } it 'unsilences accounts and removes block' do - @domain_block.update(severity: :silence) + domain_block.update(severity: :silence) - subject.call(@domain_block) + subject.call(domain_block) expect_deleted_domain_block - expect(@silenced.reload.silenced?).to be false - expect(@suspended.reload.suspended?).to be true - expect(@independently_suspended.reload.suspended?).to be true - expect(@independently_silenced.reload.silenced?).to be true + expect(silenced.reload.silenced?).to be false + expect(suspended.reload.suspended?).to be true + expect(independently_suspended.reload.suspended?).to be true + expect(independently_silenced.reload.silenced?).to be true end it 'unsuspends accounts and removes block' do - @domain_block.update(severity: :suspend) + domain_block.update(severity: :suspend) - subject.call(@domain_block) + subject.call(domain_block) expect_deleted_domain_block - expect(@suspended.reload.suspended?).to be false - expect(@silenced.reload.silenced?).to be false - expect(@independently_suspended.reload.suspended?).to be true - expect(@independently_silenced.reload.silenced?).to be true + expect(suspended.reload.suspended?).to be false + expect(silenced.reload.silenced?).to be false + expect(independently_suspended.reload.suspended?).to be true + expect(independently_silenced.reload.silenced?).to be true end end def expect_deleted_domain_block - expect { @domain_block.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { domain_block.reload }.to raise_error(ActiveRecord::RecordNotFound) end end