test_integration.rb - cleanup, try removing log output on unneeded 'forked' servers

[MSP-Greg]

Commit 'test_integration.rb - minor code clean up' - cleans up file, no real changes

Commit 'start_forked_server => server.pid ?' - stop forking servers, trying to eliminate log output. There are some retries that may indicate issues.

And yes, #test_sigterm_closes_listeners_on_forked_servers still intermittently fails, usually on macOS jobs.

Regarding instance variables in setup, Minitest runs every test in a new instance. Hence, creating strings, etc really shouldn't be considered bad. Removed all the past object code, so objects are only created as needed.

[nateberkopec]

Minitest runs every test in a new instance. Hence, creating strings, etc really shouldn't be considered bad.

Yeah it's more just the ugliness than the actual correctness that inspired that TODO

[nateberkopec]

This is a little too parallel with my existing integration cleanup PR. I'm going to prioritize that one. I just have to figure out why it's not working on Linux.

[MSP-Greg]

@nateberkopec

This is a little too parallel with my existing integration cleanup PR

I just pushed what I can do with this.

I saw your integration_cleanup branch, and tried working with it, but starting with something that's broken can be nonproductive. Also, the differences between Ubuntu & macOS are beyond my pay grade...

When you asked for changes in test_integration.rb on PR #1908, I decided to add some of things I had done with your branch, and this is the result of that.

1. i tried creating a 'test_phased_restart_via_pumactl' test that used 'server' instead of 'run_launcher', and it kept failing in Puma control_cli.rb code. For now, I give up.

2. You renamed a few tests, and I agree that the names are cluttered. At present, we've got suffixes of 'workers', 'on_forked_servers', 'clustered_mode', and a prefix of 'test_workers', etc. Maybe if there are two companion tests or a specific test, simply a 'single' or 'cluster' suffix might be used?

3. test_phased_restart_via_pumactl is similar to test_worker_phased_restart. I added a time based assert to the first, as it's timing should be governed by 'worker_shutdown_timeout'.

Hence, feel free to use whatever. Some of the code I added (like @ios_to_close) is just 'good practices', and allows objects to be created without doing so as instance variables.

I'm pissed about the macOS intermittent failures. I think we're all tired of the need to look at test logs due to test stability issues...

[MSP-Greg]

@nateberkopec

While AFK, had a few thoughts, and I think that with the 2nd commit, this is similar to what you're looking for. Many of the tests here are end-to-end, maybe a few should move elsewhere, like the 'stuck_workers' tests. Some of the common methods may be applicable to other test files.

I'll probably jinx it by mentioning it, but the macOS jobs have been passing & stable in my fork while working on the 2nd commit...

[nateberkopec]

Cool! There's some things you were doing in here that I prefer, I just need to sit down and "merge" the two PRs so they're just the way I like.

[MSP-Greg]

@nateberkopec

Cool. I added one more commit that adds another 'shared method'... Now I'm really finished.

Please give some thought to moving some of these tests to a test_cluster.rb file.

I think integration should test if pumactl and signals are generating the correct actions in Puma, but the tests that check whether cluster.rb is actually handling timing, TERM vs KILL, etc might be better placed elsewhere...

[MSP-Greg]

Errors in Travis 3783 fixed by PR #1932

[nateberkopec]

Merged w/ 9efee4e