-
-
Notifications
You must be signed in to change notification settings - Fork 62
Description
Background
While implementing a multi-mode E2E test runner for React on Rails demos (shakacode/react_on_rails-demos#17), we discovered some improvements that could benefit the server process management in cypress-on-rails.
Suggested Improvements
1. Add Warning Logging for Process Group Kill Failures
Current behavior: When Process.kill raises Errno::EPERM, there's silent fallback which makes debugging difficult.
Suggestion: Add warning logs with process group ID and error details:
def send_term_signal
if @server_pgid
Process.kill('TERM', -@server_pgid)
else
safe_kill_process('TERM', @server_pid)
end
rescue Errno::ESRCH, Errno::EPERM => e
puts "Warning: Failed to kill process group #{@server_pgid}: #{e.message}, trying single process"
safe_kill_process('TERM', @server_pid)
end2. Improve Server Readiness Check
Current behavior (assumed): May accept any non-error response as "ready"
Suggestion: Be more precise about what constitutes a "ready" server:
def server_responding?
url = "http://localhost:#{@port}"
response = Net::HTTP.get_response(URI(url))
# Accept 200-399 (success and redirects), reject 404 and 5xx
(200..399).cover?(response.code.to_i)
rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, SocketError
false
endRationale: A 404 response means the server is running but the app isn't ready. Accepting only 2xx-3xx status codes provides a more accurate health check.
Benefits
- Better debugging: Process termination issues become visible in logs
- More reliable tests: Server readiness checks won't pass prematurely
- Fewer flaky tests: Proper health checks reduce timing-related failures
Implementation Note
These improvements were tested in production use with Playwright tests across multiple server configurations. See the implementation at: https://github.com/shakacode/react_on_rails-demos/pull/17/files
Would you be interested in a PR implementing these improvements?