From 1a5eedf3fedb09a3f6a7210ce435c95812c55013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 18 Nov 2016 18:30:07 +0100 Subject: [PATCH] Fix a wrong "The build for this merge request failed" message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also allow merge request to be merged with skipped pipeline and the "only allow merge when pipeline is green" feature enabled Signed-off-by: Rémy Coutable --- app/models/merge_request.rb | 2 +- .../merge_requests/widget/_open.html.haml | 2 +- ...t-failed-although-builds-still-running.yml | 4 ++ ...eable_with_unresolved_discussions_spec.rb} | 0 ...nly_allow_merge_if_build_succeeds_spec.rb} | 45 +++++++++++++++---- ...s.rb => toggle_whitespace_changes_spec.rb} | 0 spec/models/merge_request_spec.rb | 20 ++++++++- 7 files changed, 61 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml rename spec/features/merge_requests/{check_if_mergeable_with_unresolved_discussions.rb => check_if_mergeable_with_unresolved_discussions_spec.rb} (100%) rename spec/features/merge_requests/{only_allow_merge_if_build_succeeds.rb => only_allow_merge_if_build_succeeds_spec.rb} (69%) rename spec/features/merge_requests/{toggle_whitespace_changes.rb => toggle_whitespace_changes_spec.rb} (100%) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9d3eab52189..6c3c093d084 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -686,7 +686,7 @@ class MergeRequest < ActiveRecord::Base def mergeable_ci_state? return true unless project.only_allow_merge_if_build_succeeds? - !pipeline || pipeline.success? + !pipeline || pipeline.success? || pipeline.skipped? end def environments diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 01314eb37d0..ac26aa569ba 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -23,7 +23,7 @@ = render 'projects/merge_requests/widget/open/merge_when_build_succeeds' - elsif !@merge_request.can_be_merged_by?(current_user) = render 'projects/merge_requests/widget/open/not_allowed' - - elsif !@merge_request.mergeable_ci_state? + - elsif !@merge_request.mergeable_ci_state? && (@pipeline.failed? || @pipeline.canceled?) = render 'projects/merge_requests/widget/open/build_failed' - elsif !@merge_request.mergeable_discussions_state? = render 'projects/merge_requests/widget/open/unresolved_discussions' diff --git a/changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml b/changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml new file mode 100644 index 00000000000..3773459550f --- /dev/null +++ b/changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml @@ -0,0 +1,4 @@ +--- +title: Fix a wrong "The build for this merge request failed" message +merge_request: 7579 +author: diff --git a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions_spec.rb similarity index 100% rename from spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb rename to spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions_spec.rb diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb similarity index 69% rename from spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb rename to spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb index 80e8b8fc642..1ec3103feef 100644 --- a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb +++ b/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' feature 'Only allow merge requests to be merged if the build succeeds', feature: true do - let(:project) { create(:project, :public) } - let(:merge_request) { create(:merge_request_with_diffs, source_project: project) } + let(:merge_request) { create(:merge_request_with_diffs) } + let(:project) { merge_request.target_project } before do login_as merge_request.author @@ -19,7 +19,13 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when project has CI enabled' do - let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) } + given!(:pipeline) do + create(:ci_empty_pipeline, + project: project, + sha: merge_request.diff_head_sha, + ref: merge_request.source_branch, + status: status) + end context 'when merge requests can only be merged if the build succeeds' do before do @@ -27,7 +33,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI is running' do - before { pipeline.update_column(:status, :running) } + given(:status) { :running } it 'does not allow to merge immediately' do visit_merge_request(merge_request) @@ -38,7 +44,18 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI failed' do - before { pipeline.update_column(:status, :failed) } + given(:status) { :failed } + + it 'does not allow MR to be merged' do + visit_merge_request(merge_request) + + expect(page).not_to have_button 'Accept Merge Request' + expect(page).to have_content('Please retry the build or push a new commit to fix the failure.') + end + end + + context 'when CI canceled' do + given(:status) { :canceled } it 'does not allow MR to be merged' do visit_merge_request(merge_request) @@ -49,7 +66,17 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI succeeded' do - before { pipeline.update_column(:status, :success) } + given(:status) { :success } + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'when CI skipped' do + given(:status) { :skipped } it 'allows MR to be merged' do visit_merge_request(merge_request) @@ -65,7 +92,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI is running' do - before { pipeline.update_column(:status, :running) } + given(:status) { :running } it 'allows MR to be merged immediately', js: true do visit_merge_request(merge_request) @@ -78,7 +105,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI failed' do - before { pipeline.update_column(:status, :failed) } + given(:status) { :failed } it 'allows MR to be merged' do visit_merge_request(merge_request) @@ -88,7 +115,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: end context 'when CI succeeded' do - before { pipeline.update_column(:status, :success) } + given(:status) { :success } it 'allows MR to be merged' do visit_merge_request(merge_request) diff --git a/spec/features/merge_requests/toggle_whitespace_changes.rb b/spec/features/merge_requests/toggle_whitespace_changes_spec.rb similarity index 100% rename from spec/features/merge_requests/toggle_whitespace_changes.rb rename to spec/features/merge_requests/toggle_whitespace_changes_spec.rb diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fb032a89d50..0b4277b1edd 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -856,13 +856,31 @@ describe MergeRequest, models: true do context 'when it is only allowed to merge when build is green' do context 'and a failed pipeline is associated' do before do - pipeline.statuses << create(:commit_status, status: 'failed', project: project) + pipeline.update(status: 'failed') allow(subject).to receive(:pipeline) { pipeline } end it { expect(subject.mergeable_ci_state?).to be_falsey } end + context 'and a successful pipeline is associated' do + before do + pipeline.update(status: 'success') + allow(subject).to receive(:pipeline) { pipeline } + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + + context 'and a skipped pipeline is associated' do + before do + pipeline.update(status: 'skipped') + allow(subject).to receive(:pipeline) { pipeline } + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + context 'when no pipeline is associated' do before do allow(subject).to receive(:pipeline) { nil }