-
Notifications
You must be signed in to change notification settings - Fork 0
Closed
Description
Context
From PR #13 code review - medium priority improvements for robustness and completeness.
Issues to Address
1. Incomplete Implementation (demo_creator.rb:540-544)
Problem: Method run_automated_tests is defined but empty.
Current code:
def run_automated_tests
# TODO: Implement automated testing
endRecommendation:
- Either implement it using
E2eTestRunner - Or remove it entirely if not needed
- Document decision in commit message
2. Race Condition in Port Release (e2e_test_runner.rb:53-55)
Problem: Fixed 2-second delay is fragile and may not be enough on slow systems.
Current code:
def cleanup_between_modes
puts 'Waiting 2 seconds for server to release port...'
sleep 2
endRecommendation: Poll for port availability instead of fixed sleep.
def cleanup_between_modes
puts 'Waiting for server to release port...'
max_attempts = 10
max_attempts.times do
return true unless port_in_use?(@port)
sleep 0.5
end
puts "Warning: Port #{@port} may still be in use"
end
def port_in_use?(port)
TCPServer.new('localhost', port).close
false
rescue Errno::EADDRINUSE
true
end3. Missing Timeout on HTTP Request (e2e_test_runner.rb:146-152)
Problem: No timeout set, could hang indefinitely if server is stuck.
Current code:
def server_responding?
url = "http://localhost:#{@port}"
response = Net::HTTP.get_response(URI(url))
response.code.to_i < 500
rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, SocketError
false
endRecommendation: Add read/open timeouts (2 seconds).
def server_responding?
url = URI("http://localhost:#{@port}")
response = Net::HTTP.start(url.host, url.port,
open_timeout: 2,
read_timeout: 2) do |http|
http.get(url.path.empty? ? '/' : url.path)
end
(200..399).cover?(response.code.to_i)
rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, SocketError, Net::OpenTimeout, Net::ReadTimeout
false
end4. RuboCop Configuration Change (.rubocop.yml:3)
Problem: Changed from plugins: to require: which may cause compatibility issues.
Current change:
require:
- rubocop-performanceWarning message:
rubocop-performance extension supports plugin, specify `plugins: rubocop-performance`
instead of `require: rubocop-performance`
Recommendation:
- Investigate why
plugins:syntax caused syntax error - Ensure all team members have compatible RuboCop versions (>= 1.0)
- Update to use
plugins:syntax if possible - Document minimum RuboCop version in README
Testing
- Add tests for timeout scenarios
- Test port polling with slow shutdowns
- Verify RuboCop config works across different versions
Acceptance Criteria
-
run_automated_testsimplemented or removed - Port polling replaces fixed sleep
- HTTP timeouts prevent hangs
- RuboCop configuration clarified/fixed
- All tests pass
Metadata
Metadata
Assignees
Labels
No labels