Closed Bug 974465 Opened 6 years ago Closed 6 years ago

Remove asterisk imports in Marionette

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: ato, Assigned: william.h.angell)

Details

(Whiteboard: [good first bug][lang=py][mentor=automatedtester])

Attachments

(1 file, 1 obsolete file)

Importing all symbols from a module is a bad idea and discouraged.  Instead import the required symbols or just the module.
Summary: Remove asterisk import in testing/marionette/client/marionette/marionette_test.py → Remove asterisk imports in Marionette
we need to update http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#17 to remove the * import
Whiteboard: [good first bug][lang=py][mentor=automatedtester]
Hi, I wish to fix this bug. Please guide me through the process.
Hi Subhendu,

Follow the steps on https://developer.mozilla.org/en-US/docs/Simple_Firefox_build to get firefox built.

Make the change as mentioned in comment 1. We need to be explicit with our imports and then follow 

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests for running the tests.

Hope that helps!
Thanks for the patch. I have pushed to our try servers which will run all the tests like they do in production. You can see the results from the links below.

Mobile try https://tbpl.mozilla.org/?tree=Try&rev=3f27942e502b
Desktop Try https://tbpl.mozilla.org/?tree=Try&rev=f2c3d8f7f1e7
Please note that there are several other files in the Marionette client code that uses asterisk imports.  A quick grep gives me the following:

% grep -r -i --include \*.py "import \*" .
./__init__.py:from gestures import *
./__init__.py:from errors import *
./__init__.py:from runner import *
./marionette.py:from errors import *
./marionette_test.py:from errors import *
./runner/__init__.py:from base import *
./runner/__init__.py:from mixins import *
./runner/mixins/__init__.py:from endurance import *
./runner/mixins/__init__.py:from reporting import *
./runner/mixins/__init__.py:from b2g import *
./tests/unit/test_single_finger.py:from single_finger_functions import *
./tests/unit/test_single_finger_desktop.py:from single_finger_functions import *

Also the parenthesis construct lets you avoid redefining the "from foo import …" statement when you have really long imports that exceed the maximum allowed line length:

    from foo import (
        foo
        bar
    )
I've replaced the "import *"s in all of the files mentioned above, as well as changing the long import statements to the suggested form.
Attachment #8413394 - Attachment is obsolete: true
Attachment #8413394 - Flags: review?(dburns)
Attachment #8413839 - Flags: review?(dburns)
Assignee: nobody → william.h.angell
Flags: needinfo?(dburns)
Flags: needinfo?(dburns)
It looks like a random failure.  I consulted with #ateam in IRC and someone scheduled another try:
https://tbpl.mozilla.org/?tree=Try&rev=dfe64ba80354

I think it's okay now (if I'm reading this right, new Trys are appended to the first Try).

Please let me know.
Comment on attachment 8413839 [details] [diff] [review]
Patch for Bug 974465 to remove asterisk imports

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

lgtm
Attachment #8413839 - Flags: review?(dburns) → review+
trees closed will try push in a bit
Flags: needinfo?(dburns)
William, thanks and congrats on getting the patch landed!  It will appear on m-c soon and we can resolve this bug as fixed.
https://hg.mozilla.org/mozilla-central/rev/0d3f2e53c30e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.