bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary

RESOLVED FIXED in Firefox 38

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: whimboo, Assigned: parkouss, Mentored)

Tracking

({ateam-marionette-runner})

Trunk
mozilla39
ateam-marionette-runner
Points:
---

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

(Whiteboard: [good first bug][lang=py], URL)

Attachments

(1 attachment)

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)
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#484 would need updating
Mentor: dburns
Keywords: ateam-marionette-runner
Whiteboard: [good first bug][lang=py]
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8574846 [details] [diff] [review]
BaseMarionetteTestRunner.__init__ should initialize self.prefs as dictionary
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8574846 - Flags: review?(dburns)
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
Attachment #8574846 - Flags: review?(dburns) → review-
(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
(Assignee)

Comment 6

3 years ago
I agree with Henrik here, this is a dangerous thing to do.
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.
Attachment #8574846 - Flags: review- → review?(dburns)
Attachment #8574846 - Flags: review?(dburns) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3fb409fdb1ea
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
status-firefox38: --- → affected
Keywords: checkin-needed
Sorry, that was the wrong bug. We don't really need this backported.
status-firefox38: affected → ---
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.