From 7036574ec18ebb337b01678de9621242f5f5785e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 31 Jan 2013 20:44:55 +0000 Subject: [PATCH] Get the port from the server Ask, don't tell, or something like that! Now the WebSocketServer decides what port it will use and everything else relies on that. By default an ephemeral port is used, but a fixed port number can be passed in too. This work is necessary to support Windows (#240) because on Windows a TCP socket cannot be closed and immediately reopened, which we are doing in the tests. So this change means that when restarting, a new ephemeral port can be used by the server. --- lib/capybara/poltergeist.rb | 1 - lib/capybara/poltergeist/client.rb | 20 +++++++++---------- lib/capybara/poltergeist/driver.rb | 7 ++----- lib/capybara/poltergeist/server.rb | 14 ++++++++----- lib/capybara/poltergeist/util.rb | 12 ----------- lib/capybara/poltergeist/web_socket_server.rb | 17 ++++++++++------ spec/integration/driver_spec.rb | 2 +- spec/unit/client_spec.rb | 16 ++++++++------- 8 files changed, 41 insertions(+), 48 deletions(-) delete mode 100644 lib/capybara/poltergeist/util.rb diff --git a/lib/capybara/poltergeist.rb b/lib/capybara/poltergeist.rb index 5767e5d..0948050 100644 --- a/lib/capybara/poltergeist.rb +++ b/lib/capybara/poltergeist.rb @@ -17,7 +17,6 @@ module Capybara require 'capybara/poltergeist/network_traffic' require 'capybara/poltergeist/errors' require 'capybara/poltergeist/cookie' - require 'capybara/poltergeist/util' end end diff --git a/lib/capybara/poltergeist/client.rb b/lib/capybara/poltergeist/client.rb index 35fc132..a5e6aae 100644 --- a/lib/capybara/poltergeist/client.rb +++ b/lib/capybara/poltergeist/client.rb @@ -14,10 +14,10 @@ module Capybara::Poltergeist client end - attr_reader :pid, :port, :path, :window_size, :phantomjs_options + attr_reader :pid, :server, :path, :window_size, :phantomjs_options - def initialize(port, options = {}) - @port = port + def initialize(server, options = {}) + @server = server @path = options[:path] || PHANTOMJS_NAME @window_size = options[:window_size] || [1024, 768] @phantomjs_options = options[:phantomjs_options] || [] @@ -57,14 +57,12 @@ module Capybara::Poltergeist end def command - @command ||= begin - parts = [path] - parts.concat phantomjs_options - parts << PHANTOMJS_SCRIPT - parts << port - parts.concat window_size - parts - end + parts = [path] + parts.concat phantomjs_options + parts << PHANTOMJS_SCRIPT + parts << server.port + parts.concat window_size + parts end private diff --git a/lib/capybara/poltergeist/driver.rb b/lib/capybara/poltergeist/driver.rb index dc77e3c..6c2a26d 100644 --- a/lib/capybara/poltergeist/driver.rb +++ b/lib/capybara/poltergeist/driver.rb @@ -34,14 +34,11 @@ module Capybara::Poltergeist end def server - @server ||= Server.new( - options.fetch(:port) { Util.find_available_port }, - options.fetch(:timeout) { DEFAULT_TIMEOUT } - ) + @server ||= Server.new(options[:port], options.fetch(:timeout) { DEFAULT_TIMEOUT }) end def client - @client ||= Client.start(server.port, + @client ||= Client.start(server, :path => options[:phantomjs], :window_size => options[:window_size], :phantomjs_options => phantomjs_options, diff --git a/lib/capybara/poltergeist/server.rb b/lib/capybara/poltergeist/server.rb index e4be0c3..4c47148 100644 --- a/lib/capybara/poltergeist/server.rb +++ b/lib/capybara/poltergeist/server.rb @@ -1,19 +1,23 @@ module Capybara::Poltergeist class Server - attr_reader :port, :socket, :timeout + attr_reader :socket, :fixed_port, :timeout - def initialize(port, timeout = nil) - @port = port - @timeout = timeout + def initialize(port = nil, timeout = nil) + @fixed_port = fixed_port + @timeout = timeout start end + def port + @socket.port + end + def timeout=(sec) @timeout = @socket.timeout = sec end def start - @socket = WebSocketServer.new(port, timeout) + @socket = WebSocketServer.new(fixed_port, timeout) end def stop diff --git a/lib/capybara/poltergeist/util.rb b/lib/capybara/poltergeist/util.rb deleted file mode 100644 index 737a8ed..0000000 --- a/lib/capybara/poltergeist/util.rb +++ /dev/null @@ -1,12 +0,0 @@ -require 'socket' - -module Capybara::Poltergeist - module Util - def self.find_available_port - server = TCPServer.new('127.0.0.1', 0) - server.addr[1] - ensure - server.close if server - end - end -end diff --git a/lib/capybara/poltergeist/web_socket_server.rb b/lib/capybara/poltergeist/web_socket_server.rb index c71f68d..2e956bd 100644 --- a/lib/capybara/poltergeist/web_socket_server.rb +++ b/lib/capybara/poltergeist/web_socket_server.rb @@ -57,21 +57,26 @@ module Capybara::Poltergeist # How many seconds to try to bind to the port for before failing BIND_TIMEOUT = 5 + HOST = '127.0.0.1' + attr_reader :port, :parser, :socket, :handler, :server attr_accessor :timeout - def initialize(port, timeout = nil) - @port = port - @parser = Http::Parser.new - @server = start_server + def initialize(port = nil, timeout = nil) @timeout = timeout + @parser = Http::Parser.new + @server = start_server(port) end - def start_server + def port + server.addr[1] + end + + def start_server(port) time = Time.now begin - TCPServer.open(port) + TCPServer.open(HOST, port || 0) rescue Errno::EADDRINUSE if (Time.now - time) < BIND_TIMEOUT sleep(0.01) diff --git a/spec/integration/driver_spec.rb b/spec/integration/driver_spec.rb index 96af8b9..fe2044b 100644 --- a/spec/integration/driver_spec.rb +++ b/spec/integration/driver_spec.rb @@ -54,7 +54,7 @@ module Capybara::Poltergeist end end - it 'raises an error and restart the client, if the client dies while executing a command' do + it 'raises an error and restarts the client, if the client dies while executing a command' do lambda { @driver.browser.command('exit') }.should raise_error(DeadClient) @session.visit('/') @driver.html.should include('Hello world') diff --git a/spec/unit/client_spec.rb b/spec/unit/client_spec.rb index 3979f08..e7901f2 100644 --- a/spec/unit/client_spec.rb +++ b/spec/unit/client_spec.rb @@ -2,7 +2,9 @@ require 'spec_helper' module Capybara::Poltergeist describe Client do - subject { Client.new(6000) } + let(:server) { stub(port: 6000) } + + subject { Client.new(server) } it 'raises an error if phantomjs is too old' do `true` # stubbing $? @@ -21,7 +23,7 @@ module Capybara::Poltergeist end it 'raises an error if phantomjs returns a non-zero exit code' do - subject = Client.new(6000, :path => 'exit 42 && ') + subject = Client.new(server, :path => 'exit 42 && ') expect { subject.start }.to raise_error(Error) begin @@ -32,16 +34,16 @@ module Capybara::Poltergeist end context 'with width and height specified' do - subject { Client.new(6000, :window_size => [800, 600]) } + subject { Client.new(server, :window_size => [800, 600]) } it 'starts phantomjs, passing the width and height through' do Process.should_receive(:spawn).with("phantomjs", Client::PHANTOMJS_SCRIPT, "6000", "800", "600", out: $stdout) subject.start end end - + context 'with a custom phantomjs_logger' do - subject { Client.new(6000, :phantomjs_logger => :my_custom_logger, :window_size => [800, 600]) } + subject { Client.new(server, :phantomjs_logger => :my_custom_logger, :window_size => [800, 600]) } it 'starts phantomjs, capturing the STDOUT to custom phantomjs_logger' do Process.should_receive(:spawn).with("phantomjs", Client::PHANTOMJS_SCRIPT, "6000", "800", "600", out: :my_custom_logger) @@ -50,7 +52,7 @@ module Capybara::Poltergeist end context 'with additional command-line options' do - subject { Client.new(6000, :phantomjs_options => %w[--ignore-ssl-error=yes --load-images=no]) } + subject { Client.new(server, :phantomjs_options => %w[--ignore-ssl-error=yes --load-images=no]) } it 'passed additional command-line options to phantomjs' do Process.should_receive(:spawn).with("phantomjs", '--ignore-ssl-error=yes', '--load-images=no', anything, anything, anything, anything, out: $stdout) @@ -64,7 +66,7 @@ module Capybara::Poltergeist alias old_wait wait end - client = Client.new(1234) + client = Client.new(server) Process.stub(spawn: 5678) client.start