Cleanup tests, parallelize a few (#1850)

+ Some tests getting frozen_string_literal
+ Remove unneccessary bundler setup
+ Fewer/tighter sleeps

Gemfile

@@ -9,6 +9,7 @@ gem "nio4r", "~> 2.0"
 gem "rack", "< 3.0"
 gem "minitest", "~> 5.11"
 gem "minitest-retry"
+gem "minitest-proveit"
 
 gem "jruby-openssl", :platform => "jruby"
 

test/helper.rb

@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 # Copyright (c) 2011 Evan Phoenix
 # Copyright (c) 2005 Zed A. Shaw
 
@@ -10,18 +11,11 @@
   end
 end
 
-begin
-  require "bundler/setup"
-  # bundler/setup may not load bundler
-  require "bundler" unless Bundler.const_defined?(:ORIGINAL_ENV)
-rescue LoadError
-  warn "Failed to load bundler ... this should only happen during package building"
-end
-
 require "net/http"
 require "timeout"
 require "minitest/autorun"
 require "minitest/pride"
+require "minitest/proveit"
 
 $LOAD_PATH << File.expand_path("../../lib", __FILE__)
 Thread.abort_on_exception = true
@@ -128,3 +122,10 @@ def next_port(incr = 1)
 end
 
 Minitest::Test.include TestSkips
+
+class Minitest::Test
+  def self.run(reporter, options = {}) # :nodoc:
+    prove_it!
+    super
+  end
+end

test/test_cli.rb

@@ -87,8 +87,6 @@ def test_control_clustered
 
     wait_booted
 
-    sleep 2
-
     s = UNIXSocket.new @tmp_path
     s << "GET /stats HTTP/1.0\r\n\r\n"
     body = s.read

test/test_config.rb

@@ -1,12 +1,28 @@
+# frozen_string_literal: true
+
 require_relative "helper"
 
 require "puma/configuration"
 
-class TestConfigFile < Minitest::Test
-  def setup
-    FileUtils.mkpath("config/puma")
-    File.write("config/puma/fake-env.rb", "")
+class TestConfigFileBase < Minitest::Test
+  private
+
+  def with_env(env = {})
+    original_env = {}
+    env.each do |k, v|
+      original_env[k] = ENV[k]
+      ENV[k] = v
+    end
+    yield
+  ensure
+    original_env.each do |k, v|
+      ENV[k] = v
+    end
   end
+end
+
+class TestConfigFile < TestConfigFileBase
+  parallelize_me!
 
   def test_app_from_rackup
     conf = Puma::Configuration.new do |c|
@@ -46,19 +62,6 @@ def test_ssl_configuration_from_DSL
     assert_equal [200, {}, ["embedded app"]], app.call({})
   end
 
-  def test_double_bind_port
-    port = (rand(10_000) + 30_000).to_s
-    with_env("PORT" => port) do
-      conf = Puma::Configuration.new do |user_config, file_config, default_config|
-        user_config.bind "tcp://#{Puma::Configuration::DefaultTCPHost}:#{port}"
-        file_config.load "test/config/app.rb"
-      end
-
-      conf.load
-      assert_equal ["tcp://0.0.0.0:#{port}"], conf.options[:binds]
-    end
-  end
-
   def test_ssl_bind
     skip_on :jruby
 
@@ -168,24 +171,6 @@ def test_config_files_with_non_existing_path
     assert_equal ['test/config/typo/settings.rb'], conf.config_files
   end
 
-  def test_config_files_with_rack_env
-    with_env('RACK_ENV' => 'fake-env') do
-      conf = Puma::Configuration.new do
-      end
-
-      assert_equal ['config/puma/fake-env.rb'], conf.config_files
-    end
-  end
-
-  def test_config_files_with_specified_environment
-    conf = Puma::Configuration.new do
-    end
-
-    conf.options[:environment] = 'fake-env'
-
-    assert_equal ['config/puma/fake-env.rb'], conf.config_files
-  end
-
   def test_config_files_with_integer_convert
     conf = Puma::Configuration.new(config_files: ['test/config/with_integer_convert.rb']) do
     end
@@ -211,23 +196,49 @@ def test_config_raise_exception_on_sigterm
     conf.options[:raise_exception_on_sigterm] = true
     assert_equal conf.options[:raise_exception_on_sigterm], true
   end
+end
 
-  def teardown
-    FileUtils.rm_r("config/puma")
+# Thread unsafe modification of ENV
+class TestEnvModifificationConfig < TestConfigFileBase
+  def test_double_bind_port
+    port = (rand(10_000) + 30_000).to_s
+    with_env("PORT" => port) do
+      conf = Puma::Configuration.new do |user_config, file_config, default_config|
+        user_config.bind "tcp://#{Puma::Configuration::DefaultTCPHost}:#{port}"
+        file_config.load "test/config/app.rb"
+      end
+
+      conf.load
+      assert_equal ["tcp://0.0.0.0:#{port}"], conf.options[:binds]
+    end
   end
+end
 
-  private
+class TestConfigFileWithFakeEnv < TestConfigFileBase
+  def setup
+    FileUtils.mkpath("config/puma")
+    File.write("config/puma/fake-env.rb", "")
+  end
 
-    def with_env(env = {})
-      original_env = {}
-      env.each do |k, v|
-        original_env[k] = ENV[k]
-        ENV[k] = v
-      end
-      yield
-    ensure
-      original_env.each do |k, v|
-        ENV[k] = v
+  def test_config_files_with_rack_env
+    with_env('RACK_ENV' => 'fake-env') do
+      conf = Puma::Configuration.new do
       end
+
+      assert_equal ['config/puma/fake-env.rb'], conf.config_files
+    end
+  end
+
+  def test_config_files_with_specified_environment
+    conf = Puma::Configuration.new do
     end
+
+    conf.options[:environment] = 'fake-env'
+
+    assert_equal ['config/puma/fake-env.rb'], conf.config_files
+  end
+
+  def teardown
+    FileUtils.rm_r("config/puma")
+  end
 end

test/test_null_io.rb

@@ -1,8 +1,12 @@
+# frozen_string_literal: true
+
 require_relative "helper"
 
 require "puma/null_io"
 
 class TestNullIO < Minitest::Test
+  parallelize_me!
+
   attr_accessor :nio
 
   def setup
@@ -18,7 +22,9 @@ def test_gets_returns_nil
   end
 
   def test_each_never_yields
-    nio.each { raise "never yield" }
+    nio.instance_variable_set(:@foo, :baz)
+    nio.each { @foo = :bar }
+    assert_equal :baz, nio.instance_variable_get(:@foo)
   end
 
   def test_read_with_no_arguments

test/test_persistent.rb

@@ -152,14 +152,14 @@ def test_one_with_keep_alive_header
   end
 
   def test_persistent_timeout
-    @server.persistent_timeout = 2
+    @server.persistent_timeout = 1
     @client << @valid_request
     sz = @body[0].size.to_s
 
     assert_equal "HTTP/1.1 200 OK\r\nX-Header: Works\r\nContent-Length: #{sz}\r\n\r\n", lines(4)
     assert_equal "Hello", @client.read(5)
 
-    sleep 3
+    sleep 2
 
     assert_raises EOFError do
       @client.read_nonblock(1)

test/test_rack_server.rb

@@ -1,46 +1,38 @@
+# frozen_string_literal: true
 require_relative "helper"
 
 require "rack"
 
 class TestRackServer < Minitest::Test
+  parallelize_me!
 
   class ErrorChecker
     def initialize(app)
       @app = app
       @exception = nil
-      @env = nil
     end
 
     attr_reader :exception, :env
 
     def call(env)
       begin
-        @env = env
-        return @app.call(env)
+        @app.call(env)
       rescue Exception => e
         @exception = e
-
-        [
-          500,
-          { "X-Exception" => e.message, "X-Exception-Class" => e.class.to_s },
-          ["Error detected"]
-        ]
+        [ 500, {}, ["Error detected"] ]
       end
     end
   end
 
   class ServerLint < Rack::Lint
     def call(env)
-      assert("No env given") { env }
       check_env env
 
       @app.call(env)
     end
   end
 
   def setup
-    @valid_request = "GET / HTTP/1.1\r\nHost: test.com\r\nContent-Type: text/plain\r\n\r\n"
-
     @simple = lambda { |env| [200, { "X-Header" => "Works" }, ["Hello"]] }
     @server = Puma::Server.new @simple
     @server.add_tcp_listener "127.0.0.1", 0
@@ -67,9 +59,7 @@ def test_lint
 
     stop
 
-    if exc = @checker.exception
-      raise exc
-    end
+    refute @checker.exception, "Checker raised exception"
   end
 
   def test_large_post_body
@@ -85,9 +75,7 @@ def test_large_post_body
 
     stop
 
-    if exc = @checker.exception
-      raise exc
-    end
+    refute @checker.exception, "Checker raised exception"
   end
 
   def test_path_info
@@ -134,5 +122,4 @@ def test_common_logger
 
     assert_match %r!GET /test HTTP/1\.1!, log.string
   end
-
 end