Merge branch 'wall-clock-time-for-showing-pipeline' into 'master'

Show wall-clock time when showing pipeline

## What does this MR do?

Show wall-clock time when showing pipeline instead of cumulative builds time.

Closes #17007

See merge request !5734

15 files changed, 47 insertions, 51 deletions

diff --git a/CHANGELOG b/CHANGELOG
index db07afb9d96..d78c51fbbc3 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -39,6 +39,7 @@ v 8.11.0 (unreleased)
   - API: Add deployment endpoints
   - API: Add Play endpoint on Builds
   - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
+  - Show wall clock time when showing a pipeline. !5734
   - Show member roles to all users on members page
   - Project.visible_to_user is instrumented again
   - Fix awardable button mutuality loading spinners (ClemMakesApps)
diff --git a/app/helpers/time_helper.rb b/app/helpers/time_helper.rb
index 790001222f1..271e839692a 100644
--- a/app/helpers/time_helper.rb
+++ b/app/helpers/time_helper.rb
@@ -15,20 +15,9 @@ module TimeHelper
     "#{from.to_s(:short)} - #{to.to_s(:short)}"
   end
 
-  def duration_in_numbers(finished_at, started_at)
-    interval = interval_in_seconds(started_at, finished_at)
-    time_format = interval < 1.hour ? "%M:%S" : "%H:%M:%S"
+  def duration_in_numbers(duration)
+    time_format = duration < 1.hour ? "%M:%S" : "%H:%M:%S"
 
-    Time.at(interval).utc.strftime(time_format)
-  end
-
-  private
-
-  def interval_in_seconds(started_at, finished_at = nil)
-    if started_at && finished_at
-      finished_at.to_i - started_at.to_i
-    elsif started_at
-      Time.now.to_i - started_at.to_i
-    end
+    Time.at(duration).utc.strftime(time_format)
   end
 end
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index c360a6ff729..0f0bfd4ee31 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -78,6 +78,10 @@ module Ci
       CommitStatus.where(pipeline: pluck(:id)).stages
     end
 
+    def self.total_duration
+      where.not(duration: nil).sum(:duration)
+    end
+
     def stages_with_latest_statuses
       statuses.latest.order(:stage_idx).group_by(&:stage)
     end
@@ -250,7 +254,7 @@ module Ci
     end
 
     def update_duration
-      self.duration = statuses.latest.duration
+      self.duration = calculate_duration
     end
 
     def execute_hooks
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 703ca90edb6..7542399169f 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -21,6 +21,7 @@ class CommitStatus < ActiveRecord::Base
 
     where(id: max_id.group(:name, :commit_id))
   end
+
   scope :retried, -> { where.not(id: latest) }
   scope :ordered, -> { order(:name) }
   scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
@@ -107,13 +108,7 @@ class CommitStatus < ActiveRecord::Base
   end
 
   def duration
-    duration =
-      if started_at && finished_at
-        finished_at - started_at
-      elsif started_at
-        Time.now - started_at
-      end
-    duration
+    calculate_duration
   end
 
   def stuck?
diff --git a/app/models/concerns/statuseable.rb b/app/models/concerns/statuseable.rb
index 5d4b0a86899..750f937b724 100644
--- a/app/models/concerns/statuseable.rb
+++ b/app/models/concerns/statuseable.rb
@@ -35,11 +35,6 @@ module Statuseable
       all.pluck(self.status_sql).first
     end
 
-    def duration
-      duration_array = all.map(&:duration).compact
-      duration_array.reduce(:+)
-    end
-
     def started_at
       all.minimum(:started_at)
     end
@@ -85,4 +80,14 @@ module Statuseable
   def complete?
     COMPLETED_STATUSES.include?(status)
   end
+
+  private
+
+  def calculate_duration
+    if started_at && finished_at
+      finished_at - started_at
+    elsif started_at
+      Time.now - started_at
+    end
+  end
 end
diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml
index 352adbedee4..f29d9c94441 100644
--- a/app/views/admin/builds/_build.html.haml
+++ b/app/views/admin/builds/_build.html.haml
@@ -51,7 +51,7 @@
     - if build.duration
       %p.duration
         = custom_icon("icon_timer")
-        = duration_in_numbers(build.finished_at, build.started_at)
+        = duration_in_numbers(build.duration)
 
     - if build.finished_at
       %p.finished-at
diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml
index 91081435220..1fdf32466f2 100644
--- a/app/views/projects/ci/builds/_build.html.haml
+++ b/app/views/projects/ci/builds/_build.html.haml
@@ -63,7 +63,7 @@
     - if build.duration
       %p.duration
         = custom_icon("icon_timer")
-        = duration_in_numbers(build.finished_at, build.started_at)
+        = duration_in_numbers(build.duration)
     - if build.finished_at
       %p.finished-at
         = icon("calendar")
diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml
index be387201f8d..9a672b23341 100644
--- a/app/views/projects/ci/pipelines/_pipeline.html.haml
+++ b/app/views/projects/ci/pipelines/_pipeline.html.haml
@@ -48,10 +48,10 @@
             \-
 
   %td
-    - if pipeline.started_at && pipeline.finished_at
+    - if pipeline.duration
       %p.duration
         = custom_icon("icon_timer")
-        = duration_in_numbers(pipeline.finished_at, pipeline.started_at)
+        = duration_in_numbers(pipeline.duration)
     - if pipeline.finished_at
       %p.finished-at
         = icon("calendar")
diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml
index 14adee7a6e9..29d767e7769 100644
--- a/app/views/projects/commit/_commit_box.html.haml
+++ b/app/views/projects/commit/_commit_box.html.haml
@@ -58,9 +58,8 @@
       = ci_icon_for_status(@commit.status)
       %span.ci-status-label
         = ci_label_for_status(@commit.status)
-    - if @commit.pipelines.duration
-      in
-      = time_interval_in_words @commit.pipelines.duration
+    in
+    = time_interval_in_words @commit.pipelines.total_duration
 
 .commit-box.content-block
   %h3.commit-title
diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml
index 8289aefcde7..063e83a407a 100644
--- a/app/views/projects/pipelines/_info.html.haml
+++ b/app/views/projects/pipelines/_info.html.haml
@@ -9,7 +9,7 @@
     = link_to @pipeline.ref, namespace_project_commits_path(@project.namespace, @project, @pipeline.ref), class: "monospace"
   - if @pipeline.duration
     in
-    = time_interval_in_words @pipeline.duration
+    = time_interval_in_words(@pipeline.duration)
 
   .pull-right
     = link_to namespace_project_pipeline_path(@project.namespace, @project, @pipeline), class: "ci-status ci-#{@pipeline.status}" do
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index d13fe0ef8a9..e59ead5d76c 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -7,7 +7,7 @@ module Gitlab
     # @param  cmd [Array<String>]
     # @return [Boolean]
     def system_silent(cmd)
-      Popen::popen(cmd).last.zero?
+      Popen.popen(cmd).last.zero?
     end
 
     def force_utf8(str)
diff --git a/spec/helpers/time_helper_spec.rb b/spec/helpers/time_helper_spec.rb
index bf3ed5c094c..21f35585367 100644
--- a/spec/helpers/time_helper_spec.rb
+++ b/spec/helpers/time_helper_spec.rb
@@ -19,16 +19,16 @@ describe TimeHelper do
 
   describe "#duration_in_numbers" do
     it "returns minutes and seconds" do
-      duration_in_numbers = {
-        [100, 0] => "01:40",
-        [121, 0] => "02:01",
-        [3721, 0] => "01:02:01",
-        [0, 0] => "00:00",
-        [nil, Time.now.to_i - 42] => "00:42"
+      durations_and_expectations = {
+        100 => "01:40",
+        121 => "02:01",
+        3721 => "01:02:01",
+        0 => "00:00",
+        42 => "00:42"
       }
 
-      duration_in_numbers.each do |interval, expectation|
-        expect(duration_in_numbers(*interval)).to eq(expectation)
+      durations_and_expectations.each do |duration, expectation|
+        expect(duration_in_numbers(duration)).to eq(expectation)
       end
     end
   end
diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb
index 72688137f08..02d6263094a 100644
--- a/spec/models/broadcast_message_spec.rb
+++ b/spec/models/broadcast_message_spec.rb
@@ -1,8 +1,6 @@
 require 'spec_helper'
 
 describe BroadcastMessage, models: true do
-  include ActiveSupport::Testing::TimeHelpers
-
   subject { create(:broadcast_message) }
 
   it { is_expected.to be_valid }
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 8137e9f8f71..721b20e0cb2 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -124,17 +124,21 @@ describe Ci::Pipeline, models: true do
 
   describe 'state machine' do
     let(:current) { Time.now.change(usec: 0) }
-    let(:build) { create :ci_build, name: 'build1', pipeline: pipeline, started_at: current - 60, finished_at: current }
-    let(:build2) { create :ci_build, name: 'build2', pipeline: pipeline, started_at: current - 60, finished_at: current }
+    let(:build) { create :ci_build, name: 'build1', pipeline: pipeline }
 
     describe '#duration' do
       before do
-        build.skip
-        build2.skip
+        travel_to(current - 120) do
+          pipeline.run
+        end
+
+        travel_to(current) do
+          pipeline.succeed
+        end
       end
 
       it 'matches sum of builds duration' do
-        expect(pipeline.reload.duration).to eq(build.duration + build2.duration)
+        expect(pipeline.reload.duration).to eq(120)
       end
     end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 2e2aa7c4fc0..c144cd85487 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -33,6 +33,7 @@ RSpec.configure do |config|
   config.include EmailHelpers
   config.include TestEnv
   config.include ActiveJob::TestHelper
+  config.include ActiveSupport::Testing::TimeHelpers
   config.include StubGitlabCalls
   config.include StubGitlabData