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>

Author: nateberkopec

.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