Server#responsive? now rescues all SystemCallError exceptions

[jwilger]

It previously only rescued Errno::ECONNREFUSED and Errno::EBADF. On a
few occasions, I have also seen the Errno::ETIMEDOUT exception raised by
Net::HTTP from this method. Since the Net::HTTP calls are the only thing
in this method that should be making any kind of system calls, a rescue
of all SystemCallError exceptions (the superclass of all of the Errno
exceptions) will catch these as well as any other system call errors
triggered by Net::HTTP.

[jnicklas]

I kind of feel that those tests aren't particularly meaningful. Way too many mocks.

[jwilger]

The mocks are at system boundaries and are only replacing behavior of well-documented and stable APIs. The only possible exception to that is the stub on #server_thread; I suppose the test around thread joining could be tossed out to make that unnecessary (since that would then just be nil and the condition would be passed over), but I included it for the sake of complete coverage.

Short of mocking, what would you suggest to cause the Net::HTTP calls to raise a SystemCallError? Clearly at some level you need to manipulate something and cause it to fail to get that exception. A test specifically for something easy to produce, such as ECONNREFUSED, would not be sufficient, given that it would not catch the issue prompting the patch. I suppose we could add a complete wrapper around Net::HTTP that allowed specific errors to be triggered and use that in the production code, but that seems like overkill and is just another form of mocking anyway. The difficulty in testing this in any meaningful way without mocks is likely evident in the fact that there is seemingly no test coverage of this behavior prior to this change.

[jwilger]

Crap, that comes across more confrontational than I intended. If you can think of a better way to test that without the mocks, I am genuinely interested. :-)

[jnicklas]

I guess this comes down to the eternal debate about mocks vs no mocks. In that debate I land on the "mock as little as possible" side. I find mocks inherently clumsy and unreliable, especially when mocking third party libraries. Given that you have experienced this error, there must be some way of reproducing it, if there isn't, then why is worth fixing in the first place?

[jwilger]

It comes up intermittently. Run Cabybara::Server#boot once and It bombs on the ETIMEDOUT error, run it again and it works fine. Ultimately it's something from either the underlying Ruby code or the OS, since those are system call error codes. There may be someone out there who could easily cause that to be reproduced on demand, but not I.

If you like, I can update this and remove the tests that aren't specifically about this error condition (I only added them because it seemed like more coverage would be a good thing). That should at least reduce the amount of mocking to only the one Net::HTTP.stub!(:start).and_raise(SystemCallError.allocate), and it would adequately cover the case for which I'm submitting the patch.

[jnicklas]

Thanks!