Fix handling of allowed to failure jobs

7 files changed, 49 insertions, 63 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index b6b9a90a589..aa23b5cf97c 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -114,7 +114,7 @@ module Ci
         pluck('sg.stage', status_sql)
 
       stages_with_statuses.map do |stage|
-        Ci::Stage.new(self, stage.first, status: stage.last)
+        Ci::Stage.new(self, name: stage.first, status: stage.last)
       end
     end
 
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 2a537dc2a13..cf90475f4d4 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -31,14 +31,9 @@ class CommitStatus < ActiveRecord::Base
   end
 
   scope :exclude_ignored, -> do
-    quoted_when = connection.quote_column_name('when')
     # We want to ignore failed_but_allowed jobs
     where("allow_failure = ? OR status IN (?)",
-      false, all_state_names - [:failed, :canceled]).
-      # We want to ignore skipped manual jobs
-      where("#{quoted_when} <> ? OR status <> ?", 'manual', 'skipped').
-      # We want to ignore skipped on_failure
-      where("#{quoted_when} <> ? OR status <> ?", 'on_failure', 'skipped')
+      false, all_state_names - [:failed, :canceled])
   end
 
   scope :latest_ordered, -> { latest.ordered.includes(project: :namespace) }
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index 215367cc1dc..43f312d319e 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -24,8 +24,9 @@ module HasStatus
 
       "(CASE
         WHEN (#{builds})=(#{skipped}) THEN 'skipped'
+        WHEN (#{builds})=(#{success}) THEN 'success'
         WHEN (#{builds})=(#{created}) THEN 'created'
-        WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success'
+        WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'skipped'
         WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled'
         WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending'
         WHEN (#{running})+(#{pending})+(#{created})>0 THEN 'running'
diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml
index e4a963a278c..b58dceb58c9 100644
--- a/app/views/projects/ci/pipelines/_pipeline.html.haml
+++ b/app/views/projects/ci/pipelines/_pipeline.html.haml
@@ -44,7 +44,7 @@
         Cant find HEAD commit for this branch
 
   %td.stage-cell
-    - pipeline.stages_with_statuses.each do |stage|
+    - pipeline.stages.each do |stage|
       - if stage.status
         - tooltip = "#{stage.name.titleize}: #{stage.status || 'not found'}"
         .stage-container
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 3f93d9ddf19..cdc858c13b4 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -20,8 +20,6 @@ describe Ci::Pipeline, models: true do
   it { is_expected.to respond_to :git_author_email }
   it { is_expected.to respond_to :short_sha }
 
-  it { is_expected.to delegate_method(:stages).to(:statuses) }
-
   describe '#valid_commit_sha' do
     context 'commit.sha can not start with 00000000' do
       before do
@@ -125,16 +123,51 @@ describe Ci::Pipeline, models: true do
   end
 
   describe '#stages' do
-    let(:pipeline2) { FactoryGirl.create :ci_pipeline, project: project }
-    subject { CommitStatus.where(pipeline: [pipeline, pipeline2]).stages }
-
     before do
-      FactoryGirl.create :ci_build, pipeline: pipeline2, stage: 'test', stage_idx: 1
-      FactoryGirl.create :ci_build, pipeline: pipeline, stage: 'build', stage_idx: 0
+      create(:commit_status, pipeline: pipeline, stage: 'build', name: 'linux', stage_idx: 0, status: 'success')
+      create(:commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'failed')
+      create(:commit_status, pipeline: pipeline, stage: 'deploy', name: 'staging', stage_idx: 2, status: 'running')
+      create(:commit_status, pipeline: pipeline, stage: 'test', name: 'rspec', stage_idx: 1, status: 'success')
     end
 
-    it 'return all stages' do
-      is_expected.to eq(%w(build test))
+    subject { pipeline.stages }
+
+    context 'stages list' do
+      it 'returns ordered list of stages' do
+        expect(subject.map(&:name)).to eq(%w[build test deploy])
+      end
+    end
+
+    it 'returns a valid number of stages' do
+      expect(pipeline.stages_count).to eq(3)
+    end
+
+    context 'stages with statuses' do
+      let(:statuses) do
+        subject.map do |stage|
+          [stage.name, stage.status]
+        end
+      end
+
+      it 'returns list of stages with statuses' do
+        expect(statuses).to eq([['build', 'failed'],
+                                ['test', 'success'],
+                                ['deploy', 'running']
+                               ])
+      end
+
+      context 'when build is retried' do
+        before do
+          create(:commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'success')
+        end
+
+        it 'ignores the previous state' do
+          expect(statuses).to eq([['build', 'success'],
+                                  ['test', 'success'],
+                                  ['deploy', 'running']
+                                 ])
+        end
+      end
     end
   end
 
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index 80c2a1bc7a9..1ec08c2a9d0 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -175,7 +175,7 @@ describe CommitStatus, models: true do
     end
 
     it 'returns statuses without what we want to ignore' do
-      is_expected.to eq(statuses.values_at(1, 2, 4, 5, 6, 8, 9))
+      is_expected.to eq(statuses.values_at(0, 1, 2, 3, 4, 5, 6, 8, 9))
     end
   end
 
@@ -200,49 +200,6 @@ describe CommitStatus, models: true do
     end
   end
 
-  describe '#stages' do
-    before do
-      create :commit_status, pipeline: pipeline, stage: 'build', name: 'linux', stage_idx: 0, status: 'success'
-      create :commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'failed'
-      create :commit_status, pipeline: pipeline, stage: 'deploy', name: 'staging', stage_idx: 2, status: 'running'
-      create :commit_status, pipeline: pipeline, stage: 'test', name: 'rspec', stage_idx: 1, status: 'success'
-    end
-
-    context 'stages list' do
-      subject { CommitStatus.where(pipeline: pipeline).stages }
-
-      it 'returns ordered list of stages' do
-        is_expected.to eq(%w[build test deploy])
-      end
-    end
-
-    context 'stages with statuses' do
-      subject { CommitStatus.where(pipeline: pipeline).latest.stages_status }
-
-      it 'returns list of stages with statuses' do
-        is_expected.to eq({
-          'build' => 'failed',
-          'test' => 'success',
-          'deploy' => 'running'
-        })
-      end
-
-      context 'when build is retried' do
-        before do
-          create :commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'success'
-        end
-
-        it 'ignores a previous state' do
-          is_expected.to eq({
-            'build' => 'success',
-            'test' => 'success',
-            'deploy' => 'running'
-          })
-        end
-      end
-    end
-  end
-
   describe '#commit' do
     it 'returns commit pipeline has been created for' do
       expect(commit_status.commit).to eq project.commit
diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb
index 9defb17dc92..4d0f51fe82a 100644
--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -48,7 +48,7 @@ describe HasStatus do
           [create(type, status: :failed, allow_failure: true)]
         end
 
-        it { is_expected.to eq 'success' }
+        it { is_expected.to eq 'skipped' }
       end
 
       context 'success and canceled' do