[Draft] Various test-suite speed/reliability improvements #2241

wjordan · 2020-04-29T05:29:38Z

Description

Spike with a bunch of different changes focused on improving the speed and reliability of the test suite. The goal was just to quickly experiment with running tests faster and with fewer flaky failures, and to share some of the high-level ideas for early discussion. Next step would be to break out the most useful fixes/changes into smaller PRs to be merged separately.

Highlights

Higher parallelization (increase MT_CPU from default 2 to 10)
Reduce duration of various sleeps and timeouts in tests (e.g., sleep 1 to sleep 0.01)
Replace UniquePort implementation with port 0 in all tests (UniquePort has a race condition that can cause intermittent port conflicts) (Stabilize CI testing? [changelog skip] #2270)

The following changes have since been broken out into separate smaller PRs:

~~Fire on_booted after server starts (fixes TestCLI#test_control failing with "pool_capacity": 0 #2212)~~ (Fire on_booted after server starts #2431)
~~Catch more stream-interruption errors (EPIPE, etc) that can happen during shutdown/thread-exit races~~ (Improve worker shutdown reliability, CI fixes #2312)
~~Catch and handle ThreadPool::ForceShutdown errors throughout entire threadpool lifecycle~~ (Better error handling during force shutdown #2271)
~~Add mutex for adding connections to reactor based on queue_requests~~ (Better error handling during force shutdown #2271 / Prevent connections from entering Reactor after shutdown begins #2377)
Fix some race conditions / edge-cases in Reactor (process multiple triggers in single cycle, re-add timeout when client-request isn't ready to be added back to thread pool [issue Minor timeout bug in Reactor #2282]) (Reactor refactor #2279)
~~Move shell tests into Minitest class so they can be run in parallel~~ ([changelog skip] Move integration logging test to main test suite #2240, [changelog skip] Move integration logging test to main test suite #2346, [changelog skip] Move integration logging test to main test suite #2347)

Performance

Ubuntu 2.7
- Before: 2m 8s
- After: 13s
Windows 2.7
- Before: 53s
- After: 12s
MacOS JRuby
- Before: 3m 8s
- After: 32s

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
I have added appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

nateberkopec · 2020-04-29T05:53:31Z

.github/workflows/puma.yml

@@ -9,6 +9,8 @@ jobs:
    env:
      CI: true
      TESTOPTS: -v
+      MT_CPU: 10


nateberkopec · 2020-04-29T05:54:25Z

test/helper.rb

 require "timeout"
 module TimeoutEveryTestCase
  # our own subclass so we never confused different timeouts
  class TestTookTooLong < Timeout::Error
  end

  def run(*)
-    ::Timeout.timeout(RUBY_ENGINE == 'ruby' ? 60 : 120, TestTookTooLong) { super }
+    ::Timeout.timeout(RUBY_ENGINE == 'ruby' ? 10 : 120, TestTookTooLong) { super }


nateberkopec · 2020-04-29T05:55:11Z

test/helper.rb

+
+require 'openssl'
+# Silence warnings on SSLSocket initialization.
+module SSLSocketFix


Can you point me to where these warnings happen currently?

This was a Ruby 2.2 warning

nateberkopec · 2020-04-29T05:56:00Z

test/helpers/integration.rb

@@ -10,30 +10,30 @@ class TestIntegration < Minitest::Test
  TOKEN = "xxyyzz"
  WORKERS = 2

-  BASE = defined?(Bundler) ? "bundle exec #{Gem.ruby} -Ilib" :
-    "#{Gem.ruby} -Ilib"
+  BASE = "#{Gem.ruby} -rrubygems -Ilib"


This might break tests for people running locally. In CI it will be fine of course. Not sure if we can do this.

nateberkopec · 2020-04-29T05:56:59Z

test/shell/t1.rb

@@ -1,10 +1,16 @@
-system "ruby -rrubygems -Ilib bin/puma -p 10102 -C test/shell/t1_conf.rb test/rackup/hello.ru &"
+tcp_port = 0


See #2240, I want to just get rid of this folder and replace with normal tests.

nateberkopec · 2020-04-29T05:57:19Z

test/test_app_status.rb

@@ -4,6 +4,7 @@

 require "puma/app/status"
 require "rack"
+require "rack/mock"


required but not used?

I think this eliminated a 'circular require considered harmful' warning on Ruby 2.2.

nateberkopec · 2020-04-29T05:57:49Z

test/helper.rb

@@ -44,22 +36,14 @@ def hit(uris)
  end
 end

-module UniquePort
-  def self.call
-    TCPServer.open('127.0.0.1', 0) do |server|


nateberkopec · 2020-04-29T05:58:00Z

test/helper.rb

@@ -2,15 +2,6 @@
 # Copyright (c) 2011 Evan Phoenix
 # Copyright (c) 2005 Zed A. Shaw

-if %w(2.2.7 2.2.8 2.2.9 2.2.10 2.3.4 2.4.1).include? RUBY_VERSION


Hmmm... why though?

Why move to lib I mean.

I think I saw integration tests running Puma in a subprocess still encountering this issue on Ruby2.2. Plus I figured it made sense to warn anyone on relevant Ruby versions possibly affected by this issue that weren't already using the gem.
Hopefully not super important though since 2.2 support probably won't last much longer?

Since we're doing a major release next, I'll revisit that policy. Dropping 2.2 especially would really help for SSL maintenance.

nateberkopec · 2020-04-29T06:00:14Z

Gut reaction: yes, yes, and hell yes.

Ready to discuss these individually/in 100-line chunks when you are.

wjordan · 2020-09-23T21:15:39Z

Returning to this draft after being idle for a while - I've updated the original description, crossing out items that have since been broken out into separate smaller PRs (and cross-referencing them). Next up I plan to rebase this PR to remove those parts that have been merged.

After that I plan to break out two final pieces into separate PRs:

Fire on_booted after server starts (fixes TestCLI#test_control failing with "pool_capacity": 0 #2212)
Replace UniquePort implementation with port 0 in all tests (UniquePort has a race condition that can cause intermittent port conflicts) (see also Stabilize CI testing? [changelog skip] #2270, which was closed in favor of the code from here)

After one more rebase, that should leave the test-suite speed improvements in this PR for review.

nateberkopec · 2020-09-23T23:23:30Z

Nice to have you back @wjordan :)

nateberkopec · 2021-02-06T21:36:40Z

@wjordan will close these for now, let me know if you want to come back to them

Various test-suite speed/reliability improvements

3e5ff03

nateberkopec added the maintenance label Apr 29, 2020

nateberkopec reviewed Apr 29, 2020

View reviewed changes

.github/workflows/puma.yml

@@ -9,6 +9,8 @@ jobs:

env:

CI: true

TESTOPTS: -v

MT_CPU: 10

Copy link

Member

nateberkopec Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niiice.

nateberkopec reviewed Apr 29, 2020

View reviewed changes

harrylewis mentioned this pull request Apr 29, 2020

Fold the old integration tests into the new test suite. #2172

Closed

nateberkopec added the waiting-for-changes Waiting on changes from the requestor label May 8, 2020

wjordan mentioned this pull request May 17, 2020

Stabilize CI testing? [changelog skip] #2270

Closed

8 tasks

wjordan mentioned this pull request Sep 23, 2020

Prevent connections from entering Reactor after shutdown begins #2377

Merged

8 tasks

nateberkopec closed this Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Various test-suite speed/reliability improvements #2241

[Draft] Various test-suite speed/reliability improvements #2241

wjordan commented Apr 29, 2020 •

edited

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

wjordan Apr 29, 2020

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

wjordan Apr 29, 2020

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

nateberkopec Apr 29, 2020

wjordan Apr 29, 2020 •

edited

nateberkopec Apr 30, 2020

nateberkopec commented Apr 29, 2020

wjordan commented Sep 23, 2020 •

edited

nateberkopec commented Sep 23, 2020

nateberkopec commented Feb 6, 2021

		@@ -1,10 +1,16 @@
		system "ruby -rrubygems -Ilib bin/puma -p 10102 -C test/shell/t1_conf.rb test/rackup/hello.ru &"
		tcp_port = 0

[Draft] Various test-suite speed/reliability improvements #2241

[Draft] Various test-suite speed/reliability improvements #2241

Conversation

wjordan commented Apr 29, 2020 • edited

Description

Highlights

Performance

Your checklist for this pull request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjordan Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec commented Apr 29, 2020

wjordan commented Sep 23, 2020 • edited

nateberkopec commented Sep 23, 2020

nateberkopec commented Feb 6, 2021

wjordan commented Apr 29, 2020 •

edited

wjordan Apr 29, 2020 •

edited

wjordan commented Sep 23, 2020 •

edited