[rb] update chrome extensions & firefox profile behavior to evaluate validity when assigned rather than when used

Author: titusfortner

rb/.rubocop.yml

@@ -62,7 +62,7 @@ Metrics/ClassLength:
     - 'lib/selenium/webdriver/remote/capabilities.rb'
 
 Metrics/CyclomaticComplexity:
-  Max: 10
+  Max: 9
   Exclude:
     - 'lib/selenium/webdriver/remote/capabilities.rb'
     - 'lib/selenium/webdriver/support/color.rb'

rb/lib/selenium/webdriver/chrome/options.rb

@@ -29,7 +29,6 @@ class Options < WebDriver::Options
         # see: http://chromedriver.chromium.org/capabilities
         CAPABILITIES = {args: 'args',
                         binary: 'binary',
-                        extensions: 'extensions',
                         local_state: 'localState',
                         prefs: 'prefs',
                         detach: 'detach',
@@ -40,6 +39,9 @@ class Options < WebDriver::Options
                         perf_logging_prefs: 'perfLoggingPrefs',
                         window_types: 'windowTypes'}.freeze
 
+        # NOTE: special handling of 'extensions' to validate when set instead of when used
+        attr_reader :extensions
+
         # Create a new Options instance.
         #
         # @example
@@ -64,21 +66,23 @@ class Options < WebDriver::Options
         # @option opts [Array<String>] :window_types A list of window types to appear in the list of window handles
         #
 
-        def initialize(profile: nil, encoded_extensions: [], **opts)
+        def initialize(profile: nil, **opts)
           super(**opts)
 
           @profile = profile
-          @options[:encoded_extensions] = encoded_extensions
-
-          @options[:args] ||= []
-          @options[:prefs] ||= {}
-          @options[:emulation] ||= {}
-          @options[:local_state] ||= {}
-          @options[:exclude_switches] ||= []
-          @options[:perf_logging_prefs] ||= {}
-          @options[:window_types] ||= []
-          @options[:extensions] ||= []
-          @options[:extensions].each(&method(:validate_extension))
+
+          @options = {args: [],
+                      prefs: {},
+                      emulation: {},
+                      extensions: [],
+                      local_state: {},
+                      exclude_switches: [],
+                      perf_logging_prefs: {},
+                      window_types: []}.merge(@options)
+
+          @encoded_extensions = @options.delete(:encoded_extensions) || []
+          @extensions = []
+          (@options.delete(:extensions)).each(&method(:validate_extension))
         end
 
         #
@@ -93,7 +97,21 @@ def initialize(profile: nil, encoded_extensions: [], **opts)
 
         def add_extension(path)
           validate_extension(path)
-          @options[:extensions] << path
+        end
+
+        #
+        # Add an extension by local path.
+        #
+        # @example
+        #   extensions = ['/path/to/extension.crx', '/path/to/other.crx']
+        #   options = Selenium::WebDriver::Chrome::Options.new
+        #   options.extensions = extensions
+        #
+        # @param [Array<String>] :extensions A list of paths to (.crx) Chrome extensions to install on startup
+        #
+
+        def extensions=(extensions)
+          extensions.each(&method(:validate_extension))
         end
 
         #
@@ -107,9 +125,8 @@ def add_extension(path)
         #
 
         def add_encoded_extension(encoded)
-          @options[:encoded_extensions] << encoded
+          @encoded_extensions << encoded
         end
-        alias_method :encoded_extension=, :add_encoded_extension
 
         #
         # Add a command-line argument to use when starting Chrome.
@@ -178,15 +195,16 @@ def add_emulation(**opts)
         private
 
         def process_browser_options(browser_options)
-          options = browser_options[KEY]
+          options = browser_options[self.class::KEY]
           options['binary'] ||= binary_path if binary_path
-          options['args'] << "--user-data-dir=#{@profile.directory}" if @profile
-          merge_extensions(options)
-        end
+          if @profile
+            options['args'] ||= []
+            options['args'] << "--user-data-dir=#{@profile.directory}"
+          end
+
+          return if (@encoded_extensions + @extensions).empty?
 
-        def merge_extensions(browser_options)
-          encoded_extensions = browser_options.delete(:encoded_extensions)
-          browser_options[:extensions] = extensions.map(&method(:encode_extension)) + encoded_extensions
+          options['extensions'] = @encoded_extensions + @extensions.map(&method(:encode_extension))
         end
 
         def binary_path
@@ -200,6 +218,8 @@ def encode_extension(path)
         def validate_extension(path)
           raise Error::WebDriverError, "could not find extension at #{path.inspect}" unless File.file?(path)
           raise Error::WebDriverError, "file was not an extension #{path.inspect}" unless File.extname(path) == '.crx'
+
+          @extensions << path
         end
 
         def camelize?(key)

rb/lib/selenium/webdriver/common/options.rb

@@ -82,12 +82,16 @@ def as_json(*)
         end
         browser_options = defined?(self.class::KEY) ? {self.class::KEY => options} : options
 
-        process_browser_options(browser_options) if private_methods(false).include?(:process_browser_options)
+        process_browser_options(browser_options)
         generate_as_json(w3c_options.merge(browser_options))
       end
 
       private
 
