Improve process cleanup and server readiness checks in E2eTestRunner (#14)

[justin808]

Summary

• Add warning logging when process group kill fails with EPERM to aid debugging
• Log process group ID when fallback to single process kill occurs
• Fix server_responding? to reject 404 responses
• Accept 200-399 status codes (success and redirects) instead of all <500
• Add comprehensive test coverage for EPERM handling and 404 rejection
• Add test for 3xx redirect handling

Changes Made

Process Cleanup Improvements

• Modified send_term_signal and send_kill_signal to log warnings when Process.kill raises Errno::EPERM
• Warning message includes process group ID and error details for easier debugging
• Ensures visibility into process termination issues

Server Readiness Check Improvements

• Changed server_responding? from accepting any response <500 to accepting only 200-399
• Now properly rejects 404 responses, which indicate the server is not ready
• Accepts redirects (3xx) as valid server responses
• More precise health check for server startup

Test Coverage

• Added test for EPERM handling with warning logging verification
• Updated existing test to reflect 404s should return false
• Added test for 3xx redirect handling
• All 31 tests passing

Closes

Closes #14

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 38 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 72f666e and 680b021.

📒 Files selected for processing (4)

• packages/shakacode_demo_common/.gitignore (1 hunks)
• packages/shakacode_demo_common/lib/shakacode_demo_common/e2e_test_runner.rb (3 hunks)
• packages/shakacode_demo_common/spec/examples.txt (0 hunks)
• packages/shakacode_demo_common/spec/lib/shakacode_demo_common/e2e_test_runner_spec.rb (2 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/fix-e2e-process-cleanup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

PR Review: Improve process cleanup and server readiness checks in E2eTestRunner (#14)

Summary

This PR successfully addresses the issues identified in #14 with solid improvements to process cleanup and server readiness detection. The changes are well-implemented with comprehensive test coverage.

✅ Code Quality & Best Practices

Strengths:

• Clear intent: The code changes directly address the documented issues with appropriate solutions
• Good documentation: Inline comments explain the logic (e.g., e2e_test_runner.rb:149)
• Ruby conventions: Proper exception handling with captured error messages
• Test organization: Well-structured specs with clear context groupings

Minor suggestions:

• The warning messages use puts for logging (e2e_test_runner.rb:176, e2e_test_runner.rb:188). Consider whether a proper logger would be more appropriate for production use, though puts is acceptable for test utilities.

✅ Bug Fixes & Correctness

Server Readiness Check (e2e_test_runner.rb:146-153):

• Excellent fix: Changing from < 500 to (200..399).cover? correctly identifies when the server is truly ready
• Proper handling: 404 responses now correctly indicate the server is not ready (likely hitting a non-existent route)
• Redirect support: Accepting 3xx responses is appropriate for servers that redirect root to other paths

Process Cleanup (e2e_test_runner.rb:174-189):

• Good error handling: Capturing the exception and logging details aids debugging
• Correct fallback: Falls back to single process kill when EPERM occurs
• Includes context: Logging @server_pgid helps identify which process group failed

✅ Performance Considerations

No performance concerns. The changes are minimal and don't introduce any new bottlenecks.

✅ Security Concerns

No security issues identified. The changes improve robustness without introducing vulnerabilities.

✅ Test Coverage

Excellent test coverage for all changes:

1. EPERM handling (e2e_test_runner_spec.rb:210-217):

   • ✅ Verifies warning is logged
   • ✅ Tests fallback to single process kill
   • ✅ Good use of expectations for behavior verification

2. 404 rejection (e2e_test_runner_spec.rb:267-270):

   • ✅ Updated test correctly expects false for 404 responses
   • ✅ Proper test name change from "returns true" to "returns false"

3. 3xx redirect support (e2e_test_runner_spec.rb:262-265):

   • ✅ New test verifies redirects are accepted as valid responses

Test quality:

• All test descriptions accurately reflect behavior
• Good use of mocking/stubbing to isolate units
• Edge cases covered (EPERM, ESRCH, connection refused, etc.)

Minor Observations

1. Test expectation at e2e_test_runner_spec.rb:214: The test expects Process.kill with ('TERM', 12345) to raise StandardError. This works but could be more explicit about what error is expected or use and_call_original if that's the intent.

2. Consistency: Both send_term_signal and send_kill_signal have identical EPERM handling logic. This is good for consistency, though you could consider extracting it to a helper method if similar logic appears elsewhere. Not required for this PR.

Recommendation

Approve ✅

This PR:

• Solves the identified issues from High Priority: Improve process cleanup and server readiness checks in E2eTestRunner #14
• Includes comprehensive tests (31 passing)
• Follows Ruby best practices
• Has clear documentation
• No security or performance concerns

The implementation is clean, well-tested, and ready to merge.

[claude]

Pull Request Review: PR #17

Summary

This PR successfully addresses all requirements from issue #14, improving the robustness and debuggability of the E2eTestRunner. The implementation is clean, well-tested, and follows Ruby best practices.

✅ Code Quality & Best Practices

Strengths:

• Clean implementation: The code changes are minimal, focused, and easy to understand
• Proper error handling: Exception handling now captures error details (=> e) and logs them appropriately
• Good variable naming: Clear and descriptive throughout
• Consistent style: Follows Ruby conventions and existing codebase patterns
• Inline documentation: Helpful comments explain the intent (e.g., lines 149-150 in e2e_test_runner.rb)

Minor suggestions:

1. Consider using warn instead of puts for warning messages (lines 176, 188) for better semantics - this would respect Ruby stderr conventions and make it easier to filter logs.

✅ Potential Bugs or Issues

Good catches:

• The 404 rejection fix (line 150) is crucial - accepting 404s as server ready was definitely a bug
• The EPERM logging (lines 174-177, 187-189) provides valuable debugging information

No issues found - The implementation correctly:

• Handles all edge cases in process termination
• Properly falls back from process group to single process kill
• Maintains existing error handling for Errno::ESRCH

✅ Performance Considerations

All good:

• No performance regressions introduced
• The additional logging has negligible overhead (only on error paths)
• HTTP status code check using Range#cover? is efficient

✅ Security Concerns

No security issues identified:

• Process signal handling is done safely
• No exposure of sensitive information in logs
• Error messages are appropriately scoped

✅ Test Coverage

Excellent test coverage:

• ✅ New test for EPERM warning logging (lines 210-217)
• ✅ Updated test for 404 rejection (lines 267-270)
• ✅ New test for 3xx redirect acceptance (lines 262-265)
• ✅ All 31 tests passing
• ✅ Test structure follows existing patterns well

Observations:

• The EPERM test properly verifies both warning output and fallback behavior
• Good use of RSpec doubles and expectations
• Tests cover both happy path and error scenarios

📋 Additional Notes

File cleanup:

• ✅ Good addition of spec/examples.txt to .gitignore - this is a generated file that should not be committed

Documentation:

• The PR description is comprehensive and clearly explains all changes
• Inline code comments are helpful and accurate

🎯 Recommendation

APPROVE - This PR is ready to merge.

All requirements from issue #14 have been successfully implemented:

• ✅ Warning logged when process group kill fails with EPERM
• ✅ Process group ID included in warning messages
• ✅ Server readiness check rejects 404 responses
• ✅ Accepts 200-399 status codes (success and redirects)
• ✅ Comprehensive test coverage added
• ✅ All existing tests pass

The only minor suggestion is considering warn instead of puts for warning messages, but this is optional and does not block merging.

Great work addressing all the feedback from the code review! 🎉

---

Review generated by Claude Code