New FirefoxDriver ctor precedence logic and moz:firefoxOptions su… #2882

andreastt · 2016-10-06T15:26:17Z

This change tries to make sense of the keyword arguments accepted by
FirefoxDriver's constructor in the transition phase between the Firefox
add-on driver and the Mozilla supported geckodriver HTTP service.

Prior to this patch, which arguments were consumed and respected varied
greatly on whether the capabilities["marionette"] key was set to true.
Following this change, the options passed on to geckodriver will
consistently come out of firefox.Options.

The patch implements precedence logic for capabilities,
firefox_options, and firefox_binary/firefox_profile. They will
populate the underlying data model in that order, which means a binary-
or profile location set in firefox_options will override one set in the
capabilities dictionary, and that firefox_binary and firefox_profile
will override both of these.

The goal is to make it predictable what key is chosen, and to make the
underlying data model somewhat easier to understand. The latter is
particularly important because the extension connection used for the old
Firefox add-on does not accept a binary, profile, or arguments through
capabilities since it is the Python bindings themselves that manage the
lifetime of the browser.

In the case of geckodriver, which is an external HTTP service, it
manages the lifetime and accepts a moz:firefoxOptions dictionary that
is not unlike chromeOptions as of version 0.11.0. Versions prior to
geckodriver 0.11.0 accepted firefox_binary, firefox_profile, and
firefox_args as top-level entries on the capabilities JSON Object.
This is no longer supported behaviour by either the Python bindings
or geckodriver.

Fixes mozilla/geckodriver#228.

andreastt · 2016-10-06T15:26:36Z

r? @davehunt @AutomatedTester

andreastt · 2016-10-06T15:27:01Z

I found the tests can be run like this:

 % tox -e py27-marionette py/test/selenium/webdriver/marionette/mn_options_tests.py -- -r=aPp -n0

davehunt

Looks good to me, with a few comments. I'd like to get @AutomatedTester to look over it too, as the maintainer of the python bindings.

davehunt · 2016-10-06T15:45:47Z

py/selenium/webdriver/firefox/webdriver.py

+        this would rank below `firefox_options.profile`.
+
+        :param firefox_profile: Instance of ``FirefoxProfile`` object.
+        :param firefox_binary: Full path to the Firefox binary to use.


The current implementation supports either a string or a FirefoxBinary instance.

davehunt · 2016-10-06T15:50:04Z

py/selenium/webdriver/firefox/webdriver.py

+        # firefox_options overrides capabilities
+        if firefox_options is not None:
+            self.binary = firefox_options.binary or self.binary
+            self.profile = firefox_options.profile or self.profile


self.profile will be None

Addressed this, in a few other bugs that were if firefox_options was not set.

davehunt · 2016-10-06T15:52:47Z

py/selenium/webdriver/firefox/webdriver.py

+        # disable native events if globally disabled
+        if self.profile is not None:
+            self.profile.native_events_enabled = (
+                self.NATIVE_EVENTS_ALLOWED and self.profile.native_events_enabled)


Previously if no profile was specified then we created a default one, which meant we were able to set the native events for the profile. By leaving GeckoDriver to create a default profile, we won't have the native events preferences set. I'm not overly familiar with these preferences, so I'm not sure if it matters.

I think you’re right this might matter when you’re not using geckodriver. It is an intentional change not to pass a profile to geckodriver as it is able to create one itself.

davehunt · 2016-10-06T15:53:25Z

py/selenium/webdriver/firefox/webdriver.py

+                self.NATIVE_EVENTS_ALLOWED and self.profile.native_events_enabled)
+
+        # W3C remote
+        # TODO(ato): Perform conformance negotiation


Does this mean as an alternative to checking the marionette capability? If so, +1.

I think one possible approach is to look at what version the system Firefox is, and if it is less than or equal to 47.0.1 default to the old add-on approach, otherwise implicitly go for geckodriver.

