From bdbe892ea229e8916f922b168c239db393f3d677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 18 Feb 2016 14:28:25 +0000 Subject: [PATCH] Merge branch 'fix/ci-first-job-allow-failure' into 'master' Fix CI builds scheduler when first build in stage is allowed to fail This fixes an edge case in CI builds scheduler when first build in stage was marked as allowed to fail. Closes #3192 See merge request !2869 --- CHANGELOG | 1 + app/services/ci/create_builds_service.rb | 1 + lib/ci/status.rb | 4 +- spec/factories/ci/builds.rb | 20 ++++++++++ spec/lib/ci/status_spec.rb | 37 +++++++++++++++++++ spec/models/ci/commit_spec.rb | 29 +++++++++++++++ .../services/ci/create_builds_service_spec.rb | 28 ++++++++++++++ 7 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 spec/lib/ci/status_spec.rb create mode 100644 spec/services/ci/create_builds_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 08a9b8df4a..b0d08d9722 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -63,6 +63,7 @@ v 8.5.0 (unreleased) - Fix CI builds badge, add a new link to builds badge, deprecate the old one - Fix broken link to project in build notification emails - Ability to see and sort on vote count from Issues and MR lists + - Fix builds scheduler when first build in stage was allowed to fail v 8.4.4 - Update omniauth-saml gem to 1.4.2 diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb index ad901f2da5..002f7ba127 100644 --- a/app/services/ci/create_builds_service.rb +++ b/app/services/ci/create_builds_service.rb @@ -34,6 +34,7 @@ module Ci build = commit.builds.create!(build_attrs) build.execute_hooks + build end end end diff --git a/lib/ci/status.rb b/lib/ci/status.rb index c02b3b8f3e..3fb1fe2949 100644 --- a/lib/ci/status.rb +++ b/lib/ci/status.rb @@ -1,11 +1,9 @@ module Ci class Status def self.get_status(statuses) - statuses.reject! { |status| status.try(&:allow_failure?) } - if statuses.none? 'skipped' - elsif statuses.all?(&:success?) + elsif statuses.all? { |status| status.success? || status.ignored? } 'success' elsif statuses.all?(&:pending?) 'pending' diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index c1b6ecd329..1243647a78 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -16,10 +16,30 @@ FactoryGirl.define do commit factory: :ci_commit + trait :success do + status 'success' + end + + trait :failed do + status 'failed' + end + trait :canceled do status 'canceled' end + trait :running do + status 'running' + end + + trait :pending do + status 'pending' + end + + trait :allowed_to_fail do + allow_failure true + end + after(:build) do |build, evaluator| build.project = build.commit.project end diff --git a/spec/lib/ci/status_spec.rb b/spec/lib/ci/status_spec.rb new file mode 100644 index 0000000000..cc4199dc7b --- /dev/null +++ b/spec/lib/ci/status_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe Ci::Status do + describe '.get_status' do + subject { described_class.get_status(builds) } + + context 'all builds successful' do + let(:builds) { Array.new(2) { create(:ci_build, :success) } } + it { is_expected.to eq 'success' } + end + + context 'at least one build failed' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :failed)] } + it { is_expected.to eq 'failed' } + end + + context 'at least one running' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :running)] } + it { is_expected.to eq 'running' } + end + + context 'at least one pending' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :pending)] } + it { is_expected.to eq 'running' } + end + + context 'build success and failed but allowed to fail' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :failed, :allowed_to_fail)] } + it { is_expected.to eq 'success' } + end + + context 'one build failed but allowed to fail' do + let(:builds) { [create(:ci_build, :failed, :allowed_to_fail)] } + it { is_expected.to eq 'success' } + end + end +end diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb index dfc0cc3be1..4dc309a425 100644 --- a/spec/models/ci/commit_spec.rb +++ b/spec/models/ci/commit_spec.rb @@ -247,6 +247,35 @@ describe Ci::Commit, models: true do end end + + context 'custom stage with first job allowed to fail' do + let(:yaml) do + { + stages: ['clean', 'test'], + clean_job: { + stage: 'clean', + allow_failure: true, + script: 'BUILD', + }, + test_job: { + stage: 'test', + script: 'TEST', + }, + } + end + + before do + stub_ci_commit_yaml_file(YAML.dump(yaml)) + create_builds + end + + it 'properly schedules builds' do + expect(commit.builds.pluck(:status)).to contain_exactly('pending') + commit.builds.running_or_pending.each(&:drop) + expect(commit.builds.pluck(:status)).to contain_exactly('pending', 'failed') + end + end + context 'properly creates builds when "when" is defined' do let(:yaml) do { diff --git a/spec/services/ci/create_builds_service_spec.rb b/spec/services/ci/create_builds_service_spec.rb new file mode 100644 index 0000000000..1fca362868 --- /dev/null +++ b/spec/services/ci/create_builds_service_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe Ci::CreateBuildsService, services: true do + let(:commit) { create(:ci_commit) } + let(:user) { create(:user) } + + describe '#execute' do + # Using stubbed .gitlab-ci.yml created in commit factory + # + + subject do + described_class.new.execute(commit, 'test', 'master', nil, user, nil, status) + end + + context 'next builds available' do + let(:status) { 'success' } + + it { is_expected.to be_an_instance_of Array } + it { is_expected.to all(be_an_instance_of Ci::Build) } + end + + context 'builds skipped' do + let(:status) { 'skipped' } + + it { is_expected.to be_empty } + end + end +end