From 160ffb6870c39d467bd5efa4170007be18ccd7d2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Apr 2018 12:56:52 +0200 Subject: [PATCH 1/6] Fix empty state for build that doesn ot have a trace It also adds a fallback that covers other edge cases that might surface later. --- lib/gitlab/ci/status/build/common.rb | 7 +++++++ lib/gitlab/ci/status/build/empty.rb | 21 +++++++++++++++++++++ lib/gitlab/ci/status/build/factory.rb | 3 ++- 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 lib/gitlab/ci/status/build/empty.rb diff --git a/lib/gitlab/ci/status/build/common.rb b/lib/gitlab/ci/status/build/common.rb index c0c7c7f5b5d..639e88ebf1f 100644 --- a/lib/gitlab/ci/status/build/common.rb +++ b/lib/gitlab/ci/status/build/common.rb @@ -3,6 +3,13 @@ module Gitlab module Status module Build module Common + def illustration + { + image: 'illustrations/skipped-job_empty.svg', + size: 'svg-430' + } + end + def has_details? can?(user, :read_build, subject) end diff --git a/lib/gitlab/ci/status/build/empty.rb b/lib/gitlab/ci/status/build/empty.rb new file mode 100644 index 00000000000..3e598877602 --- /dev/null +++ b/lib/gitlab/ci/status/build/empty.rb @@ -0,0 +1,21 @@ +module Gitlab + module Ci + module Status + module Build + class Empty < Status::Extended + def illustration + { + image: 'illustrations/skipped-job_empty.svg', + size: 'svg-430', + title: _('This job does not have a trace.') + } + end + + def self.matches?(build, user) + !build.has_trace? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index 2b26ebb45a1..0b42d5edfc1 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -17,7 +17,8 @@ module Gitlab Status::Build::Play, Status::Build::Stop], [Status::Build::Action], - [Status::Build::Retried]] + [Status::Build::Retried], + [Status::Build::Empty]] end def self.common_helpers From 199e31eb24ae314428c2801454f9de676a78496c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Apr 2018 13:33:27 +0200 Subject: [PATCH 2/6] Extend specs for detailed build extended statuses --- lib/gitlab/ci/status/build/factory.rb | 6 ++-- .../lib/gitlab/ci/status/build/common_spec.rb | 6 ++++ spec/lib/gitlab/ci/status/build/empty_spec.rb | 31 +++++++++++++++++++ .../gitlab/ci/status/build/factory_spec.rb | 9 ++++-- 4 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 spec/lib/gitlab/ci/status/build/empty_spec.rb diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index 0b42d5edfc1..9b3a97a0f0f 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -9,7 +9,8 @@ module Gitlab Status::Build::Canceled, Status::Build::Created, Status::Build::Pending, - Status::Build::Skipped], + Status::Build::Skipped, + Status::Build::Empty], [Status::Build::Cancelable, Status::Build::Retryable], [Status::Build::Failed], @@ -17,8 +18,7 @@ module Gitlab Status::Build::Play, Status::Build::Stop], [Status::Build::Action], - [Status::Build::Retried], - [Status::Build::Empty]] + [Status::Build::Retried]] end def self.common_helpers diff --git a/spec/lib/gitlab/ci/status/build/common_spec.rb b/spec/lib/gitlab/ci/status/build/common_spec.rb index 2cce7a23ea7..ca3c66f0152 100644 --- a/spec/lib/gitlab/ci/status/build/common_spec.rb +++ b/spec/lib/gitlab/ci/status/build/common_spec.rb @@ -38,4 +38,10 @@ describe Gitlab::Ci::Status::Build::Common do expect(subject.details_path).to include "jobs/#{build.id}" end end + + describe '#illustration' do + it 'provides a fallback empty state illustration' do + expect(subject.illustration).not_to be_empty + end + end end diff --git a/spec/lib/gitlab/ci/status/build/empty_spec.rb b/spec/lib/gitlab/ci/status/build/empty_spec.rb new file mode 100644 index 00000000000..379a5a9849b --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/empty_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::Empty do + let(:build) { create(:ci_build, :running) } + let(:status) { double('core status') } + let(:user) { double('user') } + + subject { described_class.new(status) } + + describe '#illustration' do + it 'provides an empty state illustration' do + expect(subject.illustration).not_to be_empty + end + end + + describe '.matches?' do + subject { described_class.matches?(build, user) } + + context 'when a build has trace' do + let(:build) { create(:ci_build, :trace_artifact) } + + it { is_expected.to be_falsy } + end + + context 'with a build that has not been retried' do + let(:build) { create(:ci_build, :running) } + + it { is_expected.to be_truthy } + end + end +end diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index 6d5b73bb01b..83d48f1bc73 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -75,7 +75,9 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Retryable, Gitlab::Ci::Status::Build::Failed] + .to eq [Gitlab::Ci::Status::Build::Empty, + Gitlab::Ci::Status::Build::Retryable, + Gitlab::Ci::Status::Build::Failed] end it 'fabricates a failed build status' do @@ -94,7 +96,7 @@ describe Gitlab::Ci::Status::Build::Factory do end context 'when build is allowed to fail' do - let(:build) { create(:ci_build, :failed, :allowed_to_fail) } + let(:build) { create(:ci_build, :failed, :allowed_to_fail, :trace_artifact) } it 'matches correct core status' do expect(factory.core_status).to be_a Gitlab::Ci::Status::Failed @@ -160,7 +162,8 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Cancelable] + .to eq [Gitlab::Ci::Status::Build::Empty, + Gitlab::Ci::Status::Build::Cancelable] end it 'fabricates a canceable build status' do From 2ea25cbcaff0ca62b0570a129771f08bca532820 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Apr 2018 14:27:34 +0200 Subject: [PATCH 3/6] Revert adding `Empty` extended status to avoid N+1 queries --- lib/gitlab/ci/status/build/common.rb | 3 +- lib/gitlab/ci/status/build/empty.rb | 21 ------------- lib/gitlab/ci/status/build/factory.rb | 3 +- spec/lib/gitlab/ci/status/build/empty_spec.rb | 31 ------------------- .../gitlab/ci/status/build/factory_spec.rb | 6 ++-- 5 files changed, 5 insertions(+), 59 deletions(-) delete mode 100644 lib/gitlab/ci/status/build/empty.rb delete mode 100644 spec/lib/gitlab/ci/status/build/empty_spec.rb diff --git a/lib/gitlab/ci/status/build/common.rb b/lib/gitlab/ci/status/build/common.rb index 639e88ebf1f..52d2df0a40a 100644 --- a/lib/gitlab/ci/status/build/common.rb +++ b/lib/gitlab/ci/status/build/common.rb @@ -6,7 +6,8 @@ module Gitlab def illustration { image: 'illustrations/skipped-job_empty.svg', - size: 'svg-430' + size: 'svg-430', + title: _('This job does not have a trace.'), } end diff --git a/lib/gitlab/ci/status/build/empty.rb b/lib/gitlab/ci/status/build/empty.rb deleted file mode 100644 index 3e598877602..00000000000 --- a/lib/gitlab/ci/status/build/empty.rb +++ /dev/null @@ -1,21 +0,0 @@ -module Gitlab - module Ci - module Status - module Build - class Empty < Status::Extended - def illustration - { - image: 'illustrations/skipped-job_empty.svg', - size: 'svg-430', - title: _('This job does not have a trace.') - } - end - - def self.matches?(build, user) - !build.has_trace? - end - end - end - end - end -end diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index 9b3a97a0f0f..2b26ebb45a1 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -9,8 +9,7 @@ module Gitlab Status::Build::Canceled, Status::Build::Created, Status::Build::Pending, - Status::Build::Skipped, - Status::Build::Empty], + Status::Build::Skipped], [Status::Build::Cancelable, Status::Build::Retryable], [Status::Build::Failed], diff --git a/spec/lib/gitlab/ci/status/build/empty_spec.rb b/spec/lib/gitlab/ci/status/build/empty_spec.rb deleted file mode 100644 index 379a5a9849b..00000000000 --- a/spec/lib/gitlab/ci/status/build/empty_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Status::Build::Empty do - let(:build) { create(:ci_build, :running) } - let(:status) { double('core status') } - let(:user) { double('user') } - - subject { described_class.new(status) } - - describe '#illustration' do - it 'provides an empty state illustration' do - expect(subject.illustration).not_to be_empty - end - end - - describe '.matches?' do - subject { described_class.matches?(build, user) } - - context 'when a build has trace' do - let(:build) { create(:ci_build, :trace_artifact) } - - it { is_expected.to be_falsy } - end - - context 'with a build that has not been retried' do - let(:build) { create(:ci_build, :running) } - - it { is_expected.to be_truthy } - end - end -end diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index 83d48f1bc73..d53a7d468e3 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -75,8 +75,7 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Empty, - Gitlab::Ci::Status::Build::Retryable, + .to eq [Gitlab::Ci::Status::Build::Retryable, Gitlab::Ci::Status::Build::Failed] end @@ -162,8 +161,7 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Empty, - Gitlab::Ci::Status::Build::Cancelable] + .to eq [Gitlab::Ci::Status::Build::Cancelable] end it 'fabricates a canceable build status' do From 8196794c1efb5492a7df24337d2fc08fe76cc8f7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Apr 2018 14:37:08 +0200 Subject: [PATCH 4/6] Add a test for job empty state with missing trace --- lib/gitlab/ci/status/build/common.rb | 2 +- spec/features/projects/jobs_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ci/status/build/common.rb b/lib/gitlab/ci/status/build/common.rb index 52d2df0a40a..c1fc70ac266 100644 --- a/lib/gitlab/ci/status/build/common.rb +++ b/lib/gitlab/ci/status/build/common.rb @@ -7,7 +7,7 @@ module Gitlab { image: 'illustrations/skipped-job_empty.svg', size: 'svg-430', - title: _('This job does not have a trace.'), + title: _('This job does not have a trace.') } end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 749a1b81872..bba39c9f37f 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -464,6 +464,17 @@ feature 'Jobs' do expect(page).to have_content('This job has been skipped') end end + + context 'when job is running but has no trace yet' do + let(:job) { create(:ci_build, :running, pipeline: pipeline) } + + it 'renders empty state' do + visit project_job_path(project, job) + + expect(job).not_to have_trace + expect(page).to have_content('This job does not have a trace.') + end + end end describe "POST /:project/jobs/:id/cancel", :js do From 255f7386f7604a829755e39185f8b62163d854ce Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Apr 2018 10:55:48 +0200 Subject: [PATCH 5/6] Show build trace placeholder when build is running --- app/views/projects/jobs/show.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index 8beb4ffef45..cbbcc8f1db5 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -55,7 +55,7 @@ - else Job has been erased #{time_ago_with_tooltip(@build.erased_at)} - - if @build.has_trace? + - if @build.running? || @build.has_trace? .build-trace-container.prepend-top-default .top-bar.js-top-bar .js-truncated-info.truncated-info.hidden-xs.pull-left.hidden< From 11bfc4b29b92a3748423114a0f9576d6b96bc9ff Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Apr 2018 11:37:56 +0200 Subject: [PATCH 6/6] Fix tests for job empty states for a failed build --- spec/features/projects/jobs_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index bba39c9f37f..a460024542c 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -465,8 +465,8 @@ feature 'Jobs' do end end - context 'when job is running but has no trace yet' do - let(:job) { create(:ci_build, :running, pipeline: pipeline) } + context 'when job is failed but has no trace' do + let(:job) { create(:ci_build, :failed, pipeline: pipeline) } it 'renders empty state' do visit project_job_path(project, job)