From 3e7f3bc7980018910f6c6e7a19f706ad18ab2793 Mon Sep 17 00:00:00 2001
From: Tomasz Maczukin <tomasz@maczukin.pl>
Date: Mon, 26 Feb 2018 18:51:56 +0100
Subject: [PATCH] Refactor timeout selection mechanism

---
 app/models/ci/build.rb       | 10 ++------
 spec/models/ci/build_spec.rb | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index f47cbe0a206..f9a83965199 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -124,7 +124,7 @@ module Ci
 
       after_transition pending: :running do |build|
         build.used_timeout = build.timeout
-        build.timeout_source = build.should_use_runner_timeout? ? 'Runner' : 'Project'
+        build.timeout_source = build.timeout < build.project.build_timeout ? 'Runner' : 'Project'
         build.save!
 
         build.run_after_commit do
@@ -239,13 +239,7 @@ module Ci
     end
 
     def timeout
-      return runner.maximum_job_timeout if should_use_runner_timeout?
-
-      project.build_timeout
-    end
-
-    def should_use_runner_timeout?
-      !runner.nil? && runner.defines_maximum_job_timeout? && runner.maximum_job_timeout < project.build_timeout
+      [project.build_timeout, runner&.maximum_job_timeout].compact.min
     end
 
     def triggered_by?(current_user)
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index cceb2033cbb..78842bd8a92 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2036,6 +2036,52 @@ describe Ci::Build do
     end
   end
 
+  describe 'state transition: pending: :running' do
+    let(:runner) { create(:ci_runner) }
+    let(:job) { create(:ci_build, :pending, runner: runner) }
+
+    before do
+      job.project.build_timeout = 1800
+      job.project.save!
+    end
+
+    shared_examples 'saves data on transition' do
+      it 'saves used_timeout and timeout_source on transition' do
+        expect(job.used_timeout).to be_nil
+        expect(job.timeout_source).to be_nil
+
+        job.run!
+
+        expect(job.used_timeout).to eq(expected_timeout)
+        expect(job.timeout_source).to eq(expected_timeout_source)
+      end
+    end
+
+    context 'when runner timeout overrides project timeout' do
+      let(:expected_timeout) { 900 }
+      let(:expected_timeout_source) { 'Runner' }
+
+      before do
+        runner.maximum_job_timeout = 900
+        runner.save!
+      end
+
+      it_behaves_like 'saves data on transition'
+    end
+
+    context "when runner timeout doesn't override project timeout" do
+      let(:expected_timeout) { 1800 }
+      let(:expected_timeout_source) { 'Project' }
+
+      before do
+        runner.maximum_job_timeout = 3600
+        runner.save!
+      end
+
+      it_behaves_like 'saves data on transition'
+    end
+  end
+
   describe 'state transition: any => [:running]' do
     shared_examples 'validation is active' do
       context 'when depended job has not been completed yet' do