Skip to content
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

TestCLI and TestIntegration need a ground-up rethink #1841

Closed
nateberkopec opened this issue Jul 11, 2019 · 5 comments
Closed

TestCLI and TestIntegration need a ground-up rethink #1841

nateberkopec opened this issue Jul 11, 2019 · 5 comments

Comments

@nateberkopec
Copy link
Member

These tests are failing/exceptioning like crazy even in CI (and still passing somehow? IDK!). We need a ground up rethink of these classes and how we test this stuff, because the globbed-on, piecemeal approach we've used over the years is falling apart.

@MSP-Greg
Copy link
Member

I looked quickly for builds right after the change to Ruby >= 2.5 was reverted, and (for instance) Travis Build 3471 2019-06-14 seems fine.

So it's not that bad...

A lot of the odd tests (exceptions, etc) seem to involve fork or unix sockets so I can't personally check any thing locally.

@MSP-Greg
Copy link
Member

MSP-Greg commented Jul 11, 2019

@nateberkopec

Needed a break from my Ruby/JS mess.

Travis 3517 is ok, Errno::ECONNREFUSED exceptions started with 3519, which is PR #1829

@MSP-Greg
Copy link
Member

@nateberkopec

Being a windows type, I can't run/test a lot of Puma functionality without fork & unix sockets. I also don't have a cloud server available to test on. Hence, I'm probably totally wrong.

It seems there are issues both before and after #1829.

Looking at Binder#close before the pr/commit:

puma/lib/puma/binder.rb

Lines 51 to 60 in 1814008

def close
@ios.each { |i| i.close }
@unix_paths.each do |i|
# Errno::ENOENT is intermittently raised
begin
File.unlink i
rescue Errno::ENOENT
end
end
end

and after:

puma/lib/puma/binder.rb

Lines 51 to 62 in 13567c9

def close
@ios.each { |i| i.close }
@unix_paths.each do |i|
# Errno::ENOENT is intermittently raised
begin
unix_socket = UNIXSocket.new i
unix_socket.close
rescue Errno::ENOENT
end
end
end

Maybe I've got it all wrong, but it seems to me that truly closing Puma should unlink everything in @unix_paths, but a re-start should not. A restart should be able to reconnect/recreate the UnixServer's that are listeners?

Also, from throwing some things at Travis, it seems that Binder has no way to determine if it's a 'hard shutdown' or a restart (with a `graceful shutdown')?

Lastly, re testing, I don't know if some of the UnixSockets are using temp files for every instance, or whether the same (temp) file should used in all tests, and hence, allowing tests to see if Puma is removing them when it should. Same may hold for tcp ports...

I'm probably totally wrong... Have a good weekend...

@nateberkopec
Copy link
Member Author

Integration tests are much improved, just the CLI tests need work now

Sent with GitHawk

@MSP-Greg
Copy link
Member

just the CLI tests need work now

Anything in particular?

Are you thinking of starting Puma::CLI.new [<args>] with ruby (or bundle exec ruby) via IO.popen?

Might be able to keep the code in the test methods by using something like test/helper.rb in #1952...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants