974465 - Remove asterisk imports in Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Importing all symbols from a module is a bad idea and discouraged.  Instead import the required symbols or just the module.

Comment 1

[David Burns :automatedtester]

we need to update http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#17 to remove the * import

Comment 2

[Subhendu Ghosh]

Hi, I wish to fix this bug. Please guide me through the process.

Comment 3

[David Burns :automatedtester]

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!

Comment 4

[william.h.angell]

Attachment: Bug 974465 - removed import * and replaced with explicit symbol import

Comment 5

[David Burns :automatedtester]

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

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
    )

Comment 7

[william.h.angell]

Attachment: Patch for Bug 974465 to remove asterisk imports

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.

Comment 8

[David Burns :automatedtester]

try pushes 

https://tbpl.mozilla.org/?tree=Try&rev=dfe64ba80354
https://tbpl.mozilla.org/?tree=Try&rev=4915fb44f7a2

Comment 9

[william.h.angell]

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 10

[David Burns :automatedtester]

Comment on attachment 8413839 [details] [diff] [review]
Patch for Bug 974465 to remove asterisk imports

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

lgtm

Comment 11

[David Burns :automatedtester]

trees closed will try push in a bit

Comment 12

[David Burns :automatedtester]

landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3f2e53c30e

William, thanks for doing this! 

If you want to try do another bug have a look at https://bugzilla.mozilla.org/buglist.cgi?resolution=---&status_whiteboard_type=allwordssubstr&query_format=advanced&status_whiteboard=[good%20first%20bug]&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&component=Marionette&product=Testing&list_id=10003041

Comment 13

[Andreas Tolfsen ❲:ato❳]

William, thanks and congrats on getting the patch landed!  It will appear on m-c soon and we can resolve this bug as fixed.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0d3f2e53c30e