From a6a920ce7b269c11d1fe9576ff8e73c61cd73336 Mon Sep 17 00:00:00 2001
From: Jean Boussier <jean.boussier@gmail.com>
Date: Fri, 23 Aug 2024 12:54:15 +0200
Subject: [PATCH] Refactor test suite so all test case inherit from a single
 class

Also cleanup a few tests that were assertion-less etc.
---
 test/env.rb                               |  1 +
 test/hiredis/test_helper.rb               |  2 +-
 test/redis_client/circuit_breaker_test.rb |  3 ++-
 test/redis_client/command_builder_test.rb |  2 +-
 test/redis_client/config_test.rb          |  2 +-
 test/redis_client/connection_test.rb      |  8 ++++----
 test/redis_client/decorator_test.rb       |  4 ++--
 test/redis_client/middlewares_test.rb     |  2 +-
 test/redis_client/pooled_test.rb          |  2 +-
 test/redis_client/ractor_test.rb          | 15 +++++++++++----
 test/redis_client/resp3_test.rb           |  2 +-
 test/redis_client/subscriptions_test.rb   |  2 +-
 test/redis_client_test.rb                 | 10 +++++-----
 test/sentinel/sentinel_test.rb            |  2 +-
 test/sentinel/test_helper.rb              |  1 +
 test/shared/redis_client_tests.rb         | 10 ++++++----
 test/support/client_test_helper.rb        | 22 +++++++++++++++-------
 test/support/server_manager.rb            |  2 +-
 test/support/servers.rb                   | 12 +++++-------
 test/test_helper.rb                       |  3 +++
 20 files changed, 64 insertions(+), 43 deletions(-)

diff --git a/test/env.rb b/test/env.rb
index 7d4ab46..dabc643 100644
--- a/test/env.rb
+++ b/test/env.rb
@@ -7,6 +7,7 @@
 require "redis_client/decorator"
 
 require "toxiproxy"
+require "stringio"
 
 Dir[File.join(__dir__, "support/**/*.rb")].sort.each { |f| require f }
 Dir[File.join(__dir__, "shared/**/*.rb")].sort.each { |f| require f }
diff --git a/test/hiredis/test_helper.rb b/test/hiredis/test_helper.rb
index b32fc4d..3b8b341 100644
--- a/test/hiredis/test_helper.rb
+++ b/test/hiredis/test_helper.rb
@@ -8,7 +8,7 @@
 unless RUBY_PLATFORM == "java"
   require "redis"
   require "hiredis"
-  Redis.new(host: Servers::HOST, port: Servers::REDIS_TCP_PORT, driver: :hiredis).ping
+  Redis.new(host: Servers::HOST, port: Servers::REDIS.port, driver: :hiredis).ping
 end
 
 require "hiredis-client"
diff --git a/test/redis_client/circuit_breaker_test.rb b/test/redis_client/circuit_breaker_test.rb
index d744a5b..aadf33b 100644
--- a/test/redis_client/circuit_breaker_test.rb
+++ b/test/redis_client/circuit_breaker_test.rb
@@ -3,10 +3,11 @@
 require "test_helper"
 
 class RedisClient
-  class CircuitBreakerTest < Minitest::Test
+  class CircuitBreakerTest < RedisClientTestCase
     include ClientTestHelper
 
     def setup
