1 files changed, 106 insertions, 14 deletions

diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb
index 3d8c377ab21..c3484c4a42c 100644
--- a/spec/models/hooks/project_hook_spec.rb
+++ b/spec/models/hooks/project_hook_spec.rb
@@ -2,7 +2,19 @@
 
 require 'spec_helper'
 
-RSpec.describe ProjectHook do
+RSpec.describe ProjectHook, feature_category: :integrations do
+  include_examples 'a hook that gets automatically disabled on failure' do
+    let_it_be(:project) { create(:project) }
+
+    let(:hook) { build(:project_hook, project: project) }
+    let(:hook_factory) { :project_hook }
+    let(:default_factory_arguments) { { project: project } }
+
+    def find_hooks
+      project.hooks
+    end
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to :project }
   end
@@ -67,17 +79,91 @@ RSpec.describe ProjectHook do
   end
 
   describe '#update_last_failure', :clean_gitlab_redis_shared_state do
-    let(:hook) { build(:project_hook) }
+    let_it_be(:hook) { create(:project_hook) }
+
+    def last_failure
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.get(hook.project.last_failure_redis_key)
+      end
+    end
+
+    def any_failed?
+      Gitlab::Redis::SharedState.with do |redis|
+        Gitlab::Utils.to_boolean(redis.get(hook.project.web_hook_failure_redis_key))
+      end
+    end
 
     it 'is a method of this class' do
       expect { hook.update_last_failure }.not_to raise_error
     end
 
     context 'when the hook is executable' do
-      it 'does not update the state' do
-        expect(Gitlab::Redis::SharedState).not_to receive(:with)
+      let(:redis_key) { hook.project.web_hook_failure_redis_key }
+
+      def redis_value
+        any_failed?
+      end
+
+      context 'when the state was previously failing' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(redis_key, true)
+          end
+        end
+
+        it 'does update the state' do
+          expect { hook.update_last_failure }.to change { redis_value }.to(false)
+        end
+
+        context 'when there is another failing sibling hook' do
+          before do
+            create(:project_hook, :permanently_disabled, project: hook.project)
+          end
+
+          it 'does not update the state' do
+            expect { hook.update_last_failure }.not_to change { redis_value }.from(true)
+          end
+
+          it 'caches the current value' do
+            Gitlab::Redis::SharedState.with do |redis|
+              expect(redis).to receive(:set).with(redis_key, 'true', ex: 1.hour).and_call_original
+            end
+
+            hook.update_last_failure
+          end
+        end
+      end
+
+      context 'when the state was previously unknown' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.del(redis_key)
+          end
+        end
+
+        it 'does not update the state' do
+          expect { hook.update_last_failure }.not_to change { redis_value }.from(nil)
+        end
+      end
+
+      context 'when the state was previously not failing' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(redis_key, false)
+          end
+        end
 
-        hook.update_last_failure
+        it 'does not update the state' do
+          expect { hook.update_last_failure }.not_to change { redis_value }.from(false)
+        end
+
+        it 'does not cache the current value' do
+          Gitlab::Redis::SharedState.with do |redis|
+            expect(redis).not_to receive(:set)
+          end
+
+          hook.update_last_failure
+        end
       end
     end
 
@@ -86,28 +172,34 @@ RSpec.describe ProjectHook do
         allow(hook).to receive(:executable?).and_return(false)
       end
 
-      def last_failure
-        Gitlab::Redis::SharedState.with do |redis|
-          redis.get("web_hooks:last_failure:project-#{hook.project.id}")
-        end
-      end
-
       context 'there is no prior value', :freeze_time do
-        it 'updates the state' do
+        it 'updates last_failure' do
           expect { hook.update_last_failure }.to change { last_failure }.to(Time.current)
         end
+
+        it 'updates any_failed?' do
+          expect { hook.update_last_failure }.to change { any_failed? }.to(true)
+        end
       end
 
-      context 'there is a prior value, from before now' do
+      context 'when there is a prior last_failure, from before now' do
         it 'updates the state' do
           the_future = 1.minute.from_now
-
           hook.update_last_failure
 
           travel_to(the_future) do
             expect { hook.update_last_failure }.to change { last_failure }.to(the_future.iso8601)
           end
         end
+
+        it 'does not change the failing state' do
+          the_future = 1.minute.from_now
+          hook.update_last_failure
+
+          travel_to(the_future) do
+            expect { hook.update_last_failure }.not_to change { any_failed? }.from(true)
+          end
+        end
       end
 
       context 'there is a prior value, from after now' do