Closed Bug 774293 Opened 7 years ago Closed 7 years ago

pyflakes analysis of mobile automation code identifies unused variables and minor nits

Categories

(Testing :: General, defect)

ARM
Android
defect
Not set

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)

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'
Whiteboard: [good-first-bug][lang=python][mentor=wlach] → [good-first-bug][lang=python][mentor=wlach][mobile]
The title freaked me out with the word 'problems', maybe 'cleanup nits'
(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
I'll take a shot at this bug
Assignee: nobody → pattenabshire
(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.
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
Attachment #650472 - Flags: review?(wlachance)
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.
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-
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.
Attached patch Updated patchSplinter Review
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)
(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
Comment on attachment 651303 [details] [diff] [review]
Updated patch

This looks great and try server is happy.
Attachment #651303 - Flags: review?(wlachance) → review+
Reassigning
Assignee: pattenabshire → nobody
(reassign to self to take responsibility for any unlikely breakage)

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/22d3f22dd62e
Assignee: nobody → wlachance
https://hg.mozilla.org/mozilla-central/rev/22d3f22dd62e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.