From 9dc3ce878b51ab80b19365b2a74101cb3e5a8903 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Fri, 10 Nov 2023 10:13:42 -0500 Subject: [PATCH] Speed-up in `Settings::` controllers specs (#27808) --- .../settings/aliases_controller_spec.rb | 5 +- .../settings/applications_controller_spec.rb | 66 ++++++------------ .../settings/deletes_controller_spec.rb | 21 +----- .../settings/exports_controller_spec.rb | 5 +- .../settings/imports_controller_spec.rb | 69 ++++--------------- .../login_activities_controller_spec.rb | 5 +- .../migration/redirects_controller_spec.rb | 5 +- .../settings/migrations_controller_spec.rb | 21 +++--- .../preferences/appearance_controller_spec.rb | 5 +- .../notifications_controller_spec.rb | 5 +- .../preferences/other_controller_spec.rb | 5 +- .../settings/profiles_controller_spec.rb | 5 +- .../webauthn_credentials_controller_spec.rb | 59 +++------------- ..._authentication_methods_controller_spec.rb | 5 +- 14 files changed, 65 insertions(+), 216 deletions(-) diff --git a/spec/controllers/settings/aliases_controller_spec.rb b/spec/controllers/settings/aliases_controller_spec.rb index 9636c1ac5..18e568be0 100644 --- a/spec/controllers/settings/aliases_controller_spec.rb +++ b/spec/controllers/settings/aliases_controller_spec.rb @@ -17,11 +17,8 @@ describe Settings::AliasesController do get :index end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/applications_controller_spec.rb b/spec/controllers/settings/applications_controller_spec.rb index 169304b3e..ccbb63491 100644 --- a/spec/controllers/settings/applications_controller_spec.rb +++ b/spec/controllers/settings/applications_controller_spec.rb @@ -18,11 +18,8 @@ describe Settings::ApplicationsController do get :index end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end @@ -51,7 +48,7 @@ describe Settings::ApplicationsController do describe 'POST #create' do context 'when success (passed scopes as a String)' do - def call_create + subject do post :create, params: { doorkeeper_application: { name: 'My New App', @@ -60,20 +57,16 @@ describe Settings::ApplicationsController do scopes: 'read write follow', }, } - response end - it 'creates an entry in the database' do - expect { call_create }.to change(Doorkeeper::Application, :count) - end - - it 'redirects back to applications page' do - expect(call_create).to redirect_to(settings_applications_path) + it 'creates an entry in the database', :aggregate_failures do + expect { subject }.to change(Doorkeeper::Application, :count) + expect(response).to redirect_to(settings_applications_path) end end context 'when success (passed scopes as an Array)' do - def call_create + subject do post :create, params: { doorkeeper_application: { name: 'My New App', @@ -82,15 +75,11 @@ describe Settings::ApplicationsController do scopes: %w(read write follow), }, } - response end - it 'creates an entry in the database' do - expect { call_create }.to change(Doorkeeper::Application, :count) - end - - it 'redirects back to applications page' do - expect(call_create).to redirect_to(settings_applications_path) + it 'creates an entry in the database', :aggregate_failures do + expect { subject }.to change(Doorkeeper::Application, :count) + expect(response).to redirect_to(settings_applications_path) end end @@ -106,11 +95,8 @@ describe Settings::ApplicationsController do } end - it 'returns http success' do + it 'returns http success and renders form', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'renders form again' do expect(response).to render_template(:new) end end @@ -118,13 +104,7 @@ describe Settings::ApplicationsController do describe 'PATCH #update' do context 'when success' do - let(:opts) do - { - website: 'https://foo.bar/', - } - end - - def call_update + subject do patch :update, params: { id: app.id, doorkeeper_application: opts, @@ -132,13 +112,17 @@ describe Settings::ApplicationsController do response end - it 'updates existing application' do - call_update - expect(app.reload.website).to eql(opts[:website]) + let(:opts) do + { + website: 'https://foo.bar/', + } end - it 'redirects back to applications page' do - expect(call_update).to redirect_to(settings_application_path(app)) + it 'updates existing application' do + subject + + expect(app.reload.website).to eql(opts[:website]) + expect(response).to redirect_to(settings_application_path(app)) end end @@ -155,11 +139,8 @@ describe Settings::ApplicationsController do } end - it 'returns http success' do + it 'returns http success and renders form', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'renders form again' do expect(response).to render_template(:show) end end @@ -170,11 +151,8 @@ describe Settings::ApplicationsController do post :destroy, params: { id: app.id } end - it 'redirects back to applications page' do + it 'redirects back to applications page and removes the app' do expect(response).to redirect_to(settings_applications_path) - end - - it 'removes the app' do expect(Doorkeeper::Application.find_by(id: app.id)).to be_nil end end diff --git a/spec/controllers/settings/deletes_controller_spec.rb b/spec/controllers/settings/deletes_controller_spec.rb index 7ee18f9fb..2c1532ecd 100644 --- a/spec/controllers/settings/deletes_controller_spec.rb +++ b/spec/controllers/settings/deletes_controller_spec.rb @@ -14,22 +14,16 @@ describe Settings::DeletesController do get :show end - it 'renders confirmation page' do + it 'renders confirmation page with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end context 'when suspended' do let(:user) { Fabricate(:user, account_attributes: { suspended_at: Time.now.utc }) } - it 'returns http forbidden' do + it 'returns http forbidden with private cache control headers', :aggregate_failures do expect(response).to have_http_status(403) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end @@ -56,19 +50,10 @@ describe Settings::DeletesController do delete :destroy, params: { form_delete_confirmation: { password: 'petsmoldoggos' } } end - it 'redirects to sign in page' do + it 'removes user record and redirects', :aggregate_failures do expect(response).to redirect_to '/auth/sign_in' - end - - it 'removes user record' do expect(User.find_by(id: user.id)).to be_nil - end - - it 'marks account as suspended' do expect(user.account.reload).to be_suspended - end - - it 'does not create an email block' do expect(CanonicalEmailBlock.block?(user.email)).to be false end diff --git a/spec/controllers/settings/exports_controller_spec.rb b/spec/controllers/settings/exports_controller_spec.rb index 6a4202131..c8c11c3be 100644 --- a/spec/controllers/settings/exports_controller_spec.rb +++ b/spec/controllers/settings/exports_controller_spec.rb @@ -14,11 +14,8 @@ describe Settings::ExportsController do get :show end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/imports_controller_spec.rb b/spec/controllers/settings/imports_controller_spec.rb index 900d0eb90..1e7b75893 100644 --- a/spec/controllers/settings/imports_controller_spec.rb +++ b/spec/controllers/settings/imports_controller_spec.rb @@ -19,15 +19,9 @@ RSpec.describe Settings::ImportsController do get :index end - it 'assigns the expected imports' do - expect(assigns(:recent_imports)).to eq [import] - end - - it 'returns http success' do + it 'assigns the expected imports', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do + expect(assigns(:recent_imports)).to eq [import] expect(response.headers['Cache-Control']).to include('private, no-store') end end @@ -72,17 +66,10 @@ RSpec.describe Settings::ImportsController do context 'with someone else\'s import' do let(:bulk_import) { Fabricate(:bulk_import, state: :unconfirmed) } - it 'does not change the import\'s state' do + it 'does not change the import\'s state and returns missing', :aggregate_failures do expect { subject }.to_not(change { bulk_import.reload.state }) - end - it 'does not fire the import worker' do - subject expect(BulkImportWorker).to_not have_received(:perform_async) - end - - it 'returns http not found' do - subject expect(response).to have_http_status(404) end end @@ -90,17 +77,10 @@ RSpec.describe Settings::ImportsController do context 'with an already-confirmed import' do let(:bulk_import) { Fabricate(:bulk_import, account: user.account, state: :in_progress) } - it 'does not change the import\'s state' do + it 'does not change the import\'s state and returns missing', :aggregate_failures do expect { subject }.to_not(change { bulk_import.reload.state }) - end - it 'does not fire the import worker' do - subject expect(BulkImportWorker).to_not have_received(:perform_async) - end - - it 'returns http not found' do - subject expect(response).to have_http_status(404) end end @@ -108,17 +88,10 @@ RSpec.describe Settings::ImportsController do context 'with an unconfirmed import' do let(:bulk_import) { Fabricate(:bulk_import, account: user.account, state: :unconfirmed) } - it 'changes the import\'s state to scheduled' do + it 'changes the import\'s state to scheduled and redirects', :aggregate_failures do expect { subject }.to change { bulk_import.reload.state.to_sym }.from(:unconfirmed).to(:scheduled) - end - it 'fires the import worker on the expected import' do - subject expect(BulkImportWorker).to have_received(:perform_async).with(bulk_import.id) - end - - it 'redirects to imports path' do - subject expect(response).to redirect_to(settings_imports_path) end end @@ -130,12 +103,9 @@ RSpec.describe Settings::ImportsController do context 'with someone else\'s import' do let(:bulk_import) { Fabricate(:bulk_import, state: :unconfirmed) } - it 'does not delete the import' do + it 'does not delete the import and returns missing', :aggregate_failures do expect { subject }.to_not(change { BulkImport.exists?(bulk_import.id) }) - end - it 'returns http not found' do - subject expect(response).to have_http_status(404) end end @@ -143,12 +113,9 @@ RSpec.describe Settings::ImportsController do context 'with an already-confirmed import' do let(:bulk_import) { Fabricate(:bulk_import, account: user.account, state: :in_progress) } - it 'does not delete the import' do + it 'does not delete the import and returns missing', :aggregate_failures do expect { subject }.to_not(change { BulkImport.exists?(bulk_import.id) }) - end - it 'returns http not found' do - subject expect(response).to have_http_status(404) end end @@ -156,12 +123,9 @@ RSpec.describe Settings::ImportsController do context 'with an unconfirmed import' do let(:bulk_import) { Fabricate(:bulk_import, account: user.account, state: :unconfirmed) } - it 'deletes the import' do + it 'deletes the import and redirects', :aggregate_failures do expect { subject }.to change { BulkImport.exists?(bulk_import.id) }.from(true).to(false) - end - it 'redirects to imports path' do - subject expect(response).to redirect_to(settings_imports_path) end end @@ -177,13 +141,10 @@ RSpec.describe Settings::ImportsController do bulk_import.update(total_items: bulk_import.rows.count, processed_items: bulk_import.rows.count, imported_items: 0) end - it 'returns http success' do + it 'returns expected contents', :aggregate_failures do subject - expect(response).to have_http_status(200) - end - it 'returns expected contents' do - subject + expect(response).to have_http_status(200) expect(response.body).to eq expected_contents end end @@ -283,12 +244,9 @@ RSpec.describe Settings::ImportsController do let(:import_file) { file } let(:import_mode) { mode } - it 'creates an unconfirmed bulk_import with expected type' do + it 'creates an unconfirmed bulk_import with expected type and redirects', :aggregate_failures do expect { subject }.to change { user.account.bulk_imports.pluck(:state, :type) }.from([]).to([['unconfirmed', import_type]]) - end - it 'redirects to confirmation page for the import' do - subject expect(response).to redirect_to(settings_import_path(user.account.bulk_imports.first)) end end @@ -298,12 +256,9 @@ RSpec.describe Settings::ImportsController do let(:import_file) { file } let(:import_mode) { mode } - it 'does not creates an unconfirmed bulk_import' do + it 'does not creates an unconfirmed bulk_import', :aggregate_failures do expect { subject }.to_not(change { user.account.bulk_imports.count }) - end - it 'sets error to the import' do - subject expect(assigns(:import).errors).to_not be_empty end end diff --git a/spec/controllers/settings/login_activities_controller_spec.rb b/spec/controllers/settings/login_activities_controller_spec.rb index 80c8f484c..4f266e03d 100644 --- a/spec/controllers/settings/login_activities_controller_spec.rb +++ b/spec/controllers/settings/login_activities_controller_spec.rb @@ -16,11 +16,8 @@ describe Settings::LoginActivitiesController do get :index end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/migration/redirects_controller_spec.rb b/spec/controllers/settings/migration/redirects_controller_spec.rb index aa6df64cf..b909a0266 100644 --- a/spec/controllers/settings/migration/redirects_controller_spec.rb +++ b/spec/controllers/settings/migration/redirects_controller_spec.rb @@ -16,11 +16,8 @@ describe Settings::Migration::RedirectsController do get :new end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/migrations_controller_spec.rb b/spec/controllers/settings/migrations_controller_spec.rb index 9b12bc40f..f3340574d 100644 --- a/spec/controllers/settings/migrations_controller_spec.rb +++ b/spec/controllers/settings/migrations_controller_spec.rb @@ -71,24 +71,22 @@ describe Settings::MigrationsController do context 'when acct is the current account' do let(:acct) { user.account } - it 'renders show' do - expect(subject).to render_template :show - end + it 'does not update the moved account', :aggregate_failures do + subject - it 'does not update the moved account' do expect(user.account.reload.moved_to_account_id).to be_nil + expect(response).to render_template :show end end context 'when target account does not reference the account being moved from' do let(:acct) { Fabricate(:account, also_known_as: []) } - it 'renders show' do - expect(subject).to render_template :show - end + it 'does not update the moved account', :aggregate_failures do + subject - it 'does not update the moved account' do expect(user.account.reload.moved_to_account_id).to be_nil + expect(response).to render_template :show end end @@ -100,12 +98,11 @@ describe Settings::MigrationsController do user.account.migrations.create!(acct: moved_to.acct) end - it 'renders show' do - expect(subject).to render_template :show - end + it 'does not update the moved account', :aggregate_failures do + subject - it 'does not update the moved account' do expect(user.account.reload.moved_to_account_id).to be_nil + expect(response).to render_template :show end end end diff --git a/spec/controllers/settings/preferences/appearance_controller_spec.rb b/spec/controllers/settings/preferences/appearance_controller_spec.rb index 9a98a4188..ee0ded1b9 100644 --- a/spec/controllers/settings/preferences/appearance_controller_spec.rb +++ b/spec/controllers/settings/preferences/appearance_controller_spec.rb @@ -16,11 +16,8 @@ describe Settings::Preferences::AppearanceController do get :show end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/preferences/notifications_controller_spec.rb b/spec/controllers/settings/preferences/notifications_controller_spec.rb index 6a04df9ed..b61d7461c 100644 --- a/spec/controllers/settings/preferences/notifications_controller_spec.rb +++ b/spec/controllers/settings/preferences/notifications_controller_spec.rb @@ -16,11 +16,8 @@ describe Settings::Preferences::NotificationsController do get :show end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/preferences/other_controller_spec.rb b/spec/controllers/settings/preferences/other_controller_spec.rb index 750510b04..61a94a414 100644 --- a/spec/controllers/settings/preferences/other_controller_spec.rb +++ b/spec/controllers/settings/preferences/other_controller_spec.rb @@ -16,11 +16,8 @@ describe Settings::Preferences::OtherController do get :show end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/profiles_controller_spec.rb b/spec/controllers/settings/profiles_controller_spec.rb index 806fad19a..e3197f0a6 100644 --- a/spec/controllers/settings/profiles_controller_spec.rb +++ b/spec/controllers/settings/profiles_controller_spec.rb @@ -17,11 +17,8 @@ RSpec.describe Settings::ProfilesController do get :show end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end diff --git a/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb b/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb index 2ffad13c6..719ed2f88 100644 --- a/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication/webauthn_credentials_controller_spec.rb @@ -121,24 +121,12 @@ describe Settings::TwoFactorAuthentication::WebauthnCredentialsController do add_webauthn_credential(user) end - it 'returns http success' do - get :options + it 'includes existing credentials in list of excluded credentials', :aggregate_failures do + expect { get :options }.to_not change(user, :webauthn_id) expect(response).to have_http_status(200) - end - - it 'stores the challenge on the session' do - get :options expect(controller.session[:webauthn_challenge]).to be_present - end - - it 'does not change webauthn_id' do - expect { get :options }.to_not change(user, :webauthn_id) - end - - it 'includes existing credentials in list of excluded credentials' do - get :options excluded_credentials_ids = response.parsed_body['excludeCredentials'].pluck('id') expect(excluded_credentials_ids).to match_array(user.webauthn_credentials.pluck(:external_id)) @@ -146,21 +134,11 @@ describe Settings::TwoFactorAuthentication::WebauthnCredentialsController do end context 'when user does not have webauthn enabled' do - it 'returns http success' do + it 'stores the challenge on the session and sets user webauthn_id', :aggregate_failures do get :options expect(response).to have_http_status(200) - end - - it 'stores the challenge on the session' do - get :options - expect(controller.session[:webauthn_challenge]).to be_present - end - - it 'sets user webauthn_id' do - get :options - expect(user.reload.webauthn_id).to be_present end end @@ -217,28 +195,15 @@ describe Settings::TwoFactorAuthentication::WebauthnCredentialsController do end context 'when creation succeeds' do - it 'returns http success' do - controller.session[:webauthn_challenge] = challenge - - post :create, params: { credential: new_webauthn_credential, nickname: nickname } - - expect(response).to have_http_status(200) - end - - it 'adds a new credential to user credentials' do + it 'adds a new credential to user credentials and does not change webauthn_id', :aggregate_failures do controller.session[:webauthn_challenge] = challenge expect do post :create, params: { credential: new_webauthn_credential, nickname: nickname } end.to change { user.webauthn_credentials.count }.by(1) - end + .and not_change(user, :webauthn_id) - it 'does not change webauthn_id' do - controller.session[:webauthn_challenge] = challenge - - expect do - post :create, params: { credential: new_webauthn_credential, nickname: nickname } - end.to_not change(user, :webauthn_id) + expect(response).to have_http_status(200) end end @@ -328,17 +293,13 @@ describe Settings::TwoFactorAuthentication::WebauthnCredentialsController do end context 'when deletion succeeds' do - it 'redirects to 2FA methods list and shows flash success' do - delete :destroy, params: { id: user.webauthn_credentials.take.id } - - expect(response).to redirect_to settings_two_factor_authentication_methods_path - expect(flash[:success]).to be_present - end - - it 'deletes the credential' do + it 'redirects to 2FA methods list and shows flash success and deletes the credential', :aggregate_failures do expect do delete :destroy, params: { id: user.webauthn_credentials.take.id } end.to change { user.webauthn_credentials.count }.by(-1) + + expect(response).to redirect_to settings_two_factor_authentication_methods_path + expect(flash[:success]).to be_present end end end diff --git a/spec/controllers/settings/two_factor_authentication_methods_controller_spec.rb b/spec/controllers/settings/two_factor_authentication_methods_controller_spec.rb index 3e61912ad..de0d28463 100644 --- a/spec/controllers/settings/two_factor_authentication_methods_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication_methods_controller_spec.rb @@ -29,11 +29,8 @@ describe Settings::TwoFactorAuthenticationMethodsController do get :index end - it 'returns http success' do + it 'returns http success with private cache control headers', :aggregate_failures do expect(response).to have_http_status(200) - end - - it 'returns private cache control headers' do expect(response.headers['Cache-Control']).to include('private, no-store') end end