1141129 - BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

By default __init__ uses None as default for prefs. This is fine in the function header, but when setting self.prefs it should be set to the passed in dictionary or an empty one, but not None. Right now subclasses would have to do an extra check to add new preferences to the dictionary like:

        prefs = self.prefs or {}
        prefs.update(my_prefs)
        self.prefs = prefs

With the above fixed this could become a single line:

        self.prefs.update(DEFAULT_PREFS)

Comment 1

[David Burns :automatedtester]

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#484 would need updating

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The line to be updated is actually 538. See the URL as listed in the URL field. You should not default to a dict in the parameter list.

Comment 3

[Julien Pagès (:parkouss)]

Attachment: BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary

Comment 4

[David Burns :automatedtester]

Comment on attachment 8574846 [details] [diff] [review]
BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary

Review of attachment 8574846 [details] [diff] [review]:
-----------------------------------------------------------------

I think this would be better if the prefs kwarg was equal to {} instead None. That way people can do what they want and it also guides them on what type we are expecting

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to David Burns :automatedtester from comment #4)
> I think this would be better if the prefs kwarg was equal to {} instead
> None. That way people can do what they want and it also guides them on what
> type we are expecting

This is not something you should do given that a dictionary is mutable! This will cause failures of subsequent instantiation of the BaseMarionetteTestRunner class. For details see:

http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument

Comment 6

[Julien Pagès (:parkouss)]

I agree with Henrik here, this is a dangerous thing to do.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8574846 [details] [diff] [review]
BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary

Re-requesting review for the same patch. Please see comment 5 for details why it should not be done via the parameter defaults.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb409fdb1ea

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fb409fdb1ea

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is a small change in how preferences are handled. It would be good to get this also backported to Firefox 38, so we don't have to make an exception for our Firefox UI tests all the 38 ESR livetime.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, that was the wrong bug. We don't really need this backported.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5e4a92755fb