Test Framework Update (Draft, review only)

[MSP-Greg]

Description

This is the branch I've been working on the test framework in. The first and last commits are framework related, most of the middle commits are revisions to single test files.

Looking for review re the API, method names, organization, etc. Pick your favorite test files, find the commits, and review the diffs...

A few points:

• Seems a lot more stable than the current tests. There are issues with JRuby (macOS in particular), as often jobs complete with all tests passing, but the test process is frozen. Of course, it's intermittent... More work to determine the cause.
• I tried to remove as many blocking operations in test code as possible. When running tests parallel, blocking operations will cause a lot of failures/retries.
• Tests should be portable, assuming the base class (ServerSpawn or ServerInProcess) remains the same.
• I think in many cases the 'test intent' is much easier to see, as there isn't the code clutter to setup things not pertinent to the test intent.
• I've got a shell script that runs all the test files one at a time. Wrote it for the JRuby issue, but I also found some issues with running tests by themselves. So, some code changes due to that, along with standardizing the require code.
• The current test system has 'helper code' that passes blocks. The blocks vary with the helper code. It gets tricky and cluttered trying to pass multiple blocks to a method, so the test framework uses lambdas for almost everything.
• The current test system often loaded rackup or config files. I found that to be a PITA, as one needed to open other files to look at what the test was doing. The framework allows config strings to be used, it handles writing a temp file for use by Puma.

I have started doc'ing the code, see the doc at https://msp-greg.github.io/puma_test/. All the test modules/classes are children of the TestPuma module at the bottom of the class list.

Once people have reviewed this, I'd start with a PR of the test framework files, then submit a PR for each test file update? Or, we could commit several without squashing the commits when merging...

Finally, the current test system divides test files by the server type used (in-process or spawned/IO.popen). This PR follows with the same idea. I think they could be combined, so one test file could contain both server types. Thoughts?

Thanks in advance.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) 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.

[dentarg]

What's the motivation behind minissl.rb - set @eng_ctx = nil to nil on close (5eec16a)? Maybe that should go in as a separate PR as it the only change to lib/ code

[MSP-Greg]

@dentarg

Re the minissl commit, I may revert that. Still doing work re the JRuby issue, as both failures in the CI run here successfully completed the tests.

Also, there are some issues with closing spawned servers using SSL listeners (possibly OS/platform related), still working on it.

Re this PR, it's more about getting more 'eyes' looking at the test framework API. I suppose I could have pushed the changes to a branch here, instead of doing a PR...

[MSP-Greg]

Just ran CI in my fork with master and the work branch I've got this code in. Ran both in my fork, since I have an individual 'Pro' account, I have a higher CI concurrency limit than a standard org account (puma). Note the time difference, and the fact that the updated test suite has no failures. I've removed all the 'allow failure' code.

Time branch Failures   Log
25:30 master 1 MRI, 5 non-MRI  https://github.com/MSP-Greg/puma/actions/runs/7160869671
12:48 PR NONE   https://github.com/MSP-Greg/puma/actions/runs/7160795377

The current log of the test suite run on master is at https://github.com/puma/puma/actions/workflows/tests.yaml?query=branch%3Amaster. Several failed runs. I've had the work branch fail also, but it's much less frequent.

It's been about 6-1/2 weeks since this was opened. Not any feedback re the test suite updates. Also no feedback about how to break the PR up into smaller PR's.

[dentarg]

Was this the idea here?

Suggested change

	# Maximum random number
	MAX = 36**6 # < 0x100000000
	# Returns new random string upto 6 bytes
	def next
	rand(0x100000000).to_s(36)
	# Maximum random number
	MAX = 36**6 # < 0x100000000
	# Returns new random string upto 6 bytes
	def next
	rand(MAX).to_s(36)

[dentarg]

Would this fix help in master? I noticed this test failing in master (https://github.com/puma/puma/actions/runs/7160776392/job/19495517337#step:8:874) but I couldn't make sense of it (systemd plugin should not be loaded on JRuby, code has guard against JRUBY_VERSION constant)

[dentarg]

I guess not, I see more test have the same super call, it is how TestIntegration works.

puma/test/helpers/integration.rb

Line 27 in 08e71a2

def setup

(Sorry)

Still curious if you @MSP-Greg have any theories about the TestPluginSystemdJruby master failures :)

[dentarg]

Can't see anything using this constant?

[dentarg]

test/helpers/integration.rb has a method named like this, it looks unused, should we remove that one?

[dentarg]

This is awesome work. No question about it. It will be much easier to maintain and contribute to Puma if we get the CI stable.

We can probably go ahead with these changes as they're only changing test files.

It's been about 6-1/2 weeks since this was opened. Not any feedback re the test suite updates. Also no feedback about how to break the PR up into smaller PR's.

It is a big PR. I pointed out a few things I happened to see now, but I have far from looked at everything. I work on Puma when I feel like it, when I think I have some time over. It is almost never a block of several hours. I forgot about this. I think smaller PR was a successful way to go: https://github.com/puma/puma/pulls?q=is%3Apr+%22PumaSocket%22

[MSP-Greg]

@dentarg

Thanks for your comments. I haven't recently had the time to work on this that I'd like, including looking at your comments.

I rebased on master, and some odd things are happening with all Ruby master builds. I also follow ruby/ruby's CI, and there are no problems there. Recently, CI for Windows builds is running much faster and is more stable.

Some of the issues have to do with GC finalizers called on Tempfile.new, which are created in Client when the request body is large or indeterminate/chunked. Also, there are hung Ruby processes when the tests complete, even if they pass.

Hopefully, more info to come.

Regardless, Happy Holidays and Merry Christmas.

Not really religious, but I appreciate the more secular ideas about Christmas and the Holidays. Sorry if anyone is offended by the mention.