From 21fdb80982ec67df7d3f0677ea2af4d3deff3c40 Mon Sep 17 00:00:00 2001
From: Anthony Dmitriyev <antstorm@gmail.com>
Date: Tue, 29 Jun 2021 19:47:20 +0100
Subject: [PATCH] Improve retryer specs

---
 lib/temporal/client/retryer.rb                | 26 +++++----
 .../client/{retryer.rb => retryer_spec.rb}    | 53 ++++++++-----------
 2 files changed, 35 insertions(+), 44 deletions(-)
 rename spec/unit/lib/temporal/client/{retryer.rb => retryer_spec.rb} (67%)

diff --git a/lib/temporal/client/retryer.rb b/lib/temporal/client/retryer.rb
index fe925093..426470bd 100644
--- a/lib/temporal/client/retryer.rb
+++ b/lib/temporal/client/retryer.rb
@@ -9,19 +9,17 @@ module Retryer
       # List pulled from RpcRetryOptions in the Java SDK
       # https://github.com/temporalio/sdk-java/blob/ad8831d4a4d9d257baf3482ab49f1aa681895c0e/temporal-serviceclient/src/main/java/io/temporal/serviceclient/RpcRetryOptions.java#L32
       # No amount of retrying will help in these cases.
-      def self.do_not_retry_errors
-        [
-          GRPC::AlreadyExists,
-          GRPC::Cancelled,
-          GRPC::FailedPrecondition,
-          GRPC::InvalidArgument,
-          # If the activity has timed out, the server will return this and will never accept a retry
-          GRPC::NotFound,
-          GRPC::PermissionDenied,
-          GRPC::Unauthenticated,
-          GRPC::Unimplemented,
-        ]
-      end
+      DO_NOT_RETRY_ERRORS = [
+        GRPC::AlreadyExists,
+        GRPC::Cancelled,
+        GRPC::FailedPrecondition,
+        GRPC::InvalidArgument,
+        # If the activity has timed out, the server will return this and will never accept a retry
+        GRPC::NotFound,
+        GRPC::PermissionDenied,
+        GRPC::Unauthenticated,
+        GRPC::Unimplemented,
+      ].freeze
 
       # Used for backoff retries in certain cases when calling temporal server.
       # on_retry - a proc that's executed each time you need to retry
@@ -33,7 +31,7 @@ def self.with_retries(times: DEFAULT_RETRIES, on_retry: nil, &block)
         loop do
           begin
             return yield
-          rescue *do_not_retry_errors
+          rescue *DO_NOT_RETRY_ERRORS
             raise
           rescue => e
             raise e if retry_i >= times
diff --git a/spec/unit/lib/temporal/client/retryer.rb b/spec/unit/lib/temporal/client/retryer_spec.rb
similarity index 67%
rename from spec/unit/lib/temporal/client/retryer.rb
rename to spec/unit/lib/temporal/client/retryer_spec.rb
index a33a29b8..e8e51b2a 100644
--- a/spec/unit/lib/temporal/client/retryer.rb
+++ b/spec/unit/lib/temporal/client/retryer_spec.rb
@@ -78,37 +78,30 @@
     expect(on_retry_calls).to equal(retries)
   end
 
-  {
-    GRPC::AlreadyExists => false,
-    GRPC::Cancelled => false,
-    GRPC::FailedPrecondition => false,
-    GRPC::InvalidArgument => false,
-    GRPC::NotFound => false,
-    GRPC::PermissionDenied => false,
-    GRPC::Unauthenticated => false,
-    GRPC::Unimplemented => false,
-    StandardError => true,
-    GRPC::Unknown => true,
-    GRPC::Unavailable => true
-  }.each do |error_class, expect_retry|
-    it "#{expect_retry ? 'does' : 'does not'} retry #{error_class}" do
-      on_retry_calls = 0
-      retried = false
-      on_retry = Proc.new {
-        on_retry_calls += 1
-      }
-
-      begin
-        described_class.with_retries(on_retry: on_retry) do
-          if !retried
-            retried = true
-            raise error_class.new("nope")
+  described_class::DO_NOT_RETRY_ERRORS.each do |error_class|
+    context "when #{error_class} is raised" do
+      let(:error) { error_class.new('nope') }
+      let(:on_retry) { Proc.new {} }
+
+      it 'does not retry' do
+        calls = 0
+
+        expect do
+          described_class.with_retries do
+            calls += 1
+            raise error
           end
-        end
-      rescue => e
-        expect(e.class).to eq(error_class)
-      ensure
-        expect(on_retry_calls).to equal(expect_retry ? 1 : 0)
+        end.to raise_error(error)
+
+        expect(calls).to eq(1)
+      end
+
+      it 'does not call on_retry' do
+        allow(on_retry).to receive(:call).and_call_original
+
+        described_class.with_retries(on_retry: on_retry) { raise error } rescue nil
+
+        expect(on_retry).not_to have_received(:call)
       end
     end
   end