Zenspider zenspider/minitest cleanup (#3604)

* Clean up minitest usage: use a subclass of Minitest::Test, hook run_one_method not run, etc

This changes the test class in every file, so it looks ugly, but it
should make things cleaner in the long run.

test_helper.rb could use some more cleanup. It is still including into
Minitest::Test when it could just PUT everything in TimeoutTestCase
now.

* Mark the one as allowed

---------

Co-authored-by: Ryan Davis <ryand-ruby@zenspider.com>

.github/workflows/tests.yml

@@ -30,7 +30,6 @@ jobs:
       PUMA_TEST_DEBUG: true
       TESTOPTS: -v
       PUMA_NO_RUBOCOP: true
-      TERM: BugMinitest
 
     runs-on: ${{ matrix.os }}
     if: |
@@ -142,7 +141,6 @@ jobs:
       PUMA_TEST_DEBUG: true
       TESTOPTS: -v
       PUMA_NO_RUBOCOP: true
-      TERM: BugMinitest
 
     runs-on: ${{ matrix.os }}
     if: |

.rubocop.yml

@@ -1,5 +1,6 @@
 require:
   - rubocop-performance
+  - ./cops/tests_must_timeout.rb
 
 AllCops:
   DisabledByDefault: true
@@ -82,3 +83,6 @@ Style/TrailingCommaInArguments:
 
 Style/WhileUntilModifier:
   Enabled: true
+
+Puma/TestsMustTimeout:
+  Enabled: true

cops/tests_must_timeout.rb

@@ -0,0 +1,25 @@
+require 'rubocop'
+
+module RuboCop
+  module Cop
+    module Puma
+      class TestsMustTimeout < Base
+        extend AutoCorrector
+
+        MSG = 'Inherit from TimeoutTestCase instead of Minitest::Test'
+
+        def_node_matcher :inherits_from_minitest_test?, <<~PATTERN
+          (class _ (const (const nil? :Minitest) :Test) ...)
+        PATTERN
+
+        def on_class(node)
+          return unless inherits_from_minitest_test?(node)
+
+          add_offense(node.children[1]) do |corrector|
+            corrector.replace(node.children[1], 'TimeoutTestCase')
+          end
+        end
+      end
+    end
+  end
+end

test/helper.rb

@@ -87,68 +87,23 @@ def self.call(host = '127.0.0.1')
 
 require "timeout"
 
-if Minitest::VERSION < '5.25'
-  module TimeoutEveryTestCase
-    # our own subclass so we never confuse different timeouts
-    class TestTookTooLong < Timeout::Error
-    end
-
-    def run
-      with_info_handler do
-        time_it do
-          capture_exceptions do
-            ::Timeout.timeout($test_case_timeout, TestTookTooLong) do
-              before_setup; setup; after_setup
-              self.send self.name
-            end
-          end
-
-          capture_exceptions do
-            ::Timeout.timeout($test_case_timeout, TestTookTooLong) do
-              Minitest::Test::TEARDOWN_METHODS.each { |hook| self.send hook }
-            end
-          end
-          if respond_to? :clean_tmp_paths
-            clean_tmp_paths
-          end
-        end
-      end
+class TimeoutTestCase < Minitest::Test # rubocop:disable Puma/TestsMustTimeout
+  # our own subclass so we never confuse different timeouts
+  class TestTookTooLong < Timeout::Error
+  end
 
-      Minitest::Result.from self # per contract
+  def self.run_one_method(klass, method_name, reporter)
+    ::Timeout.timeout($test_case_timeout, TestTookTooLong) do
+      super
     end
   end
-else
-  module TimeoutEveryTestCase
-    # our own subclass so we never confuse different timeouts
-    class TestTookTooLong < Timeout::Error
-    end
-
-    def run
-      time_it do
-        capture_exceptions do
-          ::Timeout.timeout($test_case_timeout, TestTookTooLong) do
-            Minitest::Test::SETUP_METHODS.each { |hook| self.send hook }
-            self.send self.name
-          end
-        end
 
-        capture_exceptions do
-          ::Timeout.timeout($test_case_timeout, TestTookTooLong) do
-            Minitest::Test::TEARDOWN_METHODS.each { |hook| self.send hook }
-          end
-        end
-        if respond_to? :clean_tmp_paths
-          clean_tmp_paths
-        end
-      end
-
-      Minitest::Result.from self # per contract
-    end
+  def teardown
+    super
+    clean_tmp_paths if respond_to? :clean_tmp_paths
   end
 end
 
-Minitest::Test.prepend TimeoutEveryTestCase
-
 if ENV['CI']
   require 'minitest/retry'
 

test/helpers/integration.rb

@@ -7,7 +7,7 @@
 
 # Only single mode tests go here. Cluster and pumactl tests
 # have their own files, use those instead
-class TestIntegration < Minitest::Test
+class TestIntegration < TimeoutTestCase
   include TmpPath
   HOST  = "127.0.0.1"
   TOKEN = "xxyyzz"

test/test_app_status.rb

@@ -5,8 +5,7 @@
 require "puma/app/status"
 require "rack"
 
-class TestAppStatus < Minitest::Test
-
+class TestAppStatus < TimeoutTestCase
   class FakeServer
     def initialize
       @status = :running

test/test_binder.rb

@@ -8,7 +8,7 @@
 require "puma/events"
 require "puma/configuration"
 
-class TestBinderBase < Minitest::Test
+class TestBinderBase < TimeoutTestCase
   include SSLHelper if ::Puma::HAS_SSL
   include TmpPath
 

test/test_bundle_pruner.rb

@@ -2,7 +2,7 @@
 
 require 'puma/events'
 
-class TestBundlePruner < Minitest::Test
+class TestBundlePruner < TimeoutTestCase
 
   PUMA_VERS = "puma-#{Puma::Const::PUMA_VERSION}"
 

test/test_busy_worker.rb

@@ -1,8 +1,7 @@
 require_relative "helper"
 require_relative "helpers/test_puma/puma_socket"
 
-class TestBusyWorker < Minitest::Test
-
+class TestBusyWorker < TimeoutTestCase
   include ::TestPuma::PumaSocket
 
   def setup

test/test_cli.rb

@@ -7,7 +7,7 @@
 require "json"
 require "psych"
 
-class TestCLI < Minitest::Test
+class TestCLI < TimeoutTestCase
   include SSLHelper if ::Puma::HAS_SSL
   include TmpPath
   include TestPuma::PumaSocket

test/test_config.rb

@@ -6,7 +6,7 @@
 require 'puma/log_writer'
 require 'rack'
 
-class TestConfigFile < Minitest::Test
+class TestConfigFile < TimeoutTestCase
   parallelize_me!
 
   def test_default_max_threads
@@ -633,7 +633,7 @@ def assert_warning_for_hooks_defined_in_single_mode(hook_name)
 end
 
 # contains tests that cannot run parallel
-class TestConfigFileSingle < Minitest::Test
+class TestConfigFileSingle < TimeoutTestCase
   def test_custom_logger_from_DSL
     conf = Puma::Configuration.new { |c| c.load 'test/config/custom_logger.rb' }
 
@@ -645,7 +645,7 @@ def test_custom_logger_from_DSL
 end
 
 # Thread unsafe modification of ENV
-class TestEnvModifificationConfig < Minitest::Test
+class TestEnvModifificationConfig < TimeoutTestCase
   def test_double_bind_port
     port = (rand(10_000) + 30_000).to_s
     env = { "PORT" => port }
@@ -659,7 +659,7 @@ def test_double_bind_port
   end
 end
 
-class TestConfigEnvVariables < Minitest::Test
+class TestConfigEnvVariables < TimeoutTestCase
   def test_config_loads_correct_min_threads
     assert_equal 0, Puma::Configuration.new.options.default_options[:min_threads]
 
@@ -719,7 +719,7 @@ def test_config_preloads_app_if_using_workers
   end
 end
 
-class TestConfigFileWithFakeEnv < Minitest::Test
+class TestConfigFileWithFakeEnv < TimeoutTestCase
   def setup
     FileUtils.mkpath("config/puma")
     File.write("config/puma/fake-env.rb", "")

test/test_error_logger.rb

@@ -1,7 +1,7 @@
 require 'puma/error_logger'
 require_relative "helper"
 
-class TestErrorLogger < Minitest::Test
+class TestErrorLogger < TimeoutTestCase
   Req = Struct.new(:env, :body)
 
   def test_stdio

test/test_events.rb

@@ -1,7 +1,7 @@
 require 'puma/events'
 require_relative "helper"
 
-class TestEvents < Minitest::Test
+class TestEvents < TimeoutTestCase
   def test_register_callback_with_block
     res = false
 

test/test_example_cert_expiration.rb

@@ -7,7 +7,7 @@
 #
 # These tests will start to fail 1 month before the certs expire
 #
-class TestExampleCertExpiration < Minitest::Test
+class TestExampleCertExpiration < TimeoutTestCase
   EXAMPLES_DIR = File.expand_path '../examples/puma', __dir__
   EXPIRE_THRESHOLD = Time.now.utc - (60 * 60 * 24 * 30) # 30 days
 

test/test_http10.rb

@@ -2,7 +2,7 @@
 
 require "puma/puma_http11"
 
-class Http10ParserTest < Minitest::Test
+class Http10ParserTest < TimeoutTestCase
   def test_parse_simple
     parser = Puma::HttpParser.new
     req = {}

test/test_iobuffer.rb

@@ -2,7 +2,7 @@
 
 require "puma/io_buffer"
 
-class TestIOBuffer < Minitest::Test
+class TestIOBuffer < TimeoutTestCase
   attr_accessor :iobuf
   def setup
     self.iobuf = Puma::IOBuffer.new

test/test_json_serialization.rb

@@ -2,7 +2,7 @@
 require "json"
 require "puma/json_serialization"
 
-class TestJSONSerialization < Minitest::Test
+class TestJSONSerialization < TimeoutTestCase
   parallelize_me! unless JRUBY_HEAD
 
   def test_json_generates_string_for_hash_with_string_keys

test/test_launcher.rb

@@ -5,9 +5,7 @@
 require 'puma/log_writer'
 
 # Intermittent failures & errors when run parallel in GHA, local use may run fine.
-
-
-class TestLauncher < Minitest::Test
+class TestLauncher < TimeoutTestCase
   include TmpPath
 
   def test_prints_thread_traces

test/test_log_writer.rb

@@ -2,7 +2,7 @@
 require 'puma/log_writer'
 require_relative "helper"
 
-class TestLogWriter < Minitest::Test
+class TestLogWriter < TimeoutTestCase
   def test_null
     log_writer = Puma::LogWriter.null
 

test/test_minissl.rb

@@ -2,7 +2,7 @@
 
 require "puma/minissl" if ::Puma::HAS_SSL
 
-class TestMiniSSL < Minitest::Test
+class TestMiniSSL < TimeoutTestCase
 
   if Puma.jruby?
     def test_raises_with_invalid_keystore_file

test/test_normalize.rb

@@ -4,7 +4,7 @@
 
 require "puma/request"
 
-class TestNormalize < Minitest::Test
+class TestNormalize < TimeoutTestCase
   parallelize_me!
 
   include Puma::Request

test/test_null_io.rb

@@ -4,7 +4,7 @@
 
 require "puma/null_io"
 
-class TestNullIO < Minitest::Test
+class TestNullIO < TimeoutTestCase
   parallelize_me!
 
   attr_accessor :nio

test/test_out_of_band_server.rb

@@ -1,6 +1,6 @@
 require_relative "helper"
 
-class TestOutOfBandServer < Minitest::Test
+class TestOutOfBandServer < TimeoutTestCase
   parallelize_me!
 
   def setup

test/test_persistent.rb

@@ -3,7 +3,7 @@
 require_relative "helper"
 require_relative "helpers/test_puma/puma_socket"
 
-class TestPersistent < Minitest::Test
+class TestPersistent < TimeoutTestCase
   parallelize_me!
 
   include ::TestPuma::PumaSocket

test/test_puma_localhost_authority.rb

@@ -11,7 +11,7 @@
   require "openssl" unless Object.const_defined? :OpenSSL
 end
 
-class TestPumaLocalhostAuthority < Minitest::Test
+class TestPumaLocalhostAuthority < TimeoutTestCase
   include TestPuma
   include TestPuma::PumaSocket
 
@@ -45,7 +45,7 @@ def test_localhost_authority_file_generated
 
 end if ::Puma::HAS_SSL && !Puma::IS_JRUBY
 
-class TestPumaSSLLocalhostAuthority < Minitest::Test
+class TestPumaSSLLocalhostAuthority < TimeoutTestCase
   include TestPuma
   include TestPuma::PumaSocket
 

test/test_puma_server.rb

@@ -10,7 +10,7 @@ def backtrace; nil; end
   def message; "no backtrace error"; end
 end
 
-class TestPumaServer < Minitest::Test
+class TestPumaServer < TimeoutTestCase
   parallelize_me!
 
   include TestPuma

test/test_puma_server_current.rb

@@ -13,7 +13,7 @@ def call(env)
   end
 end
 
-class PumaServerCurrentTest < Minitest::Test
+class PumaServerCurrentTest < TimeoutTestCase
   parallelize_me!
 
   def setup

test/test_puma_server_hijack.rb

@@ -14,7 +14,7 @@
 # The sleep statements may not be needed for local CI, but are needed
 # for use with GitHub Actions...
 
-class TestPumaServerHijack < Minitest::Test
+class TestPumaServerHijack < TimeoutTestCase
   parallelize_me!
 
   def setup