-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test system update #3128
base: master
Are you sure you want to change the base?
Test system update #3128
Conversation
3236224
to
3aed247
Compare
Fixed up |
lib/puma/binder.rb
Outdated
@@ -48,7 +48,7 @@ def initialize(log_writer, conf = Configuration.new) | |||
|
|||
@envs = {} | |||
@ios = [] | |||
localhost_authority | |||
# localhost_authority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line? Or was it a mistake to comment this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a mistake. Can't quite remember the particulars, but localhost_authority
will fail if some conditions aren't met. There's no reason to call it if the 'binds' do not include an ssl connection. I commented it out because I wanted to run the test suite several times, and forgot to remove the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Would be nice to know what conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may have happened when running a single file locally, can't recall.
GitHub hides the changes to Not to take anything away from the work done here, it is super great stuff @MSP-Greg 💪 but I do think we should aim for small specific PRs for changes to Puma |
I would generally agree. Unfortunately, the test suite is cluster-fu** of repetitive code, intermittent test failures, impossible to break apart into smaller files without more repetition, etc. I think most of the changes are in the commits c521c5c, 1dadb22 and 2d4ab6c. One is the changes to helper files and the addition of Or, those three commits are tightly bound and need to be committed together. Normally I prefer to see PR's squashed, but maybe these should be left as mutiple commits. I'll see if I can pull the library file commits from this into another PR. |
General Concept I started with two goals, first, make the test system more stable, and second, move towards more shared helper methods. So, I shut off the Timeout thread wrapping each test, shut off the retry, and started running the test suite locally. One main problem is running tests parallel. These run in threads. If any tests have blocking code, the tests slow down and may fail. This means that client sockets should always use wait_readable, and read_nonblock would be preferred. Code should also close client sockets. Because of this, we should discourage use of *Socket.new in test code, and provide helper methods. Obviously, there will be exceptions. Most tests either start a Puma server 'in-process' or spawn a server. In the future, having three helper classes, something like ServerInProc and ServerSpawn, along with a companion socket class would be a good setup. Implementing these classes will make it much easier to re-arrange tests. An example might be test_puma_server.rb, which contains 94 tests. The current 'socket' helper in this PR only works with TCP & UNIX sockets. SSL sockets need to be added, and integrated with the inproc and spawn server helpers (inproc is yet to be written). |
I've pulled all the code in lib files and Actions workflow files, so this now just contains test updates. I'll see whether it can be divided into smaller PR's, but it doesn't need to be squashed. Also, as the goal was to make CI more reliable and also contain less repetitive code, it was a matter of converging toward a solution, so reversing that and splitting it up may be difficult. Some of the work involved small changes to remove blocking code. |
Previous values resulted in retries with JRuby & TruffleRuby. Passing tests were often very close to time limits.
…yword Log server output only on error
Server is in-process, so it may need time to send an enum body
Description
Currently, all tests are wrapped by
::Timeout.timeout
. Switched to using a constant defined in each test class. This PR changes so that only test classes that use some form ofbundle install
are using the timeout.Created
test/helpers/puma_socket.rb
, which allows removal of duplicated code in several test files. This will also make it easier to re-arrange and/or split test files into more (smaller) files.Made minor updates to some library files, most of which related to error handling.
Currently, some of the tests fail if a client socket raises
Errno::ECONNRESET
. I've got some code that queries the GitHub API withNet::HTTP
, and it threw the same error, as GitHub has had network issues. Using the same URI, I tried several browsers, and they all retry onErrno::ECONNRESET
. Changed test code to allow a small percentage ofErrno::ECONNRESET
errors.CI is often running a full allocation of macos jobs at the end. Removed a few macos jobs to quicken the test time. This may be needed again, as GitHub will be supporting macos-13 and also
arm64
macos.Creating as a draft, not sure whether to break it up into lib file changes and test file changes, etc...
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.