diff options
25 files changed, 207 insertions, 147 deletions
diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index f410daa09e2..d7fb09cf24a 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -164,10 +164,6 @@ Rails/SaveBang: - 'spec/models/group_spec.rb' - 'spec/models/identity_spec.rb' - 'spec/models/jira_import_state_spec.rb' - - 'spec/models/key_spec.rb' - - 'spec/models/lfs_objects_project_spec.rb' - - 'spec/models/merge_request_spec.rb' - - 'spec/models/milestone_spec.rb' - 'spec/models/namespace_spec.rb' - 'spec/models/note_spec.rb' - 'spec/models/notification_setting_spec.rb' diff --git a/Gemfile.lock b/Gemfile.lock index 6b0c9222858..2d13bfb68c1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -930,7 +930,7 @@ GEM pry (~> 0.13.0) pry-rails (0.3.9) pry (>= 0.10.4) - pry-shell (0.4.0) + pry-shell (0.4.1) pry (~> 0.13.0) tty-markdown tty-prompt diff --git a/app/models/ci/bridge.rb b/app/models/ci/bridge.rb index 352229c64da..577bca282ef 100644 --- a/app/models/ci/bridge.rb +++ b/app/models/ci/bridge.rb @@ -163,7 +163,7 @@ module Ci def expanded_environment_name end - def instantized_environment + def persisted_environment end def execute_hooks diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 556c3d6b944..a26f28c3006 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -90,16 +90,6 @@ module Ci end end - # Initializing an object instead of fetching `persisted_environment` for avoiding unnecessary queries. - # We're planning to introduce a direct relationship between build and environment - # in https://gitlab.com/gitlab-org/gitlab/-/issues/326445 to let us to preload - # in batch. - def instantized_environment - return unless has_environment? - - ::Environment.new(project: self.project, name: self.expanded_environment_name) - end - serialize :options # rubocop:disable Cop/ActiveRecordSerialize serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiveRecordSerialize diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index f0a2c074584..1c6e6a07c9e 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -660,15 +660,9 @@ module Ci # Return a hash of file type => array of 1 job artifact def latest_report_artifacts ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do - # Note we use read_attribute(:project_id) to read the project - # ID instead of self.project_id. The latter appears to load - # the Project model. This extra filter doesn't appear to - # affect query plan but included to ensure we don't leak the - # wrong informaiton. ::Ci::JobArtifact.where( id: job_artifacts.with_reports .select('max(ci_job_artifacts.id) as id') - .where(project_id: self.read_attribute(:project_id)) .group(:file_type) ) .preload(:job) diff --git a/app/models/ci/processable.rb b/app/models/ci/processable.rb index 15c57550159..e2f257eab25 100644 --- a/app/models/ci/processable.rb +++ b/app/models/ci/processable.rb @@ -120,7 +120,7 @@ module Ci raise NotImplementedError end - def instantized_environment + def persisted_environment raise NotImplementedError end diff --git a/app/models/label.rb b/app/models/label.rb index a46d6bc5c0f..1a07620f944 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -9,6 +9,10 @@ class Label < ApplicationRecord include Sortable include FromUnion include Presentable + include IgnorableColumns + + # TODO: Project#create_labels can remove column exception when this column is dropped from all envs + ignore_column :remove_on_close, remove_with: '14.1', remove_after: '2021-06-22' cache_markdown_field :description, pipeline: :single_line diff --git a/app/models/project.rb b/app/models/project.rb index dc6e341d427..7abd379ebb8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1420,7 +1420,8 @@ class Project < ApplicationRecord # rubocop: disable CodeReuse/ServiceClass def create_labels Label.templates.each do |label| - params = label.attributes.except('id', 'template', 'created_at', 'updated_at', 'type') + # TODO: remove_on_close exception can be removed after the column is dropped from all envs + params = label.attributes.except('id', 'template', 'created_at', 'updated_at', 'type', 'remove_on_close') Labels::FindOrCreateService.new(nil, self, params).execute(skip_authorization: true) end end diff --git a/app/services/base_container_service.rb b/app/services/base_container_service.rb index ee15763ce65..8492b3ce92c 100644 --- a/app/services/base_container_service.rb +++ b/app/services/base_container_service.rb @@ -18,4 +18,12 @@ class BaseContainerService @current_user = current_user @params = params.dup end + + def project_container? + container.is_a?(::Project) + end + + def group_container? + container.is_a?(::Group) + end end diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb index 649cf281ab0..842a4a351fe 100644 --- a/app/services/users/build_service.rb +++ b/app/services/users/build_service.rb @@ -5,7 +5,6 @@ module Users delegate :user_default_internal_regex_enabled?, :user_default_internal_regex_instance, to: :'Gitlab::CurrentSettings.current_application_settings' - attr_reader :identity_params def initialize(current_user, params = {}) @current_user = current_user @@ -16,43 +15,135 @@ module Users def execute(skip_authorization: false) @skip_authorization = skip_authorization - raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_create_user? + build_user + build_identity + update_canonical_email - user_params = build_user_params - user = User.new(user_params) + user + end + + private + + attr_reader :skip_authorization, :identity_params, :user_params, :user - if current_user&.admin? - @reset_token = user.generate_reset_token if params[:reset_password] + def identity_attributes + [:extern_uid, :provider] + end - if user_params[:force_random_password] - random_password = User.random_password - user.password = user.password_confirmation = random_password - end + def build_user + if admin? + admin_build_user + else + standard_build_user end + end - build_identity(user) + def admin? + return false unless current_user - Users::UpdateCanonicalEmailService.new(user: user).execute + current_user.admin? + end - user + def admin_build_user + build_user_params_for_admin + init_user + password_reset end - private + def standard_build_user + # current_user non admin or nil + validate_access! + build_user_params_for_non_admin + init_user + end - attr_reader :skip_authorization + def build_user_params_for_admin + @user_params = params.slice(*admin_create_params) + @user_params.merge!(force_random_password: true, password_expires_at: nil) if params[:reset_password] + end - def identity_attributes - [:extern_uid, :provider] + def init_user + assign_common_user_params + + @user = User.new(user_params) + end + + def assign_common_user_params + @user_params[:created_by_id] = current_user&.id + @user_params[:external] = user_external? if set_external_param? + + @user_params.delete(:user_type) unless project_bot? + end + + def set_external_param? + user_default_internal_regex_enabled? && !user_params.key?(:external) end - def build_identity(user) + def user_external? + user_default_internal_regex_instance.match(params[:email]).nil? + end + + def project_bot? + user_params[:user_type]&.to_sym == :project_bot + end + + def password_reset + @reset_token = user.generate_reset_token if params[:reset_password] + + if user_params[:force_random_password] + random_password = User.random_password + @user.password = user.password_confirmation = random_password + end + end + + def validate_access! + return if skip_authorization + return if can_create_user? + + raise Gitlab::Access::AccessDeniedError + end + + def can_create_user? + current_user.nil? && Gitlab::CurrentSettings.allow_signup? + end + + def build_user_params_for_non_admin + allowed_signup_params = signup_params + allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation? + + @user_params = params.slice(*allowed_signup_params) + @user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting if assign_skip_confirmation_from_settings? + @user_params[:name] = fallback_name if use_fallback_name? + end + + def allow_caller_to_request_skip_confirmation? + skip_authorization + end + + def assign_skip_confirmation_from_settings? + user_params[:skip_confirmation].nil? + end + + def skip_user_confirmation_email_from_setting + !Gitlab::CurrentSettings.send_user_confirmation_email + end + + def use_fallback_name? + user_params[:name].blank? && fallback_name.present? + end + + def fallback_name + "#{user_params[:first_name]} #{user_params[:last_name]}" + end + + def build_identity return if identity_params.empty? user.identities.build(identity_params) end - def can_create_user? - (current_user.nil? && Gitlab::CurrentSettings.allow_signup?) || current_user&.admin? + def update_canonical_email + Users::UpdateCanonicalEmailService.new(user: user).execute end # Allowed params for creating a user (admins only) @@ -96,69 +187,15 @@ module Users def signup_params [ :email, - :password_automatically_set, :name, - :first_name, - :last_name, :password, + :password_automatically_set, :username, - :user_type + :user_type, + :first_name, + :last_name ] end - - def build_user_params - if current_user&.admin? - user_params = params.slice(*admin_create_params) - - if params[:reset_password] - user_params.merge!(force_random_password: true, password_expires_at: nil) - end - else - allowed_signup_params = signup_params - allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation? - - user_params = params.slice(*allowed_signup_params) - if assign_skip_confirmation_from_settings?(user_params) - user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting - end - - fallback_name = "#{user_params[:first_name]} #{user_params[:last_name]}" - - if user_params[:name].blank? && fallback_name.present? - user_params = user_params.merge(name: fallback_name) - end - end - - user_params[:created_by_id] = current_user&.id - - if user_default_internal_regex_enabled? && !user_params.key?(:external) - user_params[:external] = user_external? - end - - user_params.delete(:user_type) unless project_bot?(user_params[:user_type]) - - user_params - end - - def allow_caller_to_request_skip_confirmation? - skip_authorization - end - - def assign_skip_confirmation_from_settings?(user_params) - user_params[:skip_confirmation].nil? - end - - def skip_user_confirmation_email_from_setting - !Gitlab::CurrentSettings.send_user_confirmation_email - end - - def user_external? - user_default_internal_regex_instance.match(params[:email]).nil? - end - - def project_bot?(user_type) - user_type&.to_sym == :project_bot - end end end diff --git a/app/services/users/registrations_build_service.rb b/app/services/users/registrations_build_service.rb index 9d7bf0a7e18..6ed60840bb8 100644 --- a/app/services/users/registrations_build_service.rb +++ b/app/services/users/registrations_build_service.rb @@ -12,7 +12,7 @@ module Users end override :assign_skip_confirmation_from_settings? - def assign_skip_confirmation_from_settings?(user_params) + def assign_skip_confirmation_from_settings? user_params[:skip_confirmation].blank? end end diff --git a/changelogs/unreleased/issue-220040-fix-robocop-savebang-spec-models-5.yml b/changelogs/unreleased/issue-220040-fix-robocop-savebang-spec-models-5.yml new file mode 100644 index 00000000000..612a0d9524a --- /dev/null +++ b/changelogs/unreleased/issue-220040-fix-robocop-savebang-spec-models-5.yml @@ -0,0 +1,5 @@ +--- +title: Fixed Rails Save Bang offenses in few spec/models/* files +merge_request: 62165 +author: Suraj Tripathi @surajtripathy07 +type: fixed diff --git a/changelogs/unreleased/sh-optimize-artifact-loading-mr.yml b/changelogs/unreleased/sh-optimize-artifact-loading-mr.yml new file mode 100644 index 00000000000..9961fcc42d1 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-artifact-loading-mr.yml @@ -0,0 +1,5 @@ +--- +title: Optimize query for loading artifacts in pipeline +merge_request: 62249 +author: +type: performance diff --git a/changelogs/unreleased/update_pry_debugging_docs.yml b/changelogs/unreleased/update_pry_debugging_docs.yml new file mode 100644 index 00000000000..44dcbf69426 --- /dev/null +++ b/changelogs/unreleased/update_pry_debugging_docs.yml @@ -0,0 +1,5 @@ +--- +title: Fix `pry` debugging location with `pry-byebug` and `pry-shell` by updating the `pry-shell` gem +merge_request: 62122 +author: +type: fixed diff --git a/doc/development/pry_debugging.md b/doc/development/pry_debugging.md index d662e6bbc54..402029164a7 100644 --- a/doc/development/pry_debugging.md +++ b/doc/development/pry_debugging.md @@ -12,8 +12,10 @@ To invoke the debugger, place `binding.pry` somewhere in your code. When the Ruby interpreter hits that code, execution stops, and you can type in commands to debug the state of the program. -When debugging code in another process like Puma or Sidekiq, you can use `binding.remote_pry`. -You can then connect to this session by running `pry-remote` from your terminal. +When debugging code in another process like Puma or Sidekiq, you can use `binding.pry_shell`. +You can then connect to this session by using the [pry-shell](https://github.com/meinac/pry-shell) executable. +You can watch [this video](https://www.youtube.com/watch?v=Lzs_PL_BySo), for more information about +how to use the `pry-shell`. ## `byebug` vs `binding.pry` diff --git a/doc/user/project/merge_requests/test_coverage_visualization.md b/doc/user/project/merge_requests/test_coverage_visualization.md index c25ee1a8a94..1b6f48c1d73 100644 --- a/doc/user/project/merge_requests/test_coverage_visualization.md +++ b/doc/user/project/merge_requests/test_coverage_visualization.md @@ -157,7 +157,7 @@ test-jdk11: coverage-jdk11: # Must be in a stage later than test-jdk11's stage. # The `visualize` stage does not exist by default. - # Please define it first, or chose an existing stage like `deploy`. + # Please define it first, or choose an existing stage like `deploy`. stage: visualize image: registry.gitlab.com/haynes/jacoco2cobertura:1.0.7 script: diff --git a/lib/gitlab/ci/pipeline/preloader.rb b/lib/gitlab/ci/pipeline/preloader.rb index 7befc126ca9..31ddf2c4241 100644 --- a/lib/gitlab/ci/pipeline/preloader.rb +++ b/lib/gitlab/ci/pipeline/preloader.rb @@ -20,6 +20,7 @@ module Gitlab preloader.preload_ref_commits preloader.preload_pipeline_warnings preloader.preload_stages_warnings + preloader.preload_persisted_environments end end end @@ -54,6 +55,13 @@ module Gitlab def preload_stages_warnings @pipeline.stages.each { |stage| stage.number_of_warnings } end + + # This batch loads the associated environments of multiple actions (builds) + # that can't use `preload` due to the indirect relationship. + def preload_persisted_environments + @pipeline.scheduled_actions.each { |action| action.persisted_environment } + @pipeline.manual_actions.each { |action| action.persisted_environment } + end end end end diff --git a/qa/qa/resource/project.rb b/qa/qa/resource/project.rb index 96a60381146..4b8e15387c3 100644 --- a/qa/qa/resource/project.rb +++ b/qa/qa/resource/project.rb @@ -260,9 +260,9 @@ module QA def runners(tag_list: nil) response = if tag_list - get Runtime::API::Request.new(api_client, "#{api_runners_path}?tag_list=#{tag_list.compact.join(',')}").url + get Runtime::API::Request.new(api_client, "#{api_runners_path}?tag_list=#{tag_list.compact.join(',')}", per_page: '100').url else - get Runtime::API::Request.new(api_client, "#{api_runners_path}").url + get Runtime::API::Request.new(api_client, "#{api_runners_path}", per_page: '100').url end parse_body(response) diff --git a/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb b/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb index 4fb321f60dd..3db5b9671d9 100644 --- a/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb +++ b/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb @@ -3,7 +3,7 @@ require 'securerandom' module QA - RSpec.describe 'Package', :orchestrated, :packages, :reliable do + RSpec.describe 'Package', :orchestrated, :packages do describe 'NuGet Repository' do include Runtime::Fixtures let(:project) do diff --git a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb index ae423fa04f9..5b644e42451 100644 --- a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb @@ -5,9 +5,11 @@ require 'spec_helper' RSpec.describe Gitlab::Ci::Pipeline::Preloader do let(:stage) { double(:stage) } let(:commit) { double(:commit) } + let(:scheduled_action) { double(:scheduled_action) } + let(:manual_action) { double(:manual_action) } let(:pipeline) do - double(:pipeline, commit: commit, stages: [stage]) + double(:pipeline, commit: commit, stages: [stage], scheduled_actions: [scheduled_action], manual_actions: [manual_action]) end describe '.preload!' do @@ -33,6 +35,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Preloader do expect(pipeline).to receive(:lazy_ref_commit) expect(pipeline).to receive(:number_of_warnings) expect(stage).to receive(:number_of_warnings) + expect(scheduled_action).to receive(:persisted_environment) + expect(manual_action).to receive(:persisted_environment) described_class.preload!([pipeline]) end @@ -42,6 +46,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Preloader do allow(pipeline).to receive(:lazy_ref_commit) allow(pipeline).to receive(:number_of_warnings) allow(stage).to receive(:number_of_warnings) + allow(scheduled_action).to receive(:persisted_environment) + allow(manual_action).to receive(:persisted_environment) pipelines = [pipeline, pipeline] diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 0cb20efcb0a..73d0f273504 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -139,7 +139,7 @@ RSpec.describe Key, :mailer do end with_them do - let!(:key) { create(factory) } + let!(:key) { create(factory) } # rubocop:disable Rails/SaveBang let!(:original_fingerprint) { key.fingerprint } let!(:original_fingerprint_sha256) { key.fingerprint_sha256 } @@ -224,7 +224,7 @@ RSpec.describe Key, :mailer do expect(AuthorizedKeysWorker).to receive(:perform_async).with(:remove_key, key.shell_id) - key.destroy + key.destroy! end end @@ -244,7 +244,7 @@ RSpec.describe Key, :mailer do expect(AuthorizedKeysWorker).not_to receive(:perform_async) - key.destroy + key.destroy! end end end diff --git a/spec/models/lfs_objects_project_spec.rb b/spec/models/lfs_objects_project_spec.rb index 71009a6f28f..df49b60c4fa 100644 --- a/spec/models/lfs_objects_project_spec.rb +++ b/spec/models/lfs_objects_project_spec.rb @@ -39,7 +39,7 @@ RSpec.describe LfsObjectsProject do expect(ProjectCacheWorker).to receive(:perform_async) .with(project.id, [], [:lfs_objects_size]) - subject.destroy + subject.destroy! end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a77ca1e9a51..fd05b56203d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -296,7 +296,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end it 'does not create duplicated metrics records when MR is concurrently updated' do - merge_request.metrics.destroy + merge_request.metrics.destroy! instance1 = MergeRequest.find(merge_request.id) instance2 = MergeRequest.find(merge_request.id) @@ -347,7 +347,7 @@ RSpec.describe MergeRequest, factory_default: :keep do let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } it 'returns empty requests' do - latest_merge_request_diff = merge_request.merge_request_diffs.create + latest_merge_request_diff = merge_request.merge_request_diffs.create! MergeRequestDiffCommit.where( merge_request_diff_id: latest_merge_request_diff, @@ -459,7 +459,7 @@ RSpec.describe MergeRequest, factory_default: :keep do } create(:merge_request, params).tap do |mr| - diffs.times { mr.merge_request_diffs.create } + diffs.times { mr.merge_request_diffs.create! } mr.create_merge_head_diff end end @@ -891,7 +891,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'when there are MR diffs' do it 'delegates to the MR diffs' do - merge_request.save + merge_request.save! expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options)).and_call_original @@ -1036,20 +1036,20 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'when there are MR diffs' do it 'returns the correct count' do - merge_request.save + merge_request.save! expect(merge_request.diff_size).to eq('105') end it 'returns the correct overflow count' do allow(Commit).to receive(:max_diff_options).and_return(max_files: 2) - merge_request.save + merge_request.save! expect(merge_request.diff_size).to eq('2+') end it 'does not perform highlighting' do - merge_request.save + merge_request.save! expect(Gitlab::Diff::Highlight).not_to receive(:new) @@ -1470,7 +1470,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end it "can't remove a root ref" do - subject.update(source_branch: 'master', target_branch: 'feature') + subject.update!(source_branch: 'master', target_branch: 'feature') expect(subject.can_remove_source_branch?(user)).to be_falsey end @@ -2501,7 +2501,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'with a completely different branch' do before do - subject.update(target_branch: 'csv') + subject.update!(target_branch: 'csv') end it_behaves_like 'returning all SHA' @@ -2509,7 +2509,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'with a branch having no difference' do before do - subject.update(target_branch: 'branch-merged') + subject.update!(target_branch: 'branch-merged') subject.reload # make sure commits were not cached end @@ -3207,7 +3207,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'and a failed pipeline is associated' do before do - pipeline.update(status: 'failed', sha: subject.diff_head_sha) + pipeline.update!(status: 'failed', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline) { pipeline } end @@ -3216,7 +3216,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'and a successful pipeline is associated' do before do - pipeline.update(status: 'success', sha: subject.diff_head_sha) + pipeline.update!(status: 'success', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline) { pipeline } end @@ -3225,7 +3225,7 @@ RSpec.describe MergeRequest, factory_default: :keep do context 'and a skipped pipeline is associated' do before do - pipeline.update(status: 'skipped', sha: subject.diff_head_sha) + pipeline.update!(status: 'skipped', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline).and_return(pipeline) end @@ -3530,7 +3530,7 @@ RSpec.describe MergeRequest, factory_default: :keep do before do # Update merge_request_diff so that #diff_refs will return commit.diff_refs allow(subject).to receive(:create_merge_request_diff) do - subject.merge_request_diffs.create( + subject.merge_request_diffs.create!( base_commit_sha: commit.parent_id, start_commit_sha: commit.parent_id, head_commit_sha: commit.sha @@ -3800,7 +3800,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end it 'returns false if the merge request is merged' do - merge_request.update(state: 'merged') + merge_request.update!(state: 'merged') expect(merge_request.reload.reopenable?).to be_falsey end @@ -4029,9 +4029,9 @@ RSpec.describe MergeRequest, factory_default: :keep do subject { create(:merge_request, importing: true, source_project: project) } - let!(:merge_request_diff1) { subject.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } - let!(:merge_request_diff2) { subject.merge_request_diffs.create(head_commit_sha: nil) } - let!(:merge_request_diff3) { subject.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') } + let!(:merge_request_diff1) { subject.merge_request_diffs.create!(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } + let!(:merge_request_diff2) { subject.merge_request_diffs.create!(head_commit_sha: nil) } + let!(:merge_request_diff3) { subject.merge_request_diffs.create!(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') } context 'with diff refs' do it 'returns the diffs' do @@ -4062,9 +4062,9 @@ RSpec.describe MergeRequest, factory_default: :keep do subject { create(:merge_request, importing: true, source_project: project) } - let!(:merge_request_diff1) { subject.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } - let!(:merge_request_diff2) { subject.merge_request_diffs.create(head_commit_sha: nil) } - let!(:merge_request_diff3) { subject.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') } + let!(:merge_request_diff1) { subject.merge_request_diffs.create!(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } + let!(:merge_request_diff2) { subject.merge_request_diffs.create!(head_commit_sha: nil) } + let!(:merge_request_diff3) { subject.merge_request_diffs.create!(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') } context 'when the diff refs are for an older merge request version' do let(:diff_refs) { merge_request_diff1.diff_refs } @@ -4108,7 +4108,7 @@ RSpec.describe MergeRequest, factory_default: :keep do it 'refreshes the number of open merge requests of the target project' do project = subject.target_project - expect { subject.destroy } + expect { subject.destroy! } .to change { project.open_merge_requests_count }.from(1).to(0) end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 20dee288052..fac498bc94a 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -158,7 +158,7 @@ RSpec.describe Milestone do it 'returns false if milestone active and not all nested issues closed' do issue.milestone = milestone - issue.save + issue.save! expect(milestone.can_be_closed?).to be_falsey end diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 1111290cade..bcad9eb6e23 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -212,18 +212,17 @@ RSpec.describe PipelineSerializer do context 'with build environments' do let(:ref) { 'feature' } - it 'verifies number of queries', :request_store do - stub_licensed_features(protected_environments: true) + let_it_be(:production) { create(:environment, :production, project: project) } + let_it_be(:staging) { create(:environment, :staging, project: project) } - env = create(:environment, project: project) - create(:ci_build, :scheduled, project: project, environment: env.name) - create(:ci_build, :scheduled, project: project, environment: env.name) - create(:ci_build, :scheduled, project: project, environment: env.name) + it 'executes one query to fetch all related environments', :request_store do + pipeline = create(:ci_pipeline, project: project) + create(:ci_build, :manual, pipeline: pipeline, environment: production.name) + create(:ci_build, :manual, pipeline: pipeline, environment: staging.name) + create(:ci_build, :scheduled, pipeline: pipeline, environment: production.name) + create(:ci_build, :scheduled, pipeline: pipeline, environment: staging.name) - recorded = ActiveRecord::QueryRecorder.new { subject } - expected_queries = Gitlab.ee? ? 56 : 52 - expect(recorded.count).to be_within(1).of(expected_queries) - expect(recorded.cached_count).to eq(0) + expect { subject }.not_to exceed_query_limit(1).for_query /SELECT "environments".*/ end end |