From cd9fb6e708f4fe6e1396b40ea1b431bf1cd39fb8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 3 Nov 2021 00:22:17 +0800 Subject: [PATCH 1/8] Avoid retrying controller tests due to data not being reset --- spec/spec_helper.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6a0d9ea06695c1..47aea2c1c37460 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -197,6 +197,14 @@ if ENV['CI'] || ENV['RETRIES'] # This includes the first try, i.e. tests will be run 4 times before failing. config.default_retry_count = ENV.fetch('RETRIES', 3).to_i + 1 + + # Do not retry controller tests because rspec-retry cannot properly + # reset the controller which may contain data from last attempt. See + # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73360 + config.around(:each, type: :controller) do |example| + example.run_with_retry(retry: 1) + end + config.exceptions_to_hard_fail = [DeprecationToolkitEnv::DeprecationBehaviors::SelectiveRaise::RaiseDisallowedDeprecation] end -- GitLab From d0d0b92c75a331bae9e959411e0a1641ae37e027 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 3 Nov 2021 01:31:04 +0800 Subject: [PATCH 2/8] The hidden namespace will cause one extra query --- spec/controllers/dashboard/todos_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index f0aa351bee04b0..cf528b414c09e3 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -62,7 +62,7 @@ create(:issue, project: project, assignees: [user]) group_2 = create(:group) group_2.add_owner(user) - project_2 = create(:project) + project_2 = create(:project, namespace: user.namespace) project_2.add_developer(user) merge_request_2 = create(:merge_request, source_project: project_2) create(:todo, project: project, author: author, user: user, target: merge_request_2) -- GitLab From fe8f24a5009f32b49ab1b37f2bcfb6dff10bfdf9 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 4 Nov 2021 01:22:30 +0800 Subject: [PATCH 3/8] Permission is somehow cached. Don't add if we don't want --- .../projects/merge_requests/diffs_controller_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index 3d7636b1f30bcc..5b1c6777523e55 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb @@ -86,10 +86,11 @@ let(:project) { create(:project, :repository) } let(:user) { create(:user) } + let(:maintainer) { true } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } before do - project.add_maintainer(user) + project.add_maintainer(user) if maintainer sign_in(user) end @@ -383,8 +384,9 @@ def diff_for_path(extra_params = {}) end context 'when the user cannot view the merge request' do + let(:maintainer) { false } + before do - project.team.truncate diff_for_path(old_path: existing_path, new_path: existing_path) end -- GitLab From 5e37b06024bc0f1f6e4ba316eddf5a55df40d526 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 5 Nov 2021 02:23:06 +0800 Subject: [PATCH 4/8] Do not allow maintainer to push to maintain 404 --- .../merge_requests_controller_spec.rb | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 438fc2f210627f..e2fc867db44e6d 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -10,7 +10,8 @@ let_it_be_with_reload(:project_public_with_private_builds) { create(:project, :repository, :public, :builds_private) } let(:user) { project.owner } - let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } + let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: merge_request_source_project, allow_maintainer_to_push: false) } + let(:merge_request_source_project) { project } before do sign_in(user) @@ -2073,8 +2074,6 @@ def get_pipeline_status end describe 'POST #rebase' do - let(:viewer) { user } - def post_rebase post :rebase, params: { namespace_id: project.namespace, project_id: project, id: merge_request } end @@ -2085,7 +2084,7 @@ def expect_rebase_worker_for(user) context 'successfully' do it 'enqeues a RebaseWorker' do - expect_rebase_worker_for(viewer) + expect_rebase_worker_for(user) post_rebase @@ -2108,17 +2107,17 @@ def expect_rebase_worker_for(user) context 'with a forked project' do let(:forked_project) { fork_project(project, fork_owner, repository: true) } let(:fork_owner) { create(:user) } + let(:merge_request_source_project) { forked_project } - before do - project.add_developer(fork_owner) + context 'user cannot push to source branch' do + before do + project.add_developer(fork_owner) - merge_request.update!(source_project: forked_project) - forked_project.add_reporter(user) - end + forked_project.add_reporter(user) + end - context 'user cannot push to source branch' do it 'returns 404' do - expect_rebase_worker_for(viewer).never + expect_rebase_worker_for(user).never post_rebase -- GitLab From 613f64a09ec92d8d92f64c8872d6fa12cdcc89a0 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 5 Nov 2021 04:19:11 +0800 Subject: [PATCH 5/8] RegistrationsController#create cannot record the user This is because `auth_user` is memoized as `nil` and even after the new user is created, there's no easy way to update the current context with the newly created user. It's passing before because rspec-retry will post again, which then it memoized the current user from previous request. --- spec/controllers/registrations_controller_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index a25c597edb261a..baf500c2b57a4f 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -499,13 +499,12 @@ expect(User.last.name).to eq("#{base_user_params[:first_name]} #{base_user_params[:last_name]}") end - it 'sets the username and caller_id in the context' do + it 'sets the caller_id in the context' do expect(controller).to receive(:create).and_wrap_original do |m, *args| m.call(*args) expect(Gitlab::ApplicationContext.current) - .to include('meta.user' => base_user_params[:username], - 'meta.caller_id' => 'RegistrationsController#create') + .to include('meta.caller_id' => 'RegistrationsController#create') end subject -- GitLab From a5727ff863b1f83651a2432439ec1487828f2883 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 5 Nov 2021 04:57:52 +0800 Subject: [PATCH 6/8] We have 2 audit events now --- ee/spec/controllers/ee/registrations_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/controllers/ee/registrations_controller_spec.rb b/ee/spec/controllers/ee/registrations_controller_spec.rb index 270c5da47fb35d..081a3dda174863 100644 --- a/ee/spec/controllers/ee/registrations_controller_spec.rb +++ b/ee/spec/controllers/ee/registrations_controller_spec.rb @@ -111,8 +111,8 @@ end context 'when user registers for the instance' do - it 'logs an audit event' do - expect { subject }.to change { AuditEvent.count }.by(1) + it 'logs add email event and instance access request event' do + expect { subject }.to change { AuditEvent.count }.by(2) end it 'logs the audit event info', :aggregate_failures do -- GitLab From 5a68804fcad4b75fd05ccd891528c1aa03384c9a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 5 Nov 2021 05:28:17 +0800 Subject: [PATCH 7/8] We don't need to care about the ordering here --- .../registrations/projects_controller_shared_examples.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/support/shared_examples/controllers/registrations/projects_controller_shared_examples.rb b/ee/spec/support/shared_examples/controllers/registrations/projects_controller_shared_examples.rb index e3703cc834cfcf..b917ce7b37a934 100644 --- a/ee/spec/support/shared_examples/controllers/registrations/projects_controller_shared_examples.rb +++ b/ee/spec/support/shared_examples/controllers/registrations/projects_controller_shared_examples.rb @@ -71,7 +71,7 @@ let_it_be(:trial_onboarding_flow_params) { { trial_onboarding_flow: true } } it 'creates a new project, a "Learn GitLab - Ultimate trial" project, does not set a cookie' do - expect { subject }.to change { namespace.projects.pluck(:name) }.from([]).to(['New project', s_('Learn GitLab - Ultimate trial')]) + expect { subject }.to change { namespace.projects.pluck(:name).sort }.from([]).to(['New project', s_('Learn GitLab - Ultimate trial')].sort) expect(subject).to have_gitlab_http_status(:redirect) expect(namespace.projects.find_by_name(s_('Learn GitLab - Ultimate trial'))).to be_import_finished end -- GitLab From 461b342d270cd85c172a4cda908ded21f7e16efc Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 5 Nov 2021 22:13:13 +0800 Subject: [PATCH 8/8] It's also giving blocked property --- ee/spec/fixtures/api/schemas/related_issue.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/spec/fixtures/api/schemas/related_issue.json b/ee/spec/fixtures/api/schemas/related_issue.json index 243b66c8cc6806..3ab6f6769c34c9 100644 --- a/ee/spec/fixtures/api/schemas/related_issue.json +++ b/ee/spec/fixtures/api/schemas/related_issue.json @@ -12,7 +12,8 @@ "reference", "path", "relation_path", - "weight" + "weight", + "blocked" ], "properties": { "id": { "type": "integer" }, @@ -23,6 +24,7 @@ "due_date": { "type": ["string", "null"] }, "state": { "type": "string" }, "weight": { "type": ["integer", "null"] }, + "blocked": { "type": "boolean" }, "reference": { "type": "string" }, "path": { "type": "string" }, "relation_path": { "type": "string" }, -- GitLab