I’ll file this under “possible future improvements” (-:

davehunt · 2016-10-06T15:56:38Z

py/test/selenium/webdriver/marionette/mn_options_tests.py

-    driver = Firefox(
-        capabilities=capabilities,
-        firefox_options=options)
+def driver(options = None):


What is the options = None doing? I'm not sure it's needed.

davehunt · 2016-10-06T15:59:50Z

py/test/selenium/webdriver/marionette/mn_options_tests.py

+        opts.profile = other_profile
+        assert other_profile == opts.profile
+
+        with TemporaryDirectory() as path:


You should be able to use the built-in pytest fixture tmpdir instead: http://docs.pytest.org/en/latest/tmpdir.html

Using tmpdir_factory now.

shs96c · 2016-10-06T16:04:16Z

FYI, the java bindings throw an exception if the user has set profiles in more than one place that aren't the same profile. Silently overwriting a user choice seemed less than idea.

andreastt · 2016-10-06T16:05:25Z

It is however the only backwards compatible solution since the Python bindings have supported that behaviour for years.

AutomatedTester · 2016-10-06T20:08:41Z

py/selenium/webdriver/firefox/options.py

-        Returns the location of the binary otherwise an empty string
-        """
-        return self._binary_location
+    def binary(self):


Can we have the original method names back with deprecation warnings in addition to this change. These are in Selenium 2 and Selenium 3 is supposed to be purely removing RC, as requested by @shs96c

AutomatedTester · 2016-10-06T20:39:55Z

py/selenium/webdriver/firefox/webdriver.py

            executor = FirefoxRemoteConnection(
                remote_server_addr=self.service.service_url)
            RemoteWebDriver.__init__(
                self,
                command_executor=executor,
                desired_capabilities=capabilities,
                keep_alive=True)
+
+            self._is_remote = True


This needs to be false.

self._is_remote is used to know when we need to ship a file over to a selenium server on a different machine.

AutomatedTester · 2016-10-06T20:43:45Z

py/test/selenium/webdriver/marionette/mn_options_tests.py

@@ -15,25 +15,97 @@
 # specific language governing permissions and limitations
 # under the License.

-
+import errno


This import doesnt appear to be used.

…pport This change tries to make sense of the keyword arguments accepted by FirefoxDriver's constructor in the transition phase between the Firefox add-on driver and the Mozilla supported geckodriver HTTP service. Prior to this patch, which arguments were consumed and respected varied greatly on whether the `capabilities["marionette"]` key was set to true. Following this change, the options passed on to geckodriver will consistently come out of firefox.Options. The patch implements precedence logic for `capabilities`, `firefox_options`, and `firefox_binary`/`firefox_profile`. They will populate the underlying data model in that order, which means a binary- or profile location set in `firefox_options` will override one set in the `capabilities` dictionary, and that `firefox_binary` and `firefox_profile` will override both of these. The goal is to make it predictable what key is chosen, and to make the underlying data model somewhat easier to understand. The latter is particularly important because the extension connection used for the old Firefox add-on does not accept a binary, profile, or arguments through capabilities since it is the Python bindings themselves that manage the lifetime of the browser. In the case of geckodriver, which is an external HTTP service, it manages the lifetime and accepts a `moz:firefoxOptions` dictionary that is not unlike `chromeOptions` as of version 0.11.0. Versions prior to geckodriver 0.11.0 accepted `firefox_binary`, `firefox_profile`, and `firefox_args` as top-level entries on the capabilities JSON Object. This is no longer supported behaviour by either the Python bindings or geckodriver.

andreastt added R-awaiting review C-py D-marionette G-w3c Requires change to W3C Spec labels Oct 6, 2016

davehunt requested changes Oct 6, 2016

View reviewed changes

AutomatedTester reviewed Oct 6, 2016

View reviewed changes

AutomatedTester merged commit 9706bb7 into SeleniumHQ:master Oct 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New FirefoxDriver ctor precedence logic and moz:firefoxOptions su… #2882

New FirefoxDriver ctor precedence logic and moz:firefoxOptions su… #2882

andreastt commented Oct 6, 2016 •

edited

Loading

andreastt commented Oct 6, 2016

andreastt commented Oct 6, 2016

davehunt left a comment

davehunt Oct 6, 2016

andreastt Oct 6, 2016

davehunt Oct 6, 2016

andreastt Oct 6, 2016

davehunt Oct 6, 2016

andreastt Oct 6, 2016

davehunt Oct 6, 2016

andreastt Oct 6, 2016

davehunt Oct 6, 2016

andreastt Oct 6, 2016

davehunt Oct 6, 2016

andreastt Oct 6, 2016

shs96c commented Oct 6, 2016

andreastt commented Oct 6, 2016

AutomatedTester Oct 6, 2016

andreastt Oct 10, 2016

AutomatedTester Oct 6, 2016

andreastt Oct 10, 2016

AutomatedTester Oct 6, 2016

andreastt Oct 10, 2016

New FirefoxDriver ctor precedence logic and moz:firefoxOptions su… #2882

New FirefoxDriver ctor precedence logic and moz:firefoxOptions su… #2882

Conversation

andreastt commented Oct 6, 2016 • edited Loading

andreastt commented Oct 6, 2016

andreastt commented Oct 6, 2016

davehunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c commented Oct 6, 2016

andreastt commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreastt commented Oct 6, 2016 •

edited

Loading