From e86e1e515a7a4e4e1ee53d3d33bdfebfddd226a6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 6 Jun 2017 20:23:19 +0800 Subject: [PATCH] Try to report why it's failing and fix tests --- app/services/ci/create_pipeline_service.rb | 2 +- app/services/ci/create_trigger_request_service.rb | 3 ++- lib/api/triggers.rb | 9 +++++---- lib/api/v3/triggers.rb | 7 ++++--- lib/ci/api/triggers.rb | 7 ++++--- spec/requests/ci/api/triggers_spec.rb | 14 ++++++++++++-- .../ci/create_trigger_request_service_spec.rb | 12 ++++++------ spec/workers/post_receive_spec.rb | 1 + 8 files changed, 35 insertions(+), 20 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index a51c52b3f91..b3dbb548454 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -23,7 +23,7 @@ module Ci return error('Insufficient permissions to create a new pipeline') end - unless trigger_request && trigger_request.trigger.owner + if trigger_request && !trigger_request.trigger.owner return error('Legacy trigger without a owner is not allowed') end diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb index beb27a5a597..e4f55c27f61 100644 --- a/app/services/ci/create_trigger_request_service.rb +++ b/app/services/ci/create_trigger_request_service.rb @@ -6,7 +6,8 @@ module Ci pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref). execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request) - trigger_request if pipeline.persisted? + trigger_request.pipeline = pipeline + trigger_request end end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index a9f2ca2608e..9e444563fdf 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -28,11 +28,12 @@ module API # create request and trigger builds trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref].to_s, variables) - if trigger_request - present trigger_request.pipeline, with: Entities::Pipeline + pipeline = trigger_request.pipeline + + if pipeline.persisted? + present pipeline, with: Entities::Pipeline else - errors = 'No pipeline created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index a23d6b6b48c..7e75c579528 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -29,11 +29,12 @@ module API # create request and trigger builds trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref].to_s, variables) - if trigger_request + pipeline = trigger_request.pipeline + + if pipeline.persisted? present trigger_request, with: ::API::V3::Entities::TriggerRequest else - errors = 'No builds created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end diff --git a/lib/ci/api/triggers.rb b/lib/ci/api/triggers.rb index 6e622601680..0e5174e13ab 100644 --- a/lib/ci/api/triggers.rb +++ b/lib/ci/api/triggers.rb @@ -25,11 +25,12 @@ module Ci # create request and trigger builds trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref], variables) - if trigger_request + pipeline = trigger_request.pipeline + + if pipeline.persisted? present trigger_request, with: Entities::TriggerRequest else - errors = 'No builds created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end end diff --git a/spec/requests/ci/api/triggers_spec.rb b/spec/requests/ci/api/triggers_spec.rb index 26b03c0f148..e481ca916ab 100644 --- a/spec/requests/ci/api/triggers_spec.rb +++ b/spec/requests/ci/api/triggers_spec.rb @@ -5,7 +5,14 @@ describe Ci::API::Triggers do let!(:trigger_token) { 'secure token' } let!(:project) { create(:project, :repository, ci_id: 10) } let!(:project2) { create(:empty_project, ci_id: 11) } - let!(:trigger) { create(:ci_trigger, project: project, token: trigger_token) } + + let!(:trigger) do + create(:ci_trigger, + project: project, + token: trigger_token, + owner: create(:user)) + end + let(:options) do { token: trigger_token @@ -14,6 +21,8 @@ describe Ci::API::Triggers do before do stub_ci_pipeline_to_return_yaml_file + + project.add_developer(trigger.owner) end context 'Handles errors' do @@ -47,7 +56,8 @@ describe Ci::API::Triggers do it 'returns bad request with no builds created if there\'s no commit for that ref' do post ci_api("/projects/#{project.ci_id}/refs/other-branch/trigger"), options expect(response).to have_http_status(400) - expect(json_response['message']).to eq('No builds created') + expect(json_response['message']['base']) + .to contain_exactly('Reference not found') end context 'Validates variables' do diff --git a/spec/services/ci/create_trigger_request_service_spec.rb b/spec/services/ci/create_trigger_request_service_spec.rb index f2956262f4b..8582c74e734 100644 --- a/spec/services/ci/create_trigger_request_service_spec.rb +++ b/spec/services/ci/create_trigger_request_service_spec.rb @@ -3,10 +3,13 @@ require 'spec_helper' describe Ci::CreateTriggerRequestService, services: true do let(:service) { described_class.new } let(:project) { create(:project, :repository) } - let(:trigger) { create(:ci_trigger, project: project) } + let(:trigger) { create(:ci_trigger, project: project, owner: owner) } + let(:owner) { create(:user) } before do stub_ci_pipeline_to_return_yaml_file + + project.add_developer(owner) end describe '#execute' do @@ -21,9 +24,6 @@ describe Ci::CreateTriggerRequestService, services: true do end context 'with owner' do - let(:owner) { create(:user) } - let(:trigger) { create(:ci_trigger, project: project, owner: owner) } - it { expect(subject).to be_kind_of(Ci::TriggerRequest) } it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } it { expect(subject.pipeline).to be_trigger } @@ -36,7 +36,7 @@ describe Ci::CreateTriggerRequestService, services: true do context 'no commit for ref' do subject { service.execute(project, trigger, 'other-branch') } - it { expect(subject).to be_nil } + it { expect(subject.pipeline).not_to be_persisted } end context 'no builds created' do @@ -46,7 +46,7 @@ describe Ci::CreateTriggerRequestService, services: true do stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }') end - it { expect(subject).to be_nil } + it { expect(subject.pipeline).not_to be_persisted } end end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index f4bc63bcc6a..7da48647bb5 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -82,6 +82,7 @@ describe PostReceive do OpenStruct.new(id: '123456') end allow_any_instance_of(Ci::CreatePipelineService).to receive(:branch?).and_return(true) + allow_any_instance_of(Repository).to receive(:ref_exists?).and_return(true) stub_ci_pipeline_to_return_yaml_file end -- GitLab