Speed up specs for merge request approval state

Remove offenses for Rails/SaveBang

.rubocop_todo.yml

@@ -700,7 +700,6 @@ Rails/SaveBang:
     - 'ee/spec/models/application_setting_spec.rb'
     - 'ee/spec/models/approval_merge_request_rule_spec.rb'
     - 'ee/spec/models/approval_project_rule_spec.rb'
-    - 'ee/spec/models/approval_state_spec.rb'
     - 'ee/spec/models/burndown_spec.rb'
     - 'ee/spec/models/ci/pipeline_spec.rb'
     - 'ee/spec/models/ci/subscriptions/project_spec.rb'

ee/spec/models/approval_state_spec.rb

@@ -4,7 +4,6 @@ require 'spec_helper'
 
 RSpec.describe ApprovalState do
   def create_rule(additional_params = {})
-    default_approver = create(:user)
     params = additional_params.reverse_merge(merge_request: merge_request, users: [default_approver])
     factory =
       case params.delete(:rule_type)
@@ -28,14 +27,22 @@ RSpec.describe ApprovalState do
     allow(subject).to receive(:approval_feature_available?).and_return(false)
   end
 
-  let(:merge_request) { create(:merge_request) }
-  let(:project) { merge_request.target_project }
-  let(:approver1) { create(:user) }
-  let(:approver2) { create(:user) }
-  let(:approver3) { create(:user) }
+  def users(amount)
+    raise ArgumentError, 'not enough users' if amount > arbitrary_users.size
 
-  let(:group_approver1) { create(:user) }
-  let(:group1) do
+    arbitrary_users.take(amount)
+  end
+
+  let_it_be_with_refind(:project) { create(:project, :repository) }
+  let_it_be_with_refind(:merge_request) { create(:merge_request, source_project: project) }
+  let_it_be(:approver1) { create(:user) }
+  let_it_be(:approver2) { create(:user) }
+  let_it_be(:approver3) { create(:user) }
+  let_it_be(:default_approver) { create(:user) }
+  let_it_be(:arbitrary_users) { create_list(:user, 2) }
+
+  let_it_be(:group_approver1) { create(:user) }
+  let_it_be(:group1) do
     group = create(:group)
     group.add_developer(group_approver1)
     group
@@ -50,8 +57,8 @@ RSpec.describe ApprovalState do
     before do
       allow(merge_request).to receive(:committers).and_return(User.where(id: committers))
 
-      allow(project).to receive(:merge_requests_author_approval?).and_return(merge_requests_author_approval)
-      allow(project).to receive(:merge_requests_disable_committers_approval?).and_return(merge_requests_disable_committers_approval)
+      allow(merge_request.project).to receive(:merge_requests_author_approval?).and_return(merge_requests_author_approval)
+      allow(merge_request.project).to receive(:merge_requests_disable_committers_approval?).and_return(merge_requests_disable_committers_approval)
 
       create_rule(users: committers)
     end
@@ -98,6 +105,24 @@ RSpec.describe ApprovalState do
     it { expect(subject.can_approve?(nil)).to be_falsey }
   end
 
+  shared_context 'project members' do
+    def create_project_member(role, user_attrs = {})
+      user = create(:user, user_attrs)
+      project.add_user(user, role)
+      user
+    end
+
+    let_it_be_with_refind(:project) { create(:project, :repository) }
+    let_it_be(:author) { create_project_member(:developer) }
+    let_it_be_with_refind(:merge_request) { create(:merge_request, source_project: project, author: author) }
+    let_it_be(:approver) { create_project_member(:developer) }
+    let_it_be(:approver2) { create_project_member(:developer) }
+    let_it_be(:developer) { create_project_member(:developer) }
+    let_it_be(:other_developer) { create_project_member(:developer) }
+    let_it_be(:reporter) { create_project_member(:reporter) }
+    let_it_be(:stranger) { create(:user) }
+  end
+
   describe '#approval_rules_overwritten?' do
     context 'when approval rule on the merge request does not exist' do
       it 'returns false' do