+      super
       @circuit_breaker = CircuitBreaker.new(
         error_threshold: 3,
         error_threshold_timeout: 2,
diff --git a/test/redis_client/command_builder_test.rb b/test/redis_client/command_builder_test.rb
index b9bcda7..0d36f7c 100644
--- a/test/redis_client/command_builder_test.rb
+++ b/test/redis_client/command_builder_test.rb
@@ -3,7 +3,7 @@
 require "test_helper"
 
 class RedisClient
-  class CommandBuilderTest < Minitest::Test
+  class CommandBuilderTest < RedisClientTestCase
     def test_positional
       assert_equal ["a", "b", "c"], call("a", "b", "c")
     end
diff --git a/test/redis_client/config_test.rb b/test/redis_client/config_test.rb
index 68eaac1..74c1085 100644
--- a/test/redis_client/config_test.rb
+++ b/test/redis_client/config_test.rb
@@ -3,7 +3,7 @@
 require "test_helper"
 
 class RedisClient
-  class ConfigTest < Minitest::Test
+  class ConfigTest < RedisClientTestCase
     def test_simple_uri
       config = Config.new(url: "redis://example.com")
       assert_equal "example.com", config.host
diff --git a/test/redis_client/connection_test.rb b/test/redis_client/connection_test.rb
index 5e8a0d5..1e55e17 100644
--- a/test/redis_client/connection_test.rb
+++ b/test/redis_client/connection_test.rb
@@ -282,7 +282,7 @@ def test_oom_errors
 
     private
 
-    def assert_timeout(error, faster_than = 0.5, &block)
+    def assert_timeout(error, faster_than = ClientTestHelper::DEFAULT_TIMEOUT + 0.2, &block)
       realtime = Benchmark.realtime do
         assert_raises(error, &block)
       end
@@ -291,7 +291,7 @@ def assert_timeout(error, faster_than = 0.5, &block)
     end
   end
 
-  class TCPConnectionTest < Minitest::Test
+  class TCPConnectionTest < RedisClientTestCase
     include ClientTestHelper
     include ConnectionTests
 
@@ -431,7 +431,7 @@ def new_client(**overrides)
     end
   end
 
-  class SSLConnectionTest < Minitest::Test
+  class SSLConnectionTest < RedisClientTestCase
     include ClientTestHelper
     include ConnectionTests
 
@@ -449,7 +449,7 @@ def new_client(**overrides)
     end
   end
 
-  class UnixConnectionTest < Minitest::Test
+  class UnixConnectionTest < RedisClientTestCase
     include ClientTestHelper
 
     def test_connection_working
diff --git a/test/redis_client/decorator_test.rb b/test/redis_client/decorator_test.rb
index b6a30f5..611895a 100644
--- a/test/redis_client/decorator_test.rb
+++ b/test/redis_client/decorator_test.rb
@@ -41,7 +41,7 @@ def test_client_methods_not_available_on_pipelines
     end
   end
 
-  class DecoratorTest < Minitest::Test
+  class DecoratorTest < RedisClientTestCase
     include DecoratorTests
 
     private
@@ -51,7 +51,7 @@ def new_client(**overrides)
     end
   end
 
-  class PooledDecoratorTest < Minitest::Test
+  class PooledDecoratorTest < RedisClientTestCase
     include DecoratorTests
 
     private
diff --git a/test/redis_client/middlewares_test.rb b/test/redis_client/middlewares_test.rb
index 7c2b350..1b317e1 100644
--- a/test/redis_client/middlewares_test.rb
+++ b/test/redis_client/middlewares_test.rb
@@ -3,7 +3,7 @@
 require "test_helper"
 
 class RedisClient
-  class MiddlewaresTest < Minitest::Test
+  class MiddlewaresTest < RedisClientTestCase
     include ClientTestHelper
 
     def setup
diff --git a/test/redis_client/pooled_test.rb b/test/redis_client/pooled_test.rb
index 8f4b6fb..31ae818 100644
--- a/test/redis_client/pooled_test.rb
+++ b/test/redis_client/pooled_test.rb
@@ -2,7 +2,7 @@
 
 require "test_helper"
 
-class RedisPooledClientTest < Minitest::Test
+class RedisPooledClientTest < RedisClientTestCase
   include ClientTestHelper
   include RedisClientTests
 
diff --git a/test/redis_client/ractor_test.rb b/test/redis_client/ractor_test.rb
index 1865f03..5ee627f 100644
--- a/test/redis_client/ractor_test.rb
+++ b/test/redis_client/ractor_test.rb
@@ -2,7 +2,7 @@
 
 require "test_helper"
 
-class RactorTest < Minitest::Test
+class RactorTest < RedisClientTestCase
   def setup
     skip("Ractors are not supported on this Ruby version") unless defined?(::Ractor)
     skip("Hiredis is not Ractor safe") if RedisClient.default_driver.name == "RedisClient::HiredisConnection"
@@ -11,32 +11,39 @@ def setup
     rescue Ractor::RemoteError
       skip("Ractor implementation is too limited (MRI 3.0?)")
     end
+    super
   end
 
   def test_get_and_set_within_ractor
     ractor = Ractor.new do
-      within_ractor_redis = ClientTestHelper.new_client
+      config = Ractor.receive
+      within_ractor_redis = RedisClient.new(**config)
       within_ractor_redis.call("SET", "foo", "bar")
       within_ractor_redis.call("GET", "foo")
     end
+    ractor.send(ClientTestHelper.tcp_config.freeze)
 
     assert_equal("bar", ractor.take)
   end
 
   def test_multiple_ractors
     ractor1 = Ractor.new do
-      within_ractor_redis = ClientTestHelper.new_client
+      config = Ractor.receive
+      within_ractor_redis = RedisClient.new(**config)
       within_ractor_redis.call("SET", "foo", "bar")
       within_ractor_redis.call("GET", "foo")
     end
+    ractor1.send(ClientTestHelper.tcp_config.freeze)
 
     ractor1.take # We do this to ensure that the SET has been processed
 
     ractor2 = Ractor.new do
+      config = Ractor.receive
+      within_ractor_redis = RedisClient.new(**config)
       key = Ractor.receive
-      within_ractor_redis = ClientTestHelper.new_client
       within_ractor_redis.call("GET", key)
     end
+    ractor2.send(ClientTestHelper.tcp_config.freeze)
     ractor2.send("foo")
 
     assert_equal("bar", ractor2.take)
diff --git a/test/redis_client/resp3_test.rb b/test/redis_client/resp3_test.rb
index a549806..0be0580 100644
--- a/test/redis_client/resp3_test.rb
+++ b/test/redis_client/resp3_test.rb
@@ -6,7 +6,7 @@
 require "redis_client/ruby_connection/resp3"
 
 class RedisClient
-  class RESP3Test < Minitest::Test
+  class RESP3Test < RedisClientTestCase
     class StringIO < ::StringIO
       def skip(offset)
         seek(offset, IO::SEEK_CUR)
diff --git a/test/redis_client/subscriptions_test.rb b/test/redis_client/subscriptions_test.rb
index 83462f1..2c6c41c 100644
--- a/test/redis_client/subscriptions_test.rb
+++ b/test/redis_client/subscriptions_test.rb
@@ -3,7 +3,7 @@
 require "test_helper"
 
 class RedisClient
-  class SubscriptionsTest < Minitest::Test
+  class SubscriptionsTest < RedisClientTestCase
     include ClientTestHelper
 
     def setup
diff --git a/test/redis_client_test.rb b/test/redis_client_test.rb
index dc50a55..8b1e361 100644
--- a/test/redis_client_test.rb
+++ b/test/redis_client_test.rb
@@ -2,7 +2,7 @@
 
 require "test_helper"
 
-class RedisClientTest < Minitest::Test
+class RedisClientTest < RedisClientTestCase
   include ClientTestHelper
   include RedisClientTests
 
@@ -127,11 +127,11 @@ def test_measure_round_trip_delay
   end
 
   def test_server_url
-    assert_equal "redis://#{Servers::HOST}:#{Servers::REDIS_TCP_PORT}", @redis.server_url
+    assert_equal "redis://#{Servers::HOST}:#{Servers::REDIS.port}", @redis.server_url
   end
 
   def test_timeout
-    assert_equal 0.1, @redis.timeout
+    assert_equal ClientTestHelper::DEFAULT_TIMEOUT, @redis.timeout
   end
 
   def test_db
@@ -147,12 +147,12 @@ def test_host
   end
 
   def test_port
-    assert_equal Servers::REDIS_TCP_PORT, @redis.port
+    assert_equal Servers::REDIS.port, @redis.port
   end
 
   def test_path
     client = new_client(**unix_config)
-    assert_equal Servers::REDIS_SOCKET_FILE.to_s, client.path
+    assert_equal Servers::REDIS.socket_file.to_s, client.path
   end
 
   def test_username
diff --git a/test/sentinel/sentinel_test.rb b/test/sentinel/sentinel_test.rb
index 8de1138..ff086bb 100644
--- a/test/sentinel/sentinel_test.rb
+++ b/test/sentinel/sentinel_test.rb
@@ -3,7 +3,7 @@
 require "test_helper"
 
 class RedisClient
-  class SentinelTest < Minitest::Test
+  class SentinelTest < RedisClientTestCase
     include ClientTestHelper
 
     def setup
diff --git a/test/sentinel/test_helper.rb b/test/sentinel/test_helper.rb
index 09a9643..0e1c148 100644
--- a/test/sentinel/test_helper.rb
+++ b/test/sentinel/test_helper.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require_relative "../env"
+require_relative "../test_helper"
 
 Servers.build_redis
 Servers::SENTINEL_TESTS.prepare
diff --git a/test/shared/redis_client_tests.rb b/test/shared/redis_client_tests.rb
index 82e1d3b..0cc215c 100644
--- a/test/shared/redis_client_tests.rb
+++ b/test/shared/redis_client_tests.rb
@@ -188,20 +188,22 @@ def test_empty_pipeline
   end
 
   def test_large_read_pipelines
-    @redis.call("LPUSH", "list", *1000.times.to_a)
-    @redis.pipelined do |pipeline|
+    assert_equal 1000, @redis.call("LPUSH", "list", *1000.times.to_a)
+    results = @redis.pipelined do |pipeline|
       100.times do
         pipeline.call("LRANGE", "list", 0, -1)
       end
     end
+    assert_equal 100, results.size
   end
 
   def test_large_write_pipelines
-    @redis.pipelined do |pipeline|
+    results = @redis.pipelined do |pipeline|
       10.times do |i|
         pipeline.call("SET", i, i.to_s * 10485760)
       end
     end
+    assert_equal ["OK"] * 10, results
   end
 
   def test_get_set
@@ -570,7 +572,7 @@ def test_timeouts_are_adjustable_on_the_client
     @redis.read_timeout = 1
     @redis.write_timeout = 1
 
-    @redis.call("PING")
+    assert_equal "PONG", @redis.call("PING")
     @redis.connect_timeout = 2
     @redis.read_timeout = 2
     @redis.write_timeout = 2
diff --git a/test/support/client_test_helper.rb b/test/support/client_test_helper.rb
index 2b640b8..9d35bd8 100644
--- a/test/support/client_test_helper.rb
+++ b/test/support/client_test_helper.rb
@@ -39,12 +39,18 @@ def write_multi(commands)
   end
 
   def setup
+    super
+
     @redis = new_client
-    @redis.call("FLUSHDB")
+    check_server
   end
 
   private
 
+  def check_server
+    @redis.call("flushdb", "async")
+  end
+
   def travel(seconds)
     original_now = RedisClient.singleton_class.instance_method(:now)
     original_now_ms = RedisClient.singleton_class.instance_method(:now_ms)
@@ -90,19 +96,21 @@ def simulate_network_errors(client, failures)
 
   module_function
 
+  DEFAULT_TIMEOUT = 0.1
+
   def tcp_config
     {
       host: Servers::HOST,
-      port: Servers::REDIS_TCP_PORT,
-      timeout: 0.1,
+      port: Servers::REDIS.port,
+      timeout: DEFAULT_TIMEOUT,
     }
   end
 
   def ssl_config
     {
       host: Servers::HOST,
-      port: Servers::REDIS_TLS_PORT,
-      timeout: 0.1,
+      port: Servers::REDIS.tls_port,
+      timeout: DEFAULT_TIMEOUT,
       ssl: true,
       ssl_params: {
         cert: Servers::CERTS_PATH.join("client.crt").to_s,
@@ -114,8 +122,8 @@ def ssl_config
 
   def unix_config
     {
-      path: Servers::REDIS_SOCKET_FILE.to_s,
-      timeout: 0.1,
+      path: Servers::REDIS.socket_file.to_s,
+      timeout: DEFAULT_TIMEOUT,
     }
   end
 
diff --git a/test/support/server_manager.rb b/test/support/server_manager.rb
index 9914ae9..68e47fb 100644
--- a/test/support/server_manager.rb
+++ b/test/support/server_manager.rb
@@ -35,7 +35,7 @@ def spawn
     else
       pid_file.parent.mkpath
       @out.print "starting #{name}... "
-      pid = Process.spawn(*command, out: log_file.to_s, err: log_file.to_s)
+      pid = Process.spawn(*command.map(&:to_s), out: log_file.to_s, err: log_file.to_s)
       pid_file.write(pid.to_s)
       @out.puts "started with pid=#{pid}"
     end
diff --git a/test/support/servers.rb b/test/support/servers.rb
index b810d89..a24711e 100644
--- a/test/support/servers.rb
+++ b/test/support/servers.rb
@@ -11,12 +11,6 @@ module Servers
   HOST = "127.0.0.1"
   CERTS_PATH = ServerManager::ROOT.join("test/fixtures/certs")
 
-  REDIS_TCP_PORT = 16379
-  REDIS_TLS_PORT = 26379
-  REDIS_REAL_TCP_PORT = 16380
-  REDIS_REAL_TLS_PORT = 26380
-  REDIS_SOCKET_FILE = ServerManager::ROOT.join("tmp/redis.sock")
-
   SENTINEL_CONF_PATH = ServerManager::ROOT.join("tmp/sentinel.conf")
   SENTINEL_NAME = "cache"
 
@@ -45,10 +39,14 @@ def spawn
       super
     end
 
+    def socket_file
+      ServerManager::ROOT.join("tmp/redis.sock")
+    end
+
     def command
       [
         Servers.redis_server_bin,
-        "--unixsocket", REDIS_SOCKET_FILE.to_s,
+        "--unixsocket", socket_file,
         "--unixsocketperm", "700",
         "--port", real_port.to_s,
         "--tls-port", real_tls_port.to_s,
diff --git a/test/test_helper.rb b/test/test_helper.rb
index 725caf8..388bf31 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -13,3 +13,6 @@
 unless ENV["REDIS_CLIENT_RESTART_SERVER"] == "0"
   Minitest.after_run { Servers::TESTS.shutdown }
 end
+
+class RedisClientTestCase < Minitest::Test
+end