Use constants over class instance variables in Service classes

SeleniumHQ · Apr 26, 2019 · 44d6a8b · 44d6a8b
1 parent c205391
commit 44d6a8b
Show file tree

Hide file tree

Showing 12 changed files with 44 additions and 54 deletions.
diff --git a/rb/lib/selenium/webdriver/chrome/service.rb b/rb/lib/selenium/webdriver/chrome/service.rb
@@ -25,19 +25,14 @@ module Chrome
       #
 
       class Service < WebDriver::Service
-        @default_port = 9515
-        @executable = 'chromedriver'
-        @missing_text = <<~ERROR
+        DEFAULT_PORT = 9515
+        EXECUTABLE = 'chromedriver'
+        MISSING_TEXT = <<~ERROR
           Unable to find chromedriver. Please download the server from
           https://chromedriver.storage.googleapis.com/index.html and place it somewhere on your PATH.
           More info at https://github.com/SeleniumHQ/selenium/wiki/ChromeDriver.
         ERROR
-        @shutdown_supported = true
-
-        def self.driver_path=(path)
-          Platform.assert_executable path if path.is_a?(String)
-          @driver_path = path
-        end
+        SHUTDOWN_SUPPORTED = true
 
         private
 

diff --git a/rb/lib/selenium/webdriver/common/service.rb b/rb/lib/selenium/webdriver/common/service.rb
@@ -29,13 +29,8 @@ class Service
       SOCKET_LOCK_TIMEOUT = 45
       STOP_TIMEOUT = 20
 
-      @default_port = nil
-      @driver_path = nil
-      @executable = nil
-      @missing_text = nil
-
       class << self
-        attr_reader :default_port, :driver_path, :executable, :missing_text, :shutdown_supported
+        attr_reader :driver_path
 
         def chrome(*args)
           Chrome::Service.new(*args)
@@ -75,7 +70,7 @@ def driver_path=(path)
 
       def initialize(path: nil, port: nil, args: nil)
         path ||= self.class.driver_path
-        port ||= self.class.default_port
+        port ||= self.class::DEFAULT_PORT
         args ||= []
 
         @executable_path = binary_path(path)
@@ -100,7 +95,7 @@ def start
       end
 
       def stop
-        return unless self.class.shutdown_supported
+        return unless self.class::SHUTDOWN_SUPPORTED
 
         stop_server
         @process.poll_for_exit STOP_TIMEOUT
@@ -118,9 +113,9 @@ def uri
 
       def binary_path(path = nil)
         path = path.call if path.is_a?(Proc)
-        path ||= Platform.find_binary(self.class.executable)
+        path ||= Platform.find_binary(self.class::EXECUTABLE)
 
-        raise Error::WebDriverError, self.class.missing_text unless path
+        raise Error::WebDriverError, self.class::MISSING_TEXT unless path
 
         Platform.assert_executable path
         path
@@ -188,7 +183,7 @@ def connect_until_stable
       end
 
       def cannot_connect_error_text
-        "unable to connect to #{self.class.executable} #{@host}:#{@port}"
+        "unable to connect to #{self.class::EXECUTABLE} #{@host}:#{@port}"
       end
 
       def socket_lock

diff --git a/rb/lib/selenium/webdriver/edge/service.rb b/rb/lib/selenium/webdriver/edge/service.rb
@@ -25,14 +25,14 @@ module Edge
       #
 
       class Service < WebDriver::Service
-        @default_port = 17556
-        @executable = 'MicrosoftWebDriver'
-        @missing_text = <<~ERROR
+        DEFAULT_PORT = 17556
+        EXECUTABLE = 'MicrosoftWebDriver'
+        MISSING_TEXT = <<~ERROR
           Unable to find MicrosoftWebDriver. Please download the server from
           https://www.microsoft.com/en-us/download/details.aspx?id=48212 and place it somewhere on your PATH.
           More info at https://github.com/SeleniumHQ/selenium/wiki/MicrosoftWebDriver.
         ERROR
-        @shutdown_supported = true
+        SHUTDOWN_SUPPORTED = true
 
         private
 

