Closed
Bug 774293
Opened 12 years ago
Closed 12 years ago
pyflakes analysis of mobile automation code identifies unused variables and minor nits
Categories
(Testing :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: wlach, Assigned: wlach)
Details
(Whiteboard: [good-first-bug][lang=python][mentor=wlach][mobile])
Attachments
(1 file, 1 obsolete file)
16.94 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
Running pyflakes against the source in build/mobile, there seems to be quite a few minor issues. Most of these are probably harmless but some may reveal real bugs in the code. It would be great to get this cleaned up. wlach@eideticker:~/src/mozilla-central$ pyflakes build/mobile/*py build/mobile/b2gautomation.py:7: 'os' imported but unused build/mobile/b2gautomation.py:10: 'socket' imported but unused build/mobile/b2gautomation.py:12: 'sys' imported but unused build/mobile/b2gautomation.py:17: 'DeviceManager' imported but unused build/mobile/b2gautomation.py:116: local variable 'didTimeout' is assigned to but never used build/mobile/devicemanagerADB.py:9: 'sys' imported but unused build/mobile/devicemanagerADB.py:68: local variable 'e' is assigned to but never used build/mobile/devicemanagerADB.py:414: local variable 'p' is assigned to but never used build/mobile/devicemanagerADB.py:520: local variable 'p' is assigned to but never used build/mobile/devicemanagerADB.py:524: local variable 'p' is assigned to but never used build/mobile/devicemanagerADB.py:651: undefined name 'time' build/mobile/devicemanager.py:5: 'time' imported but unused build/mobile/devicemanagerSUT.py:8: 'datetime' imported but unused build/mobile/devicemanagerSUT.py:11: 'hashlib' imported but unused build/mobile/devicemanagerSUT.py:14: 'traceback' imported but unused build/mobile/devicemanagerSUT.py:15: 'sys' imported but unused build/mobile/devicemanagerSUT.py:17: 'DMError' imported but unused build/mobile/devicemanagerSUT.py:129: local variable 'done' is assigned to but never used build/mobile/devicemanagerSUT.py:215: local variable 'ready' is assigned to but never used build/mobile/devicemanagerSUT.py:519: local variable 'data' is assigned to but never used build/mobile/devicemanagerSUT.py:568: local variable 'data' is assigned to but never used build/mobile/devicemanagerSUT.py:661: local variable 'data' is assigned to but never used build/mobile/devicemanagerSUT.py:905: local variable 'callbacksvrstatus' is assigned to but never used build/mobile/devicemanagerSUT.py:1103: redefinition of function 'unpackFile' from line 873 build/mobile/remoteautomation.py:6: 'sys' imported but unused build/mobile/remoteautomation.py:8: 'socket' imported but unused build/mobile/remoteautomation.py:14: 'DeviceManager' imported but unused build/mobile/remoteautomation.py:111: undefined name 'subprocess'
Assignee | ||
Updated•12 years ago
|
Whiteboard: [good-first-bug][lang=python][mentor=wlach] → [good-first-bug][lang=python][mentor=wlach][mobile]
Comment 1•12 years ago
|
||
The title freaked me out with the word 'problems', maybe 'cleanup nits'
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #1) > The title freaked me out with the word 'problems', maybe 'cleanup nits' Hmm, unused variables can and do indicate actual problems with the code. That said, I have no hard evidence to prove this. :) The rest of the issues are indeed just nits.
Summary: pyflakes analysis of mobile automation code reveals problems → pyflakes analysis of mobile automation code identifies unused variables and minor nits
Comment 3•12 years ago
|
||
I'll take a shot at this bug
Updated•12 years ago
|
Assignee: nobody → pattenabshire
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to pattenabshire from comment #3) > I'll take a shot at this bug Wonderful! Feel free to post on this bug (or get in touch with us: https://wiki.mozilla.org/Auto-tools/New_Contributor) if you have any questions.
Comment 5•12 years ago
|
||
I've attached a patch to fix these nits. I've removed all unused imports and added missing imports to devicemanagerADB.py and remoteautomation.py. The unused but set variables mostly looked like copy-paste mistakes or left-overs from refactoring. I removed most of them. In two cases I am not entirely sure and would appreciate comments by other developers: In devicemanagerSUT.py I replaced the unused 'data' variable in line 639 by the 'buffer' variable. I think this was a bug in the code, since the buffer variable would otherwise always be an empty string when used in line 644. In devicemanagerADB.py I removed the 'p' (a subprocess.Popen instance) variable in the killProcess method, since it wasn't used anywhere. But maybe it would make sense to use it in order to determine if the the kill command was actually successful instead of just blindly setting 'didKillProcess' to True. I also removed some trailing whitespaces in remoteautomation.py I hope this doesn't bloat the patch too much. Best regards, Dominik
Updated•12 years ago
|
Attachment #650472 -
Flags: review?(wlachance)
Assignee | ||
Comment 6•12 years ago
|
||
Hi Dominik, Thanks so much for your patch! It's just about perfect, except for the two things you mention: (In reply to Dominik Oepen from comment #5) > Created attachment 650472 [details] [diff] [review] > Proposed patch, removes the unused imports and variables > > I've attached a patch to fix these nits. I've removed all unused imports and > added missing imports to devicemanagerADB.py and remoteautomation.py. > > The unused but set variables mostly looked like copy-paste mistakes or > left-overs from refactoring. I removed most of them. In two cases I am not > entirely sure and would appreciate comments by other developers: > > In devicemanagerSUT.py I replaced the unused 'data' variable in line 639 by > the 'buffer' variable. I think this was a bug in the code, since the buffer > variable would otherwise always be an empty string when used in line 644. So I had a look at this and it seems as if 'pull' is considered a no response command by the higher-level _doCmds function, so we don't actually grab any data when returning it. We actually process the response later. Overall I think the way we're processing response data here is problematic, since we're essentially duplicating the socket handling code in two places (just glancing at the code, I see this code still has bugs that were fixed in _doCmds, like not doing a select() to make sure that there is data available for reading before doing so). I think the easiest thing to do for now is to just remove the assignment to any variable (including data) and maybe add a comment like `# just send the command first, we read the response inline below` above that line. > In devicemanagerADB.py I removed the 'p' (a subprocess.Popen instance) > variable in the killProcess method, since it wasn't used anywhere. But maybe > it would make sense to use it in order to determine if the the kill command > was actually successful instead of just blindly setting 'didKillProcess' to > True. Yes, this definitely would make sense. Could you add a check for this in the patch? > I also removed some trailing whitespaces in remoteautomation.py I hope this > doesn't bloat the patch too much. Nope, that's much appreciated too! Thanks.
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 650472 [details] [diff] [review] Proposed patch, removes the unused imports and variables r- for now. I'd like to get the two items discussed above fixed before landing.
Attachment #650472 -
Flags: review?(wlachance) → review-
Comment 8•12 years ago
|
||
Thanks for looking into this. I will fix the two items and re-submit an improved patch. I am currently on the road but will be able to work on this on Sunday or Monday.
Comment 9•12 years ago
|
||
This patch fixes the two issues discussed about the previous patch. killProcess now checks the returncode of the 'kill' command and sets didKillProcess accordingly. The data returned by the runCmds method is now ignored and a clarifying comment was added.
Attachment #650472 -
Attachment is obsolete: true
Attachment #651303 -
Flags: review?(wlachance)
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Dominik Oepen from comment #9) > Created attachment 651303 [details] [diff] [review] > Updated patch > > This patch fixes the two issues discussed about the previous patch. > > killProcess now checks the returncode of the 'kill' command and sets > didKillProcess accordingly. > > The data returned by the runCmds method is now ignored and a clarifying > comment was added. Looks really good, just doing a try server run: https://tbpl.mozilla.org/?tree=Try&rev=c1b2b853c048
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 651303 [details] [diff] [review] Updated patch This looks great and try server is happy.
Attachment #651303 -
Flags: review?(wlachance) → review+
Assignee | ||
Comment 13•12 years ago
|
||
(reassign to self to take responsibility for any unlikely breakage) Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/22d3f22dd62e
Assignee: nobody → wlachance
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/22d3f22dd62e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•