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

Fix Rails/SaveBang offenses for */spec/models/ci/*

See merge request gitlab-org/gitlab!41329

.rubocop_todo.yml

@@ -702,7 +702,6 @@ Rails/SaveBang:
     - '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/build_spec.rb'
     - 'ee/spec/models/ci/pipeline_spec.rb'
     - 'ee/spec/models/ci/subscriptions/project_spec.rb'
     - 'ee/spec/models/concerns/approver_migrate_hook_spec.rb'
@@ -1048,14 +1047,6 @@ Rails/SaveBang:
     - 'spec/models/appearance_spec.rb'
     - 'spec/models/application_record_spec.rb'
     - 'spec/models/application_setting_spec.rb'
-    - 'spec/models/ci/build_metadata_spec.rb'
-    - 'spec/models/ci/build_spec.rb'
-    - 'spec/models/ci/build_trace_chunk_spec.rb'
-    - 'spec/models/ci/instance_variable_spec.rb'
-    - 'spec/models/ci/legacy_stage_spec.rb'
-    - 'spec/models/ci/persistent_ref_spec.rb'
-    - 'spec/models/ci/pipeline_spec.rb'
-    - 'spec/models/ci/runner_spec.rb'
     - 'spec/models/clusters/applications/helm_spec.rb'
     - 'spec/models/commit_spec.rb'
     - 'spec/models/commit_status_spec.rb'

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

+---
+title: Fix Rails/SaveBang offenses for */spec/models/ci/*
+merge_request: 41329
+author: Rajendra Kadam
+type: other

ee/spec/models/ci/build_spec.rb

@@ -105,7 +105,7 @@ RSpec.describe Ci::Build do
       expect(Gitlab::Database::LoadBalancing::Sticking).to receive(:stick)
         .with(:build, job.id)
 
-      job.update(status: :running)
+      job.update!(status: :running)
     end
   end
 
@@ -118,7 +118,7 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        job.update(environment: 'staging')
+        job.update!(environment: 'staging')
         create(:environment, name: 'staging', project: job.project)
 
         variable =

spec/models/ci/build_metadata_spec.rb

@@ -73,7 +73,7 @@ RSpec.describe Ci::BuildMetadata do
 
       context 'when both runner and job timeouts are set' do
         before do
-          build.update(runner: runner)
+          build.update!(runner: runner)
         end
 
         context 'when job timeout is higher than runner timeout' do

spec/models/ci/build_spec.rb

@@ -566,18 +566,18 @@ RSpec.describe Ci::Build do
       let(:runner) { create(:ci_runner, :project, projects: [build.project]) }
 
       before do
-        runner.update(contacted_at: 1.second.ago)
+        runner.update!(contacted_at: 1.second.ago)
       end
 
       it { is_expected.to be_truthy }
 
       it 'that is inactive' do
-        runner.update(active: false)
+        runner.update!(active: false)
         is_expected.to be_falsey
       end
 
       it 'that is not online' do
-        runner.update(contacted_at: nil)
+        runner.update!(contacted_at: nil)
         is_expected.to be_falsey
       end
 
@@ -723,7 +723,7 @@ RSpec.describe Ci::Build do
 
     context 'is expired' do
       before do
-        build.update(artifacts_expire_at: Time.current - 7.days)
+        build.update!(artifacts_expire_at: Time.current - 7.days)
       end
 
       it { is_expected.to be_truthy }
@@ -731,7 +731,7 @@ RSpec.describe Ci::Build do
 
     context 'is not expired' do
       before do
-        build.update(artifacts_expire_at: Time.current + 7.days)
+        build.update!(artifacts_expire_at: Time.current + 7.days)
       end
 
       it { is_expected.to be_falsey }
@@ -1240,7 +1240,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is defined' do
         before do
-          build.update(environment: 'review')
+          build.update!(environment: 'review')
         end
 
         it { is_expected.to be_truthy }
@@ -1248,7 +1248,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is not defined' do
         before do
-          build.update(environment: nil)
+          build.update!(environment: nil)
         end
 
         it { is_expected.to be_falsey }
@@ -1356,7 +1356,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is defined' do
         before do
-          build.update(environment: 'review')
+          build.update!(environment: 'review')
         end
 
         context 'no action is defined' do
@@ -1365,7 +1365,7 @@ RSpec.describe Ci::Build do
 
         context 'and start action is defined' do
           before do
-            build.update(options: { environment: { action: 'start' } } )
+            build.update!(options: { environment: { action: 'start' } } )
           end
 
           it { is_expected.to be_truthy }
@@ -1374,7 +1374,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is not defined' do
         before do
-          build.update(environment: nil)
+          build.update!(environment: nil)
         end
 
         it { is_expected.to be_falsey }
@@ -1386,7 +1386,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is defined' do
         before do
-          build.update(environment: 'review')
+          build.update!(environment: 'review')
         end
 
         context 'no action is defined' do
@@ -1395,7 +1395,7 @@ RSpec.describe Ci::Build do
 
         context 'and stop action is defined' do
           before do
-            build.update(options: { environment: { action: 'stop' } } )
+            build.update!(options: { environment: { action: 'stop' } } )
           end
 
           it { is_expected.to be_truthy }
@@ -1404,7 +1404,7 @@ RSpec.describe Ci::Build do
 
       context 'when environment is not defined' do
         before do
-          build.update(environment: nil)
+          build.update!(environment: nil)
         end
 
         it { is_expected.to be_falsey }
@@ -1727,7 +1727,7 @@ RSpec.describe Ci::Build do
 
     describe '#action?' do
       before do
-        build.update(when: value)
+        build.update!(when: value)
       end
 
       subject { build.action? }
@@ -2272,7 +2272,7 @@ RSpec.describe Ci::Build do
   describe '#has_expiring_archive_artifacts?' do
     context 'when artifacts have expiration date set' do
       before do
-        build.update(artifacts_expire_at: 1.day.from_now)
+        build.update!(artifacts_expire_at: 1.day.from_now)
       end
 
       context 'and job artifacts archive record exists' do
@@ -2292,7 +2292,7 @@ RSpec.describe Ci::Build do
 
     context 'when artifacts do not have expiration date set' do
       before do
-        build.update(artifacts_expire_at: nil)
+        build.update!(artifacts_expire_at: nil)
       end
 
       it 'does not have expiring artifacts' do
@@ -2567,7 +2567,7 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        build.update(user: user)
+        build.update!(user: user)
       end
 
       it { user_variables.each { |v| is_expected.to include(v) } }
@@ -2603,7 +2603,7 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        build.update(environment: 'production')
+        build.update!(environment: 'production')
       end
 
       shared_examples 'containing environment variables' do
@@ -2630,7 +2630,7 @@ RSpec.describe Ci::Build do
 
         context 'when the URL was set from the job' do
           before do
-            build.update(options: { environment: { url: url } })
+            build.update!(options: { environment: { url: url } })
           end
 
           it_behaves_like 'containing environment variables'
@@ -2648,7 +2648,7 @@ RSpec.describe Ci::Build do
 
         context 'when the URL was not set from the job, but environment' do
           before do
-            environment.update(external_url: url)
+            environment.update!(external_url: url)
           end
 
           it_behaves_like 'containing environment variables'
@@ -2684,7 +2684,7 @@ RSpec.describe Ci::Build do
 
     context 'when build started manually' do
       before do
-        build.update(when: :manual)
+        build.update!(when: :manual)
       end
 
       let(:manual_variable) do
@@ -2710,8 +2710,8 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        build.update(tag: false)
-        pipeline.update(tag: false)
+        build.update!(tag: false)
+        pipeline.update!(tag: false)
       end
 
       it { is_expected.to include(branch_variable) }
@@ -2723,8 +2723,8 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        build.update(tag: true)
-        pipeline.update(tag: true)
+        build.update!(tag: true)
+        pipeline.update!(tag: true)
       end
 
       it { is_expected.to include(tag_variable) }
@@ -2901,7 +2901,7 @@ RSpec.describe Ci::Build do
 
       context 'and is disabled for project' do
         before do
-          project.update(container_registry_enabled: false)
+          project.update!(container_registry_enabled: false)
         end
 
         it { is_expected.to include(ci_registry) }
@@ -2910,7 +2910,7 @@ RSpec.describe Ci::Build do
 
       context 'and is enabled for project' do
         before do
-          project.update(container_registry_enabled: true)
+          project.update!(container_registry_enabled: true)
         end
 
         it { is_expected.to include(ci_registry) }
@@ -2922,7 +2922,7 @@ RSpec.describe Ci::Build do
       let(:runner) { create(:ci_runner, description: 'description', tag_list: %w(docker linux)) }
 
       before do
-        build.update(runner: runner)
+        build.update!(runner: runner)
       end
 
       it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true, masked: false }) }
@@ -3726,7 +3726,7 @@ RSpec.describe Ci::Build do
     subject { described_class.where(id: build).matches_tag_ids(tag_ids) }
 
     before do
-      build.update(tag_list: build_tag_list)
+      build.update!(tag_list: build_tag_list)
     end
 
     context 'when have different tags' do
@@ -3772,7 +3772,7 @@ RSpec.describe Ci::Build do
     subject { described_class.where(id: build).with_any_tags }
 
     before do
-      build.update(tag_list: tag_list)
+      build.update!(tag_list: tag_list)
     end
 
     context 'when does have tags' do

spec/models/ci/build_trace_chunk_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
         expect(external_data_counter).to be > 0
         expect(subjects.count).to be > 0
 
-        expect { subjects.first.destroy }.to raise_error('`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`')
+        expect { subjects.first.destroy! }.to raise_error('`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`')
         expect { subjects.destroy_all }.to raise_error('`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`') # rubocop: disable Cop/DestroyAll
 
         expect(subjects.count).to be > 0
@@ -57,7 +57,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
         expect(external_data_counter).to be > 0
         expect(subjects.count).to be > 0
 
-        expect { parent.destroy }.not_to raise_error
+        expect { parent.destroy! }.not_to raise_error
 
         expect(subjects.count).to eq(0)
         expect(external_data_counter).to eq(0)

spec/models/ci/instance_variable_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Ci::InstanceVariable do
         let(:value) { SecureRandom.alphanumeric(10_002) }
 
         it 'raises a database level error' do
-          expect { variable.save }.to raise_error(ActiveRecord::StatementInvalid)
+          expect { variable.save! }.to raise_error(ActiveRecord::StatementInvalid)
         end
       end
 
@@ -36,7 +36,7 @@ RSpec.describe Ci::InstanceVariable do
         let(:value) { SecureRandom.alphanumeric(10_000) }
 
         it 'does not raise database level error' do
-          expect { variable.save }.not_to raise_error
+          expect { variable.save! }.not_to raise_error
         end
       end
     end
@@ -85,7 +85,7 @@ RSpec.describe Ci::InstanceVariable do
     it 'resets the cache when records are deleted' do
       expect(described_class.all_cached).to contain_exactly(protected_variable, unprotected_variable)
 
-      protected_variable.destroy
+      protected_variable.destroy!
 
       expect(described_class.all_cached).to contain_exactly(unprotected_variable)
     end

spec/models/ci/legacy_stage_spec.rb

@@ -116,7 +116,7 @@ RSpec.describe Ci::LegacyStage do
         let!(:new_build) { create_job(:ci_build, status: :success) }
 
         before do
-          stage_build.update(retried: true)
+          stage_build.update!(retried: true)
         end
 
         it "returns status of latest build" do

spec/models/ci/persistent_ref_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Ci::PersistentRef do
 
     context 'when a persistent ref exists' do
       before do
-        pipeline.persistent_ref.create
+        pipeline.persistent_ref.create # rubocop: disable Rails/SaveBang
       end
 
       it { is_expected.to eq(true) }
@@ -32,7 +32,7 @@ RSpec.describe Ci::PersistentRef do
   end
 
   describe '#create' do
-    subject { pipeline.persistent_ref.create }
+    subject { pipeline.persistent_ref.create } # rubocop: disable Rails/SaveBang
 
     let(:pipeline) { create(:ci_pipeline, sha: sha, project: project) }
     let(:project) { create(:project, :repository) }
@@ -58,7 +58,7 @@ RSpec.describe Ci::PersistentRef do
 
     context 'when a persistent ref already exists' do
       before do
-        pipeline.persistent_ref.create
+        pipeline.persistent_ref.create # rubocop: disable Rails/SaveBang
       end
 
       it 'overwrites a persistent ref' do
@@ -78,7 +78,7 @@ RSpec.describe Ci::PersistentRef do
 
     context 'when a persistent ref exists' do
       before do
-        pipeline.persistent_ref.create
+        pipeline.persistent_ref.create # rubocop: disable Rails/SaveBang
       end
 
       it 'deletes the ref' do

spec/models/ci/pipeline_spec.rb

@@ -901,7 +901,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
 
       context 'when there is auto_canceled_by' do
         before do
-          pipeline.update(auto_canceled_by: create(:ci_empty_pipeline))
+          pipeline.update!(auto_canceled_by: create(:ci_empty_pipeline))
         end
 
         it 'is auto canceled' do
@@ -1571,7 +1571,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
         it 'looks up a commit for a tag' do
           expect(project.repository.branch_names).not_to include 'v1.0.0'
 
-          pipeline.update(sha: project.commit('v1.0.0').sha, ref: 'v1.0.0', tag: true)
+          pipeline.update!(sha: project.commit('v1.0.0').sha, ref: 'v1.0.0', tag: true)
 
           expect(pipeline).to be_tag
           expect(pipeline).to be_latest
@@ -1580,7 +1580,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
 
       context 'with not latest sha' do
         before do
-          pipeline.update(sha: project.commit("#{project.default_branch}~1").sha)
+          pipeline.update!(sha: project.commit("#{project.default_branch}~1").sha)
         end
 
         it 'returns false' do
@@ -1608,7 +1608,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
         let!(:manual2) { create(:ci_build, :manual, pipeline: pipeline, name: 'deploy') }
 
         before do
-          manual.update(retried: true)
+          manual.update!(retried: true)
         end
 
         it 'returns latest one' do
@@ -1644,7 +1644,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   describe '#modified_paths' do
     context 'when old and new revisions are set' do
       before do
-        pipeline.update(before_sha: '1234abcd', sha: '2345bcde')
+        pipeline.update!(before_sha: '1234abcd', sha: '2345bcde')
       end
 
       it 'fetches stats for changes between commits' do
@@ -2740,7 +2740,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       project.add_developer(pipeline.user)
 
       pipeline.user.global_notification_setting
-        .update(level: 'custom', failed_pipeline: true, success_pipeline: true)
+        .update!(level: 'custom', failed_pipeline: true, success_pipeline: true)
 
       perform_enqueued_jobs do
         pipeline.enqueue

spec/models/ci/runner_spec.rb

@@ -570,7 +570,7 @@ RSpec.describe Ci::Runner do
       let!(:last_update) { runner.ensure_runner_queue_value }
 
       before do
-        Ci::UpdateRunnerService.new(runner).update(description: 'new runner')
+        Ci::UpdateRunnerService.new(runner).update(description: 'new runner') # rubocop: disable Rails/SaveBang
       end
 
       it 'sets a new last_update value' do
@@ -660,7 +660,7 @@ RSpec.describe Ci::Runner do
 
       before do
         runner.tick_runner_queue
-        runner.destroy
+        runner.destroy!
       end
 
       it 'cleans up the queue' do
@@ -878,7 +878,7 @@ RSpec.describe Ci::Runner do
 
     it 'can be destroyed' do
       subject
-      expect { subject.destroy }.to change { described_class.count }.by(-1)
+      expect { subject.destroy! }.to change { described_class.count }.by(-1)
     end
   end