diff --git a/rb/lib/selenium/webdriver/firefox/service.rb b/rb/lib/selenium/webdriver/firefox/service.rb
@@ -25,14 +25,14 @@ module Firefox
       #
 
       class Service < WebDriver::Service
-        @default_port = 4444
-        @executable = 'geckodriver'
-        @missing_text = <<~ERROR
+        DEFAULT_PORT = 4444
+        EXECUTABLE = 'geckodriver'
+        MISSING_TEXT = <<~ERROR
           Unable to find Mozilla geckodriver. Please download the server from
           https://github.com/mozilla/geckodriver/releases and place it somewhere on your PATH.
           More info at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/WebDriver.
         ERROR
-        @shutdown_supported = false
+        SHUTDOWN_SUPPORTED = false
 
         private
 

diff --git a/rb/lib/selenium/webdriver/ie/service.rb b/rb/lib/selenium/webdriver/ie/service.rb
@@ -25,14 +25,14 @@ module IE
       #
 
       class Service < WebDriver::Service
-        @default_port = 5555
-        @executable = 'IEDriverServer'
-        @missing_text = <<~ERROR
+        DEFAULT_PORT = 5555
+        EXECUTABLE = 'IEDriverServer'
+        MISSING_TEXT = <<~ERROR
           Unable to find IEDriverServer. Please download the server from
           http://selenium-release.storage.googleapis.com/index.html and place it somewhere on your PATH.
           More info at https://github.com/SeleniumHQ/selenium/wiki/InternetExplorerDriver.
         ERROR
-        @shutdown_supported = true
+        SHUTDOWN_SUPPORTED = true
 
         private
 

diff --git a/rb/lib/selenium/webdriver/safari/service.rb b/rb/lib/selenium/webdriver/safari/service.rb
@@ -25,13 +25,13 @@ module Safari
       #
 
       class Service < WebDriver::Service
-        @default_port = 7050
-        @executable = 'safaridriver'
-        @missing_text = <<~ERROR
+        DEFAULT_PORT = 7050
+        EXECUTABLE = 'safaridriver'
+        MISSING_TEXT = <<~ERROR
           Unable to find Apple's safaridriver which comes with Safari 10.
           More info at https://webkit.org/blog/6900/webdriver-support-in-safari-10/
         ERROR
-        @shutdown_supported = false
+        SHUTDOWN_SUPPORTED = false
       end # Service
     end # Safari
   end # WebDriver

diff --git a/rb/spec/unit/selenium/webdriver/chrome/service_spec.rb b/rb/spec/unit/selenium/webdriver/chrome/service_spec.rb
@@ -22,7 +22,7 @@
 module Selenium
   module WebDriver
     describe Service do
-      let(:service_path) { "/path/to/#{Chrome::Service.executable}" }
+      let(:service_path) { "/path/to/#{Chrome::Service::EXECUTABLE}" }
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
@@ -34,8 +34,8 @@ module WebDriver
 
           service = Service.chrome
 
-          expect(service.executable_path).to include Chrome::Service.executable
-          expected_port = Chrome::Service.default_port
+          expect(service.executable_path).to include Chrome::Service::EXECUTABLE
+          expected_port = Chrome::Service::DEFAULT_PORT
           expect(service.uri.to_s).to eq "http://#{Platform.localhost}:#{expected_port}"
         end
 

diff --git a/rb/spec/unit/selenium/webdriver/common/service_spec.rb b/rb/spec/unit/selenium/webdriver/common/service_spec.rb
@@ -26,16 +26,18 @@ module WebDriver
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
+        stub_const('Selenium::WebDriver::Service::DEFAULT_PORT', 1234)
+        stub_const('Selenium::WebDriver::Service::EXECUTABLE', 'service')
       end
 
       describe '#new' do
         it 'uses default path and port' do
           expect(Platform).to receive(:find_binary).and_return(service_path)
           expect(described_class).to receive(:driver_path)
-          expect(described_class).to receive(:default_port).and_return(1234)
 
           service = Service.new
           expect(service.executable_path).to eq service_path
+          expect(service.uri.to_s).to eq "http://#{Platform.localhost}:1234"
         end
 
         it 'uses provided path and port' do