+      def process_browser_options(_browser_options)
+        nil
+      end
+
       def camelize?(_key)
         true
       end

rb/lib/selenium/webdriver/firefox/options.rb

@@ -24,13 +24,15 @@ class Options < WebDriver::Options
         KEY = 'moz:firefoxOptions'
 
         # see: https://firefox-source-docs.mozilla.org/testing/geckodriver/Capabilities.html
-        # NOTE: special handling of 'profile' to validate on construction instead of use
         CAPABILITIES = {binary: 'binary',
                         args: 'args',
                         log: 'log',
                         prefs: 'prefs'}.freeze
         BROWSER = 'firefox'
 
+        # NOTE: special handling of 'profile' to validate when set instead of when used
+        attr_reader :profile
+
         #
         # Create a new Options instance, only for W3C-capable versions of Firefox.
         #
@@ -53,7 +55,8 @@ def initialize(log_level: nil, **opts)
           @options[:args] ||= []
           @options[:prefs] ||= {}
           @options[:log] ||= {level: log_level} if log_level
-          process_profile(@options[:profile]) if @options.key?(:profile)
+
+          process_profile(@options.delete(:profile))
         end
 
         #
@@ -116,10 +119,6 @@ def profile=(profile)
           process_profile(profile)
         end
 
-        def profile
-          @options[:profile]
-        end
-
         def log_level
           @options.dig(:log, :level)
         end
@@ -133,17 +132,18 @@ def log_level=(level)
         def process_browser_options(browser_options)
           options = browser_options[KEY]
           options['binary'] ||= Firefox.path if Firefox.path
-          options['profile'] = options.delete(:profile) if profile
+          options['profile'] = @profile if @profile
         end
 
         def process_profile(profile)
-          @options[:profile] = if profile.nil?
-                                 nil
-                               elsif profile.is_a? Profile
-                                 profile
-                               else
-                                 Profile.from_name(profile)
-                               end
+          @profile = case profile
+                     when nil
+                       nil
+                     when Profile
+                       profile
+                     else
+                       Profile.from_name(profile)
+                     end
         end
       end # Options
     end # Firefox

rb/spec/unit/selenium/webdriver/chrome/options_spec.rb

@@ -158,7 +158,7 @@ module Chrome
         describe '#add_encoded_extension' do
           it 'adds an encoded extension' do
             options.add_encoded_extension('foo')
-            expect(options.instance_variable_get('@options')[:encoded_extensions]).to include('foo')
+            expect(options.instance_variable_get('@encoded_extensions')).to include('foo')
           end
         end
 
@@ -264,7 +264,7 @@ module Chrome
                                                'prefs' => {'foo' => 'bar',
                                                            'key_that_should_not_be_camelcased' => 'baz'},
                                                'binary' => '/foo/bar',
-                                               'extensions' => %w[encoded_foo encoded_bar encoded_foobar],
+                                               'extensions' => %w[encoded_foobar encoded_foo encoded_bar],
                                                'foo' => 'bar',
                                                'mobileEmulation' => {'deviceName' => 'mine'},
                                                'localState' => {'foo' => 'bar'},

rb/spec/unit/selenium/webdriver/edge_chrome/driver_spec.rb

@@ -210,8 +210,11 @@ def as_json(*)
 
             it 'with Options instance with profile' do
               profile = Profile.new.tap(&:layout_on_disk)
+              allow(profile).to receive(:directory).and_return("PROF_DIR")
               options = Options.new(profile: profile)
-              expect_request(body: {capabilities: {firstMatch: [browserName: "MicrosoftEdge", 'ms:edgeOptions': {}]}})
+              expect_request(body: {capabilities:
+                                      {firstMatch: [browserName: "MicrosoftEdge",
+                                                    'ms:edgeOptions': {"args": ["--user-data-dir=PROF_DIR"]}]}})
 
               expect { Driver.new(capabilities: [options]) }.not_to raise_exception
             end

rb/spec/unit/selenium/webdriver/edge_chrome/options_spec.rb

@@ -107,7 +107,7 @@ module EdgeChrome
         describe '#add_encoded_extension' do
           it 'adds an encoded extension' do
             options.add_encoded_extension('foo')
-            expect(options.instance_variable_get('@options')[:encoded_extensions]).to include('foo')
+            expect(options.instance_variable_get('@encoded_extensions')).to include('foo')
           end
         end
 
@@ -220,10 +220,7 @@ module EdgeChrome
                                                'prefs' => {'foo' => 'bar',
                                                            'key_that_should_not_be_camelcased' => 'baz'},
                                                'binary' => '/foo/bar',
-                                               # TODO: verify this is correct behavior;
-                                               # I would expect extensions to come back encoded
-                                               'extensions' => %w[foo.crx bar.crx],
-                                               'encodedExtensions' => %w[encoded_foobar],
+                                               'extensions' => %w[encoded_foobar encoded_foo encoded_bar],
                                                'foo' => 'bar',
                                                'mobileEmulation' => {'deviceName' => 'mine'},
                                                'localState' => {'foo' => 'bar'},