Merge branch 'rails-save-bang-36' into 'master'

Fixes Rails/SaveBang cop for spec files in spec/models/concerns/*

See merge request gitlab-org/gitlab!42942

.rubocop_todo.yml

@@ -1038,18 +1038,6 @@ Rails/SaveBang:
     - 'spec/models/clusters/applications/helm_spec.rb'
     - 'spec/models/commit_spec.rb'
     - 'spec/models/commit_status_spec.rb'
-    - 'spec/models/concerns/avatarable_spec.rb'
-    - 'spec/models/concerns/bulk_insertable_associations_spec.rb'
-    - 'spec/models/concerns/cache_markdown_field_spec.rb'
-    - 'spec/models/concerns/case_sensitivity_spec.rb'
-    - 'spec/models/concerns/featurable_spec.rb'
-    - 'spec/models/concerns/issuable_spec.rb'
-    - 'spec/models/concerns/mentionable_spec.rb'
-    - 'spec/models/concerns/milestoneable_spec.rb'
-    - 'spec/models/concerns/milestoneish_spec.rb'
-    - 'spec/models/concerns/routable_spec.rb'
-    - 'spec/models/concerns/subscribable_spec.rb'
-    - 'spec/models/concerns/token_authenticatable_spec.rb'
     - 'spec/models/container_repository_spec.rb'
     - 'spec/models/deploy_keys_project_spec.rb'
     - 'spec/models/deploy_token_spec.rb'

changelogs/unreleased/rails-save-bang-36.yml

+---
+title: Fixes Rails/SaveBang cop for spec files in spec/models/concerns/*
+merge_request: 42942
+author: Rajendra Kadam
+type: other

spec/models/concerns/avatarable_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe Avatarable do
       it 'validates the file size' do
         expect(validator).to receive(:validate_each).and_call_original
 
-        project.update(avatar: 'uploads/avatar.png')
+        project.update!(avatar: 'uploads/avatar.png')
       end
     end
 
@@ -29,7 +29,7 @@ RSpec.describe Avatarable do
       it 'skips validation of file size' do
         expect(validator).not_to receive(:validate_each)
 
-        project.update(name: 'Hello world')
+        project.update!(name: 'Hello world')
       end
     end
   end

spec/models/concerns/bulk_insertable_associations_spec.rb

@@ -187,7 +187,7 @@ RSpec.describe BulkInsertableAssociations do
         it 'invalidates the parent and returns false' do
           build_invalid_items(parent: parent)
 
-          expect(save_with_bulk_inserts(parent, bangify: false)).to be false
+          expect(BulkInsertableAssociations.with_bulk_insert { parent.save }).to be false # rubocop:disable Rails/SaveBang
           expect(parent.errors[:bulk_foos].size).to eq(1)
 
           expect(BulkFoo.count).to eq(0)
@@ -211,8 +211,8 @@ RSpec.describe BulkInsertableAssociations do
 
   private
 
-  def save_with_bulk_inserts(entity, bangify: true)
-    BulkInsertableAssociations.with_bulk_insert { bangify ? entity.save! : entity.save }
+  def save_with_bulk_inserts(entity)
+    BulkInsertableAssociations.with_bulk_insert { entity.save! }
   end
 
   def build_items(parent:, relation: :bulk_foos, count: 10)

spec/models/concerns/cache_markdown_field_spec.rb

@@ -285,7 +285,7 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
     it_behaves_like 'a class with cached markdown fields'
 
     describe '#attribute_invalidated?' do
-      let(:thing) { klass.create(description: markdown, description_html: html, cached_markdown_version: cache_version) }
+      let(:thing) { klass.create!(description: markdown, description_html: html, cached_markdown_version: cache_version) }
 
       it 'returns true when cached_markdown_version is different' do
         thing.cached_markdown_version += 1
@@ -318,7 +318,7 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
       let(:thing) do
         # This forces the record to have outdated HTML. We can't use `create` because the `before_create` hook
         # would re-render the HTML to the latest version
-        klass.create.tap do |thing|
+        klass.create!.tap do |thing|
           thing.update_columns(description: markdown, description_html: old_html, cached_markdown_version: old_version)
         end
       end
@@ -326,7 +326,7 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
       it 'correctly updates cached HTML even if refresh_markdown_cache is called before updating the attribute' do
         thing.refresh_markdown_cache
 
-        thing.update(description: updated_markdown)
+        thing.update!(description: updated_markdown)
 
         expect(thing.description_html).to eq(updated_html)
       end

spec/models/concerns/case_sensitivity_spec.rb

@@ -12,8 +12,8 @@ RSpec.describe CaseSensitivity do
       end
     end
 
-    let!(:model_1) { model.create(path: 'mOdEl-1', name: 'mOdEl 1') }
-    let!(:model_2) { model.create(path: 'mOdEl-2', name: 'mOdEl 2') }
+    let!(:model_1) { model.create!(path: 'mOdEl-1', name: 'mOdEl 1') }
+    let!(:model_2) { model.create!(path: 'mOdEl-2', name: 'mOdEl 2') }
 
     it 'finds a single instance by a single attribute regardless of case' do
       expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1)

spec/models/concerns/featurable_spec.rb

@@ -180,6 +180,6 @@ RSpec.describe Featurable do
 
   def update_all_project_features(project, features, value)
     project_feature_attributes = features.map { |f| ["#{f}_access_level", value] }.to_h
-    project.project_feature.update(project_feature_attributes)
+    project.project_feature.update!(project_feature_attributes)
   end
 end

spec/models/concerns/issuable_spec.rb

@@ -69,7 +69,7 @@ RSpec.describe Issuable do
 
     it 'returns nil when author is nil' do
       issue.author_id = nil
-      issue.save(validate: false)
+      issue.save!(validate: false)
 
       expect(issue.author_name).to eq nil
     end
@@ -361,13 +361,13 @@ RSpec.describe Issuable do
       end
 
       it 'returns true when a subcription exists and subscribed is true' do
-        issue.subscriptions.create(user: user, project: project, subscribed: true)
+        issue.subscriptions.create!(user: user, project: project, subscribed: true)
 
         expect(issue.subscribed?(user, project)).to be_truthy
       end
 
       it 'returns false when a subcription exists and subscribed is false' do
-        issue.subscriptions.create(user: user, project: project, subscribed: false)
+        issue.subscriptions.create!(user: user, project: project, subscribed: false)
 
         expect(issue.subscribed?(user, project)).to be_falsey
       end
@@ -383,13 +383,13 @@ RSpec.describe Issuable do
       end
 
       it 'returns true when a subcription exists and subscribed is true' do
-        issue.subscriptions.create(user: user, project: project, subscribed: true)
+        issue.subscriptions.create!(user: user, project: project, subscribed: true)
 
         expect(issue.subscribed?(user, project)).to be_truthy
       end
 
       it 'returns false when a subcription exists and subscribed is false' do
-        issue.subscriptions.create(user: user, project: project, subscribed: false)
+        issue.subscriptions.create!(user: user, project: project, subscribed: false)
 
         expect(issue.subscribed?(user, project)).to be_falsey
       end
@@ -437,7 +437,7 @@ RSpec.describe Issuable do
       let(:labels) { create_list(:label, 2) }
 
       before do
-        issue.update(labels: [labels[1]])
+        issue.update!(labels: [labels[1]])
         expect(Gitlab::HookData::IssuableBuilder)
           .to receive(:new).with(issue).and_return(builder)
       end
@@ -456,7 +456,7 @@ RSpec.describe Issuable do
     context 'total_time_spent is updated' do
       before do
         issue.spend_time(duration: 2, user_id: user.id, spent_at: Time.current)
-        issue.save
+        issue.save!
         expect(Gitlab::HookData::IssuableBuilder)
           .to receive(:new).with(issue).and_return(builder)
       end
@@ -497,8 +497,8 @@ RSpec.describe Issuable do
       let(:user2) { create(:user) }
 
       before do
-        merge_request.update(assignees: [user])
-        merge_request.update(assignees: [user, user2])
+        merge_request.update!(assignees: [user])
+        merge_request.update!(assignees: [user, user2])
         expect(Gitlab::HookData::IssuableBuilder)
           .to receive(:new).with(merge_request).and_return(builder)
       end
@@ -554,7 +554,7 @@ RSpec.describe Issuable do
       before do
         label_link = issue.label_links.find_by(label_id: second_label.id)
         label_link.label_id = nil
-        label_link.save(validate: false)
+        label_link.save!(validate: false)
       end
 
       it 'filters out bad labels' do

spec/models/concerns/mentionable_spec.rb

@@ -177,7 +177,7 @@ RSpec.describe Issue, "Mentionable" do
 
         expect(SystemNoteService).not_to receive(:cross_reference)
 
-        issue.update(description: 'New description')
+        issue.update!(description: 'New description')
         issue.create_new_cross_references!
       end
 
@@ -186,7 +186,7 @@ RSpec.describe Issue, "Mentionable" do
 
         expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args)
 
-        issue.update(description: issues[1].to_reference)
+        issue.update!(description: issues[1].to_reference)
         issue.create_new_cross_references!
       end
 
@@ -196,7 +196,7 @@ RSpec.describe Issue, "Mentionable" do
 
         expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args)
 
-        note.update(note: issues[1].to_reference)
+        note.update!(note: issues[1].to_reference)
         note.create_new_cross_references!
       end
     end

spec/models/concerns/milestoneable_spec.rb

@@ -61,7 +61,7 @@ RSpec.describe Milestoneable do
 
       it 'returns true with a milestone from the the parent of the issue project group' do
         parent = create(:group)
-        group.update(parent: parent)
+        group.update!(parent: parent)
         milestone = create(:milestone, group: parent)
 
         expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy

spec/models/concerns/milestoneish_spec.rb

@@ -102,7 +102,7 @@ RSpec.describe Milestone, 'Milestoneish' do
 
       with_them do
         before do
-          project.update(visibility_level: project_visibility_levels[visibility])
+          project.update!(visibility_level: project_visibility_levels[visibility])
         end
 
         it 'returns the proper participants' do
@@ -139,7 +139,7 @@ RSpec.describe Milestone, 'Milestoneish' do
 
       with_them do
         before do
-          project.update(visibility_level: project_visibility_levels[visibility])
+          project.update!(visibility_level: project_visibility_levels[visibility])
         end
 
         it 'returns the proper participants' do
@@ -171,7 +171,7 @@ RSpec.describe Milestone, 'Milestoneish' do
 
     context 'when project is private' do
       before do
-        project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
       end
 
       it 'does not return any merge request for a non member' do
@@ -195,7 +195,7 @@ RSpec.describe Milestone, 'Milestoneish' do
 
       context 'when merge requests are available to project members' do
         before do
-          project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE)
+          project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
         end
 
         it 'does not return any merge request for a non member' do

spec/models/concerns/routable_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe Group, 'Routable' do
       end
 
       it 'updates route record on path change' do
-        group.update(path: 'wow', name: 'much')
+        group.update!(path: 'wow', name: 'much')
 
         expect(group.route.path).to eq('wow')
         expect(group.route.name).to eq('much')

spec/models/concerns/subscribable_spec.rb

@@ -20,13 +20,13 @@ RSpec.describe Subscribable, 'Subscribable' do
       end
 
       it 'returns true when a subcription exists and subscribed is true' do
-        resource.subscriptions.create(user: user_1, subscribed: true)
+        resource.subscriptions.create!(user: user_1, subscribed: true)
 
         expect(resource.subscribed?(user_1)).to be_truthy
       end
 
       it 'returns false when a subcription exists and subscribed is false' do
-        resource.subscriptions.create(user: user_1, subscribed: false)
+        resource.subscriptions.create!(user: user_1, subscribed: false)
 
         expect(resource.subscribed?(user_1)).to be_falsey
       end
@@ -38,13 +38,13 @@ RSpec.describe Subscribable, 'Subscribable' do
       end
 
       it 'returns true when a subcription exists and subscribed is true' do
-        resource.subscriptions.create(user: user_1, project: project, subscribed: true)
+        resource.subscriptions.create!(user: user_1, project: project, subscribed: true)
 
         expect(resource.subscribed?(user_1, project)).to be_truthy
       end
 
       it 'returns false when a subcription exists and subscribed is false' do
-        resource.subscriptions.create(user: user_1, project: project, subscribed: false)
+        resource.subscriptions.create!(user: user_1, project: project, subscribed: false)
 
         expect(resource.subscribed?(user_1, project)).to be_falsey
       end
@@ -58,9 +58,9 @@ RSpec.describe Subscribable, 'Subscribable' do
 
     it 'returns the subscribed users' do
       user_2 = create(:user)
-      resource.subscriptions.create(user: user_1, subscribed: true)
-      resource.subscriptions.create(user: user_2, project: project, subscribed: true)
-      resource.subscriptions.create(user: create(:user), project: project, subscribed: false)
+      resource.subscriptions.create!(user: user_1, subscribed: true)
+      resource.subscriptions.create!(user: user_2, project: project, subscribed: true)
+      resource.subscriptions.create!(user: create(:user), project: project, subscribed: false)
 
       expect(resource.subscribers(project)).to contain_exactly(user_1, user_2)
     end
@@ -113,7 +113,7 @@ RSpec.describe Subscribable, 'Subscribable' do
   describe '#unsubscribe' do
     context 'without project' do
       it 'unsubscribes the given current user' do
-        resource.subscriptions.create(user: user_1, subscribed: true)
+        resource.subscriptions.create!(user: user_1, subscribed: true)
         expect(resource.subscribed?(user_1)).to be_truthy
 
         resource.unsubscribe(user_1)
@@ -124,7 +124,7 @@ RSpec.describe Subscribable, 'Subscribable' do
 
     context 'with project' do
       it 'unsubscribes the given current user' do
-        resource.subscriptions.create(user: user_1, project: project, subscribed: true)
+        resource.subscriptions.create!(user: user_1, project: project, subscribed: true)
         expect(resource.subscribed?(user_1, project)).to be_truthy
 
         resource.unsubscribe(user_1, project)
@@ -139,7 +139,7 @@ RSpec.describe Subscribable, 'Subscribable' do
       context 'when desired_state is set to true' do
         context 'when a user is subscribed to the resource' do
           it 'keeps the user subscribed' do
-            resource.subscriptions.create(user: user_1, subscribed: true, project: resource_project)
+            resource.subscriptions.create!(user: user_1, subscribed: true, project: resource_project)
 
             resource.set_subscription(user_1, true, resource_project)
 
@@ -159,7 +159,7 @@ RSpec.describe Subscribable, 'Subscribable' do
       context 'when desired_state is set to false' do
         context 'when a user is subscribed to the resource' do
           it 'unsubscribes the user from the resource' do
-            resource.subscriptions.create(user: user_1, subscribed: true, project: resource_project)
+            resource.subscriptions.create!(user: user_1, subscribed: true, project: resource_project)
 
             expect { resource.set_subscription(user_1, false, resource_project) }
               .to change { resource.subscribed?(user_1, resource_project) }

spec/models/concerns/token_authenticatable_spec.rb

@@ -137,7 +137,7 @@ RSpec.describe PersonalAccessToken, 'TokenAuthenticatable' do
     subject { PersonalAccessToken.find_by_token(token_value) }
 
     it 'finds the token' do
-      personal_access_token.save
+      personal_access_token.save!
 
       expect(subject).to eq(personal_access_token)
     end