@@ -51,7 +53,6 @@ module WebDriver
         it 'does not create args by default' do
           expect(Platform).to receive(:find_binary).and_return(service_path)
           expect(described_class).to receive(:driver_path)
-          expect(described_class).to receive(:default_port).and_return(1)
 
           service = Service.new
           expect(service.instance_variable_get('@extra_args')).to be_empty
@@ -60,7 +61,6 @@ module WebDriver
         it 'uses provided args' do
           expect(Platform).to receive(:find_binary).and_return(service_path)
           expect(described_class).to receive(:driver_path)
-          expect(described_class).to receive(:default_port).and_return(1)
 
           service = Service.new(args: ['--foo', '--bar'])
           expect(service.instance_variable_get('@extra_args')).to eq ['--foo', '--bar']

diff --git a/rb/spec/unit/selenium/webdriver/edge/service_spec.rb b/rb/spec/unit/selenium/webdriver/edge/service_spec.rb
@@ -22,7 +22,7 @@
 module Selenium
   module WebDriver
     describe Service do
-      let(:service_path) { "/path/to/#{Edge::Service.executable}" }
+      let(:service_path) { "/path/to/#{Edge::Service::EXECUTABLE}" }
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
@@ -34,8 +34,8 @@ module WebDriver
 
           service = Service.edge
 
-          expect(service.executable_path).to include Edge::Service.executable
-          expected_port = Edge::Service.default_port
+          expect(service.executable_path).to include Edge::Service::EXECUTABLE
+          expected_port = Edge::Service::DEFAULT_PORT
           expect(service.uri.to_s).to eq "http://#{Platform.localhost}:#{expected_port}"
         end
 

diff --git a/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb b/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb
@@ -22,7 +22,7 @@
 module Selenium
   module WebDriver
     describe Service do
-      let(:service_path) { "/path/to/#{Firefox::Service.executable}" }
+      let(:service_path) { "/path/to/#{Firefox::Service::EXECUTABLE}" }
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
@@ -34,8 +34,8 @@ module WebDriver
 
           service = Service.firefox
 
-          expect(service.executable_path).to include Firefox::Service.executable
-          expected_port = Firefox::Service.default_port
+          expect(service.executable_path).to include Firefox::Service::EXECUTABLE
+          expected_port = Firefox::Service::DEFAULT_PORT
           expect(service.uri.to_s).to eq "http://#{Platform.localhost}:#{expected_port}"
         end
 

diff --git a/rb/spec/unit/selenium/webdriver/ie/service_spec.rb b/rb/spec/unit/selenium/webdriver/ie/service_spec.rb
@@ -22,7 +22,7 @@
 module Selenium
   module WebDriver
     describe Service do
-      let(:service_path) { "/path/to/#{IE::Service.executable}" }
+      let(:service_path) { "/path/to/#{IE::Service::EXECUTABLE}" }
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
@@ -34,8 +34,8 @@ module WebDriver
 
           service = Service.ie
 
-          expect(service.executable_path).to include IE::Service.executable
-          expected_port = IE::Service.default_port
+          expect(service.executable_path).to include IE::Service::EXECUTABLE
+          expected_port = IE::Service::DEFAULT_PORT
           expect(service.uri.to_s).to eq "http://#{Platform.localhost}:#{expected_port}"
         end
 

diff --git a/rb/spec/unit/selenium/webdriver/safari/service_spec.rb b/rb/spec/unit/selenium/webdriver/safari/service_spec.rb
@@ -22,7 +22,7 @@
 module Selenium
   module WebDriver
     describe Service do
-      let(:service_path) { "/path/to/#{Safari::Service.executable}" }
+      let(:service_path) { "/path/to/#{Safari::Service::EXECUTABLE}" }
 
       before do
         allow(Platform).to receive(:assert_executable).and_return(true)
@@ -34,8 +34,8 @@ module WebDriver
 
           service = Service.safari
 
-          expect(service.executable_path).to include Safari::Service.executable
-          expected_port = Safari::Service.default_port
+          expect(service.executable_path).to include Safari::Service::EXECUTABLE
+          expected_port = Safari::Service::DEFAULT_PORT
           expect(service.uri.to_s).to eq "http://#{Platform.localhost}:#{expected_port}"
         end