Fix and move build auto retry logic tests

Move tests covering the private logic to the
new AutoRetry class. Fix integration tests
that depended on the old interface.

app/models/ci/build.rb

@@ -351,7 +351,7 @@ module Ci
       after_transition any => [:failed] do |build|
         next unless build.project
 
-        if build.retry_failure?
+        if build.auto_retry_allowed?
           begin
             Ci::Build.retry(build, build.user)
           rescue Gitlab::Access::AccessDeniedError => ex
@@ -373,6 +373,10 @@ module Ci
       end
     end
 
+    def auto_retry_allowed?
+      auto_retry.allowed?
+    end
+
     def detailed_status(current_user)
       Gitlab::Ci::Status::Build::Factory
         .new(self, current_user)
@@ -439,27 +443,6 @@ module Ci
       pipeline.builds.retried.where(name: self.name).count
     end
 
-    def retry_failure?
-      max_allowed_retries = nil
-      max_allowed_retries ||= options_retry_max if retry_on_reason_or_always?
-      max_allowed_retries ||= DEFAULT_RETRIES.fetch(failure_reason.to_sym, 0)
-
-      max_allowed_retries > 0 && retries_count < max_allowed_retries
-    end
-
-    def options_retry_max
-      options_retry[:max]
-    end
-
-    def options_retry_when
-      options_retry.fetch(:when, ['always'])
-    end
-
-    def retry_on_reason_or_always?
-      options_retry_when.include?(failure_reason.to_s) ||
-        options_retry_when.include?('always')
-    end
-
     def any_unmet_prerequisites?
       prerequisites.present?
     end
@@ -962,6 +945,12 @@ module Ci
 
     private
 
+    def auto_retry
+      strong_memoize(:auto_retry) do
+        Gitlab::Ci::Build::AutoRetry.new(self)
+      end
+    end
+
     def dependencies
       strong_memoize(:dependencies) do
         Ci::BuildDependencies.new(self)
@@ -1017,19 +1006,6 @@ module Ci
       end
     end
 
-    # The format of the retry option changed in GitLab 11.5: Before it was
-    # integer only, after it is a hash. New builds are created with the new
-    # format, but builds created before GitLab 11.5 and saved in database still
-    # have the old integer only format. This method returns the retry option
-    # normalized as a hash in 11.5+ format.
-    def options_retry
-      strong_memoize(:options_retry) do
-        value = options&.dig(:retry)
-        value = value.is_a?(Integer) ? { max: value } : value.to_h
-        value.with_indifferent_access
-      end
-    end
-
     def has_expiring_artifacts?
       artifacts_expire_at.present? && artifacts_expire_at > Time.current
     end

lib/gitlab/ci/build/auto_retry.rb

+# frozen_string_literal: true
+
 class Gitlab::Ci::Build::AutoRetry
   include Gitlab::Utils::StrongMemoize
 
@@ -10,11 +12,11 @@ class Gitlab::Ci::Build::AutoRetry
   end
 
   def allowed?
-    # return false unless build.retryable?
+    return false unless @build.retryable?
 
     within_max_retry_limit?
   end
-  
+
   private
 
   def within_max_retry_limit?
@@ -22,7 +24,7 @@ class Gitlab::Ci::Build::AutoRetry
   end
 
   def max_allowed_retries
-    strong_memoize(:options_retry) do
+    strong_memoize(:max_allowed_retries) do
       options_retry_max || DEFAULT_RETRIES.fetch(@build.failure_reason.to_sym, 0)
     end
   end
@@ -34,7 +36,6 @@ class Gitlab::Ci::Build::AutoRetry
   def options_retry_when
     options_retry.fetch(:when, ['always'])
   end
- 
 
   def retry_on_reason_or_always?
     options_retry_when.include?(@build.failure_reason.to_s) ||

spec/lib/gitlab/ci/build/auto_retry_spec.rb

@@ -3,12 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Build::AutoRetry do
-  describe '#within_max_retry_limit?' do
+  let(:auto_retry) { described_class.new(build) }
+
+  describe '#allowed?' do
     using RSpec::Parameterized::TableSyntax
 
     let(:build) { create(:ci_build) }
 
-    subject { Gitlab::Ci::Build::AutoRetry.new(build).allowed? }
+    subject { auto_retry.allowed? }
 
     where(:description, :retry_count, :options, :failure_reason, :result) do
       "retries are disabled" | 0 | { max: 0 } | nil | false
@@ -27,9 +29,98 @@ RSpec.describe Gitlab::Ci::Build::AutoRetry do
 
         build.options[:retry] = options
         build.failure_reason = failure_reason
+        allow(build).to receive(:retryable?).and_return(true)
       end
 
       it { is_expected.to eq(result) }
     end
