Closed
Bug 974465
Opened 10 years ago
Closed 10 years ago
Remove asterisk imports in Marionette
Categories
(Remote Protocol :: Marionette, defect)
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)
9.38 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
Importing all symbols from a module is a bad idea and discouraged. Instead import the required symbols or just the module.
Reporter | ||
Updated•10 years ago
|
Summary: Remove asterisk import in testing/marionette/client/marionette/marionette_test.py → Remove asterisk imports in Marionette
Comment 1•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
Hi, I wish to fix this bug. Please guide me through the process.
Comment 3•10 years ago
|
||
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!
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8413394 -
Flags: review?(dburns)
Comment 5•10 years ago
|
||
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
Reporter | ||
Comment 6•10 years ago
|
||
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 )
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
try pushes https://tbpl.mozilla.org/?tree=Try&rev=dfe64ba80354 https://tbpl.mozilla.org/?tree=Try&rev=4915fb44f7a2
Flags: needinfo?(dburns)
Updated•10 years ago
|
Assignee: nobody → william.h.angell
Flags: needinfo?(dburns)
Updated•10 years ago
|
Flags: needinfo?(dburns)
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
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
Reporter | ||
Comment 13•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d3f2e53c30e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•