@@ -247,7 +272,7 @@ RSpec.describe ApprovalState do
 
       context 'when only code owner rules present' do
         before do
-          2.times { create_rule(users: [create(:user)], rule_type: :code_owner) }
+          users(2).each { |user| create_rule(users: [user], rule_type: :code_owner) }
         end
 
         it_behaves_like 'when rules are present'
@@ -256,7 +281,7 @@ RSpec.describe ApprovalState do
 
       context 'when only report approver rules present' do
         before do
-          2.times { create_rule(users: [create(:user)], rule_type: :report_approver) }
+          users(2).each { |user| create_rule(users: [user], rule_type: :report_approver) }
         end
 
         it_behaves_like 'when rules are present'
@@ -265,7 +290,7 @@ RSpec.describe ApprovalState do
 
       context 'when regular rules present' do
         before do
-          2.times { create_rule(users: [create(:user)]) }
+          users(2).each { |user| create_rule(users: [user]) }
         end
 
         it_behaves_like 'when rules are present'
@@ -274,7 +299,7 @@ RSpec.describe ApprovalState do
       context 'when approval feature is unavailable' do
         it 'returns true' do
           disable_feature
-          create_rule(users: [create(:user)], approvals_required: 1)
+          create_rule(users: users(1), approvals_required: 1)
 
           expect(subject.approved?).to eq(true)
         end
@@ -302,7 +327,7 @@ RSpec.describe ApprovalState do
 
     describe '#approval_rules_left' do
       def create_unapproved_rule
-        create_rule(approvals_required: 1, users: [create(:user)])
+        create_rule(approvals_required: 1, users: users(1))
       end
 
       before do
@@ -381,7 +406,7 @@ RSpec.describe ApprovalState do
         create_rule(users: [approver1, approver2])
         create_rule(users: [approver1])
 
-        merge_request.approvals.create(user: approver2)
+        merge_request.approvals.create!(user: approver2)
 
         expect(subject.unactioned_approvers).to contain_exactly(approver1)
       end
@@ -410,23 +435,10 @@ RSpec.describe ApprovalState do
         end
       end
 
-      def create_project_member(role, user_attrs = {})
-        user = create(:user, user_attrs)
-        project.add_user(user, role)
-        user
+      include_context 'project members' do
+        let_it_be(:committer) { create_project_member(:developer, email: merge_request.commits.without_merge_commits.first.committer_email) }
       end
 
-      let(:project) { create(:project, :repository) }
-      let(:merge_request) { create(:merge_request, source_project: project, author: author) }
-      let(:author) { create_project_member(:developer) }
-      let(:approver) { create_project_member(:developer) }
-      let(:approver2) { create_project_member(:developer) }
-      let(:committer) { create_project_member(:developer, email: merge_request.commits.without_merge_commits.first.committer_email) }
-      let(:developer) { create_project_member(:developer) }
-      let(:other_developer) { create_project_member(:developer) }
-      let(:reporter) { create_project_member(:reporter) }
-      let(:stranger) { create(:user) }
-
       context 'when there are no regular approval rules' do
         let!(:any_approver_rule) do
           create(:approval_project_rule, project: project, rule_type: :any_approver, approvals_required: 1)
@@ -833,7 +845,7 @@ RSpec.describe ApprovalState do
   context 'when only a single rule is allowed' do
     def create_unapproved_rule(additional_params = {})
       create_rule(
-        additional_params.reverse_merge(approvals_required: 1, users: [create(:user)])
+        additional_params.reverse_merge(approvals_required: 1, users: users(1))
       )
     end
 
@@ -957,7 +969,7 @@ RSpec.describe ApprovalState do
       context 'when only code owner rules present' do
         before do
           # setting approvals required to 0 since we don't want to block on them now