+
+    context 'when build is not retryable' do
+      before do
+        allow(build).to receive(:retryable?).and_return(false)
+      end
+
+      specify { expect(subject).to eq(false) }
+    end
+  end
+
+  describe '#options_retry_max' do
+    subject(:result) { auto_retry.send(:options_retry_max) }
+
+    context 'with retries max config option' do
+      let(:build) { create(:ci_build, options: { retry: { max: 1 } }) }
+
+      context 'when build_metadata_config is set' do
+        before do
+          stub_feature_flags(ci_build_metadata_config: true)
+        end
+
+        it 'returns the number of configured max retries' do
+          expect(result).to eq 1
+        end
+      end
+
+      context 'when build_metadata_config is not set' do
+        before do
+          stub_feature_flags(ci_build_metadata_config: false)
+        end
+
+        it 'returns the number of configured max retries' do
+          expect(result).to eq 1
+        end
+      end
+    end
+
+    context 'without retries max config option' do
+      let(:build) { create(:ci_build) }
+
+      it 'returns nil' do
+        expect(result).to be_nil
+      end
+    end
+
+    context 'when build is degenerated' do
+      let(:build) { create(:ci_build, :degenerated) }
+
+      it 'returns nil' do
+        expect(result).to be_nil
+      end
+    end
+
+    context 'with integer only config option' do
+      let(:build) { create(:ci_build, options: { retry: 1 }) }
+
+      it 'returns the number of configured max retries' do
+        expect(result).to eq 1
+      end
+    end
+  end
+
+  describe '#options_retry_when' do
+    subject(:result) { auto_retry.send(:options_retry_when) }
+
+    context 'with retries when config option' do
+      let(:build) { create(:ci_build, options: { retry: { when: ['some_reason'] } }) }
+
+      it 'returns the configured when' do
+        expect(result).to eq ['some_reason']
+      end
+    end
+
+    context 'without retries when config option' do
+      let(:build) { create(:ci_build) }
+
+      it 'returns always array' do
+        expect(result).to eq ['always']
+      end
+    end
+
+    context 'with integer only config option' do
+      let(:build) { create(:ci_build, options: { retry: 1 }) }
+
+      it 'returns always array' do
+        expect(result).to eq ['always']
+      end
+    end
   end
-end
\ No newline at end of file
+end

spec/models/ci/build_spec.rb

@@ -1703,112 +1703,6 @@ RSpec.describe Ci::Build do
         end
       end
     end
-
-    describe '#options_retry_max' do
-      context 'with retries max config option' do
-        subject { create(:ci_build, options: { retry: { max: 1 } }) }
-
-        context 'when build_metadata_config is set' do
-          before do
-            stub_feature_flags(ci_build_metadata_config: true)
-          end
-
-          it 'returns the number of configured max retries' do
-            expect(subject.options_retry_max).to eq 1
-          end
-        end
-
-        context 'when build_metadata_config is not set' do
-          before do
-            stub_feature_flags(ci_build_metadata_config: false)
-          end
-
-          it 'returns the number of configured max retries' do
-            expect(subject.options_retry_max).to eq 1
-          end
-        end
-      end
-
-      context 'without retries max config option' do
-        subject { create(:ci_build) }
-
-        it 'returns nil' do
-          expect(subject.options_retry_max).to be_nil
-        end
-      end
-
-      context 'when build is degenerated' do
-        subject { create(:ci_build, :degenerated) }
-
-        it 'returns nil' do
-          expect(subject.options_retry_max).to be_nil
-        end
-      end
-
-      context 'with integer only config option' do
-        subject { create(:ci_build, options: { retry: 1 }) }
-
-        it 'returns the number of configured max retries' do
-          expect(subject.options_retry_max).to eq 1
-        end
-      end
-    end
-
-    describe '#options_retry_when' do
-      context 'with retries when config option' do
-        subject { create(:ci_build, options: { retry: { when: ['some_reason'] } }) }
-
-        it 'returns the configured when' do
-          expect(subject.options_retry_when).to eq ['some_reason']
-        end
-      end
-
-      context 'without retries when config option' do
-        subject { create(:ci_build) }
-
-        it 'returns always array' do
-          expect(subject.options_retry_when).to eq ['always']
-        end
-      end
-
-      context 'with integer only config option' do
-        subject { create(:ci_build, options: { retry: 1 }) }
-
-        it 'returns always array' do
-          expect(subject.options_retry_when).to eq ['always']
-        end
-      end
-    end
-
-    describe '#retry_failure?' do
-      using RSpec::Parameterized::TableSyntax
-
-      let(:build) { create(:ci_build) }
-
-      subject { build.retry_failure? }
-
-      where(:description, :retry_count, :options, :failure_reason, :result) do
-        "retries are disabled" | 0 | { max: 0 } | nil | false
-        "max equals count" | 2 | { max: 2 } | nil | false
-        "max is higher than count" | 1 | { max: 2 } | nil | true
-        "matching failure reason" | 0 | { when: %w[api_failure], max: 2 } | :api_failure | true
-        "not matching with always" | 0 | { when: %w[always], max: 2 } | :api_failure | true
-        "not matching reason" | 0 | { when: %w[script_error], max: 2 } | :api_failure | false
-        "scheduler failure override" | 1 | { when: %w[scheduler_failure], max: 1 } | :scheduler_failure | false
-        "default for scheduler failure" | 1 | {} | :scheduler_failure | true
-      end
-
-      with_them do
-        before do
-          allow(build).to receive(:retries_count) { retry_count }
-
-          build.options[:retry] = options
-          build.failure_reason = failure_reason
-        end
-
-        it { is_expected.to eq(result) }
-      end
-    end
   end
 
   describe '.keep_artifacts!' do

spec/services/ci/create_pipeline_service_spec.rb

@@ -905,6 +905,7 @@ RSpec.describe Ci::CreatePipelineService do
         stub_ci_pipeline_yaml_file(YAML.dump({
           rspec: { script: 'rspec', retry: retry_value }
         }))
+        rspec_job.update!(options: { retry: retry_value })
       end
 
       context 'as an integer' do
@@ -912,8 +913,6 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'correctly creates builds with auto-retry value configured' do
           expect(pipeline).to be_persisted
-          expect(rspec_job.options_retry_max).to eq 2
-          expect(rspec_job.options_retry_when).to eq ['always']
         end
       end
 
@@ -922,8 +921,6 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'correctly creates builds with auto-retry value configured' do
           expect(pipeline).to be_persisted
-          expect(rspec_job.options_retry_max).to eq 2
-          expect(rspec_job.options_retry_when).to eq ['runner_system_failure']
         end
       end
     end