From 9f4edf4c665cd394ad363bc0984500f08c670010 Mon Sep 17 00:00:00 2001 From: Nate Berkopec Date: Sat, 27 Jul 2019 09:47:19 -0700 Subject: [PATCH] Cleanup tests, parallelize a few (#1850) + Some tests getting frozen_string_literal + Remove unneccessary bundler setup + Fewer/tighter sleeps --- Gemfile | 1 + test/helper.rb | 17 ++++--- test/test_cli.rb | 2 - test/test_config.rb | 107 +++++++++++++++++++++------------------ test/test_null_io.rb | 8 ++- test/test_persistent.rb | 4 +- test/test_rack_server.rb | 25 +++------ test/test_thread_pool.rb | 96 +++++++++++++++++++---------------- test/test_unix_socket.rb | 7 ++- test/test_web_server.rb | 58 ++++++++++----------- win_gem_test/puma.ps1 | 2 +- 11 files changed, 169 insertions(+), 158 deletions(-) diff --git a/Gemfile b/Gemfile index 44f6e48b80..24fbed33cd 100644 --- a/Gemfile +++ b/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" diff --git a/test/helper.rb b/test/helper.rb index 3575b69fe5..8ada8f5a39 100644 --- a/test/helper.rb +++ b/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 diff --git a/test/test_cli.rb b/test/test_cli.rb index cadefd097a..5140138665 100644 --- a/test/test_cli.rb +++ b/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 diff --git a/test/test_config.rb b/test/test_config.rb index ae88bcda86..2fbef5ea07 100644 --- a/test/test_config.rb +++ b/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 diff --git a/test/test_null_io.rb b/test/test_null_io.rb index 5af9d6e0b1..c620cd0090 100644 --- a/test/test_null_io.rb +++ b/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 diff --git a/test/test_persistent.rb b/test/test_persistent.rb index 06ec90aa71..7144f72fc8 100644 --- a/test/test_persistent.rb +++ b/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) diff --git a/test/test_rack_server.rb b/test/test_rack_server.rb index 063d2f6253..d0fcac7db0 100644 --- a/test/test_rack_server.rb +++ b/test/test_rack_server.rb @@ -1,37 +1,31 @@ +# 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) @@ -39,8 +33,6 @@ def call(env) 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 diff --git a/test/test_thread_pool.rb b/test/test_thread_pool.rb index f57dd2c8ad..7f2e65738b 100644 --- a/test/test_thread_pool.rb +++ b/test/test_thread_pool.rb @@ -5,11 +5,13 @@ class TestThreadPool < Minitest::Test def teardown - @pool.shutdown if @pool + @pool.shutdown(1) if @pool end def new_pool(min, max, &block) block = proc { } unless block + @work_mutex = Mutex.new + @work_done = ConditionVariable.new @pool = Puma::ThreadPool.new(min, max, &block) end @@ -22,18 +24,21 @@ def test_append_spawns thread_name = nil pool = new_pool(0, 1) do |work| - saw << work - thread_name = Thread.current.name if Thread.current.respond_to?(:name) + @work_mutex.synchronize do + saw << work + thread_name = Thread.current.name if Thread.current.respond_to?(:name) + @work_done.signal + end end pool << 1 - pause - - assert_equal [1], saw - assert_equal 1, pool.spawned - # Thread name is new in Ruby 2.3 - assert_equal('puma 001', thread_name) if Thread.current.respond_to?(:name) + @work_mutex.synchronize do + @work_done.wait(@work_mutex, 5) + assert_equal 1, pool.spawned + assert_equal [1], saw + assert_equal('puma 001', thread_name) if Thread.current.respond_to?(:name) + end end def test_converts_pool_sizes @@ -47,55 +52,60 @@ def test_converts_pool_sizes end def test_append_queues_on_max - finish = false - pool = new_pool(0, 1) { Thread.pass until finish } + pool = new_pool(0, 0) do + "Hello World!" + end pool << 1 pool << 2 pool << 3 - pause - - assert_equal 2, pool.backlog - - finish = true + assert_equal 3, pool.backlog end def test_trim - pool = new_pool(0, 1) + pool = new_pool(0, 1) do |work| + @work_mutex.synchronize do + @work_done.signal + end + end pool << 1 - pause + @work_mutex.synchronize do + @work_done.wait(@work_mutex, 5) + assert_equal 1, pool.spawned + end - assert_equal 1, pool.spawned pool.trim + pool.instance_variable_get(:@workers).first.join - pause assert_equal 0, pool.spawned end def test_trim_leaves_min - finish = false - pool = new_pool(1, 2) { Thread.pass until finish } + pool = new_pool(1, 2) do |work| + @work_mutex.synchronize do + @work_done.signal + end + end pool << 1 pool << 2 - finish = true - - pause + @work_mutex.synchronize do + @work_done.wait(@work_mutex, 5) + assert_equal 2, pool.spawned + end - assert_equal 2, pool.spawned pool.trim pause - assert_equal 1, pool.spawned + + pool.trim pause - assert_equal 1, pool.spawned - end def test_force_trim_doesnt_overtrim @@ -215,16 +225,11 @@ def test_auto_reap_dead_threads assert_equal 2, pool.spawned + # TODO: is there a point to these two lines? pool << 1 pool << 2 - pause - - assert_equal 2, pool.spawned - - pool.auto_reap! 1 - - sleep 1 + pool.auto_reap! 0.1 pause @@ -232,19 +237,22 @@ def test_auto_reap_dead_threads end def test_force_shutdown_immediately - pool = new_pool(1, 1) { + pool = new_pool(0, 1) do |work| begin - sleep 1 + @work_mutex.synchronize do + @work_done.signal + end + sleep 10 # TODO: do something here other than sleep rescue Puma::ThreadPool::ForceShutdown end - } + end pool << 1 - sleep 0.1 - - pool.shutdown(0) - - assert_equal 0, pool.spawned + @work_mutex.synchronize do + @work_done.wait(@work_mutex, 5) + pool.shutdown(0) + assert_equal 0, pool.spawned + end end end diff --git a/test/test_unix_socket.rb b/test/test_unix_socket.rb index 712d6cdb4b..870a942356 100644 --- a/test/test_unix_socket.rb +++ b/test/test_unix_socket.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "helper" class TestPumaUnixSocket < Minitest::Test @@ -14,8 +16,7 @@ def setup end def teardown - @server.stop(true) if @server - File.unlink Path if File.exist? Path + @server.stop(true) end def test_server @@ -26,7 +27,5 @@ def test_server expected = "HTTP/1.0 200 OK\r\nContent-Length: 5\r\n\r\nWorks" assert_equal expected, sock.read(expected.size) - - sock.close end end diff --git a/test/test_web_server.rb b/test/test_web_server.rb index cc691b5bd3..4fd4b34803 100644 --- a/test/test_web_server.rb +++ b/test/test_web_server.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true # Copyright (c) 2011 Evan Phoenix # Copyright (c) 2005 Zed A. Shaw @@ -16,12 +17,10 @@ def call(env) end class WebServerTest < Minitest::Test + VALID_REQUEST = "GET / HTTP/1.1\r\nHost: www.zedshaw.com\r\nContent-Type: text/plain\r\n\r\n" def setup - @valid_request = "GET / HTTP/1.1\r\nHost: www.zedshaw.com\r\nContent-Type: text/plain\r\n\r\n" - @tester = TestHandler.new - @server = Puma::Server.new @tester, Puma::Events.strings @server.add_tcp_listener "127.0.0.1", 0 @@ -33,43 +32,24 @@ def teardown end def test_simple_server - hit(["http://127.0.0.1:#{ @server.connected_port }/test"]) + hit(["http://127.0.0.1:#{@server.connected_port}/test"]) assert @tester.ran_test, "Handler didn't really run" end - - def do_test(string, chunk, close_after=nil, shutdown_delay=0) - # Do not use instance variables here, because it needs to be thread safe - socket = TCPSocket.new("127.0.0.1", @server.connected_port); - request = StringIO.new(string) - chunks_out = 0 - - while data = request.read(chunk) - chunks_out += socket.write(data) - socket.flush - sleep 0.2 - if close_after and chunks_out > close_after - socket.close - sleep 1 - end - end - sleep(shutdown_delay) - socket.write(" ") # Some platforms only raise the exception on attempted write - socket.flush - end - def test_trickle_attack - do_test(@valid_request, 3) + socket = do_test(VALID_REQUEST, 3) + assert_match "hello", socket.read end def test_close_client assert_raises IOError do - do_test(@valid_request, 10, 20) + do_test(VALID_REQUEST, 10, 20) end end def test_bad_client - do_test("GET /test HTTP/BAD", 3) + socket = do_test("GET /test HTTP/BAD", 3) + assert_match "Bad Request", socket.read end def test_header_is_too_long @@ -79,10 +59,30 @@ def test_header_is_too_long end end + # TODO: Why does this test take exactly 20 seconds? def test_file_streamed_request body = "a" * (Puma::Const::MAX_BODY * 2) long = "GET /test HTTP/1.1\r\nContent-length: #{body.length}\r\n\r\n" + body - do_test(long, (Puma::Const::CHUNK_SIZE * 2) - 400) + socket = do_test(long, (Puma::Const::CHUNK_SIZE * 2) - 400) + assert_match "hello", socket.read end + private + + def do_test(string, chunk, close_after=nil) + # Do not use instance variables here, because it needs to be thread safe + socket = TCPSocket.new("127.0.0.1", @server.connected_port); + request = StringIO.new(string) + chunks_out = 0 + + while data = request.read(chunk) + chunks_out += socket.write(data) + socket.flush + socket.close if close_after && chunks_out > close_after + end + + socket.write(" ") # Some platforms only raise the exception on attempted write + socket.flush + socket + end end diff --git a/win_gem_test/puma.ps1 b/win_gem_test/puma.ps1 index 60a6275353..89fbfce457 100644 --- a/win_gem_test/puma.ps1 +++ b/win_gem_test/puma.ps1 @@ -37,7 +37,7 @@ function Pre-Compile { #———————————————————————————————————————————————————————————————— Run-Tests function Run-Tests { # call with comma separated list of gems to install or update - Update-Gems minitest, minitest-retry, rack, rake + Update-Gems minitest, minitest-retry, minitest-proveit, rack, rake $env:CI = 1 rake -f Rakefile_wintest -N -R norakelib | Set-Content -Path $log_name -PassThru -Encoding UTF8 # add info after test results