-          2.times { create_rule(users: [create(:user)], rule_type: :code_owner, approvals_required: 0) }
+          users(2).each { |user| create_rule(users: [user], rule_type: :code_owner, approvals_required: 0) }
         end
 
         it_behaves_like 'when rules are present'
@@ -966,7 +978,7 @@ RSpec.describe ApprovalState do
 
       context 'when only report approver rules present' do
         before do
-          2.times { create_rule(users: [create(:user)], rule_type: :report_approver) }
+          users(2).each { |user| create_rule(users: [user], rule_type: :report_approver) }
         end
 
         it_behaves_like 'when rules are present'
@@ -976,7 +988,7 @@ RSpec.describe ApprovalState do
       context 'when regular rules present' do
         before do
           project.update!(approvals_before_merge: 999)
-          2.times { create_rule(users: [create(:user)]) }
+          users(2).each { |user| create_rule(users: [user]) }
         end
 
         it_behaves_like 'when rules are present'
@@ -984,7 +996,7 @@ RSpec.describe ApprovalState do
 
       context 'when a single project rule is present' do
         before do
-          create(:approval_project_rule, users: [create(:user)], project: project)
+          create(:approval_project_rule, users: users(1), project: project)
         end
 
         it_behaves_like 'when rules are present'
@@ -992,7 +1004,7 @@ RSpec.describe ApprovalState do
         context 'when the project rule is overridden by a fallback but the project does not allow overriding' do
           before do
             merge_request.update!(approvals_before_merge: 1)
-            project.update!(disable_overriding_approvers_per_merge_request: true)
+            merge_request.project.update!(disable_overriding_approvers_per_merge_request: true)
           end
 
           it_behaves_like 'when rules are present'
@@ -1009,7 +1021,7 @@ RSpec.describe ApprovalState do
 
       context 'when a single project rule is present that is overridden in the merge request' do
         before do
-          create(:approval_project_rule, users: [create(:user)], project: project)
+          create(:approval_project_rule, users: users(1), project: project)
           merge_request.update!(approvals_before_merge: 1)
         end
 
@@ -1089,7 +1101,7 @@ RSpec.describe ApprovalState do
         create_rule(users: [approver1, approver2])
         create_rule(users: [approver1])
 
-        merge_request.approvals.create(user: approver2)
+        merge_request.approvals.create!(user: approver2)
 
         expect(subject.unactioned_approvers).to contain_exactly(approver1)
       end
@@ -1118,23 +1130,10 @@ RSpec.describe ApprovalState do
         end
       end
 
-      def create_project_member(role)
-        user = create(:user)
-        project.add_user(user, role)
-        user
+      include_context 'project members' do
+        let_it_be(:guest) { create_project_member(:guest) }
       end
 
-      let(:project) { create(:project, :repository) }
-      let(:merge_request) { create(:merge_request, source_project: project, author: author) }
-      let(:author) { create_project_member(:developer) }
-      let(:approver) { create_project_member(:developer) }
-      let(:approver2) { create_project_member(:developer) }
-      let(:developer) { create_project_member(:developer) }
-      let(:other_developer) { create_project_member(:developer) }
-      let(:reporter) { create_project_member(:reporter) }
-      let(:guest) { create_project_member(:guest) }
-      let(:stranger) { create(:user) }
-
       context 'when the user is the author' do
         context 'and author is an approver' do
           before do
@@ -1170,9 +1169,9 @@ RSpec.describe ApprovalState do
       end
 
       context 'when user is a committer' do
-        let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
+        let_it_be(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
 
-        before do
+        before_all do
           project.add_developer(user)
         end
 
@@ -1339,7 +1338,7 @@ RSpec.describe ApprovalState do
 
           context 'when an approver does not have access to the merge request' do
             before do
-              project.members.find_by(user_id: developer.id).destroy
+              project.members.find_by(user_id: developer.id).destroy!
             end
 
             it 'the user cannot approver' do