822875 - Use 4-space indent in TPS and fix pyflakes warnings

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Jonathan Griffin (:jgriffin)]

TPS's Python files should use 4-space indentation instead of 2, and we should fix all the many pyflakes warnings that exist in them.

Comment 1

[Atul Jangra [:atuljangra]]

I would want to know what is needed to be done exactly in this bug.
Can you point me to some code. :-)

Comment 2

[Rohit]

(In reply to Jonathan Griffin (:jgriffin) from comment #0)
> TPS's Python files should use 4-space indentation instead of 2, and we
> should fix all the many pyflakes warnings that exist in them.

I would like to work on this bug. Where do i start?

Comment 3

[Jonathan Griffin (:jgriffin)]

You can download pyflakes (http://pypi.python.org/pypi/pyflakes) and run it on TPS's Python code.  That will flag a bunch of common errors, which can then be fixed.

Additionally, most of the code uses old-style 2-space indents; we should update this to modern 4-space indents while we're at it.

TPS lives at http://mxr.mozilla.org/mozilla-central/source/testing/tps/tps/

Comment 4

[Richard Newman [:rnewman]]

(Forgot to hit "Save Changes".)

You'll want to get a checkout of services-central.

https://developer.mozilla.org/en-US/docs/Developer_Guide
https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial?redirectlocale=en-US&redirectslug=Mozilla_Source_Code_%28Mercurial%29#Bundles

(a bundle is at http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/services-central.hg )


Get it building, and to the point where you can run TPS:

http://hg.mozilla.org/services/services-central/file/default/testing/tps/README


Once you have TPS running, and all tests passing, you can run pyflakes on the files in:

http://hg.mozilla.org/services/services-central/file/default/testing/tps/tps

and fix the reported problems. Make sure TPS still runs and passes afterwards.

Comment 5

[vignesh.sarma]

This is still assigned to nobody, so can I do it? 
I am familiar with python and have been looking to begin somewhere.

The instruction from :rnewman seem fairly clear too...

Comment 6

[vignesh.sarma]

Attachment: Diff of updated files for the simple pyflake errors I encounterd.

there are still some pyflakes error like:

tps/testrunner.py:115: redefinition of function 'mobile' from line 111         
tps/__init__.py:5: 'TPSFirefoxRunner' imported but unused
tps/__init__.py:6: 'TPSTestRunner' imported but unused
tps/__init__.py:7: 'MozHttpd' imported but unused    
tps/phase.py:47: local variable 'returncode' is assigned to but never used

I would like to get some idea about how best to solve them,

Comment 7

[Jonathan Griffin (:jgriffin)]

(In reply to vignesh.sarma from comment #6)
> Created attachment 699114 [details] [diff] [review]
> Diff of updated files for the simple pyflake errors I encounterd.
> 
> there are still some pyflakes error like:
> 
> tps/testrunner.py:115: redefinition of function 'mobile' from line 111      
> 
> tps/__init__.py:5: 'TPSFirefoxRunner' imported but unused
> tps/__init__.py:6: 'TPSTestRunner' imported but unused
> tps/__init__.py:7: 'MozHttpd' imported but unused    
> tps/phase.py:47: local variable 'returncode' is assigned to but never used
> 
> I would like to get some idea about how best to solve them,

Hi Vignesh,

Thanks for the patch!  For the unused imports, you can just remove them.  You can also remove 'returncode' variable that's never used.

The "redefinition of function 'mobile'" error isn't a real problem; it's one way that Python properties can be defined.  It seems to be a pyflakes bug, see http://stackoverflow.com/questions/12563469/fix-pyflakes-dealing-with-property-setter-decorator.  So no action is needed for that error.

Comment 8

[vignesh.sarma]

Attachment: solved all pyflakes errors, expect the one with the bug in pyflakes.

Hi Jonathan,

The problem with 
"imported but unused" is that they are in __init__ file, so they might be use in some other place with out specifying the actual file.

One place I found was this,

testing/tps/tps/cli.py
12:from tps import TPSTestRunner

I think this is the only one, so I just update that and removed those imports.

If this is ok i can re-indent the whole module in 4-space and submit the final version.

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 699540 [details] [diff] [review]
solved all pyflakes errors, expect the one with the bug in pyflakes.

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

Thanks for the patch!  Unfortunately, I gave you some bad advice; I didn't realize that some of the 'unused import' errors were in __init__.py.  Those need to be restored.  Pyflakes does some rather basic code analysis; one of the things it does is check if any imports are unused in a given file.  Imports in __init__.py are not there for use in that file, but for use by other scripts that use that module.

Please restore __init__.py and then we can land this!

Comment 10

[vignesh.sarma]

Attachment: updated all valid pyflakes errors

updated the patch,
if this is ok, I would like to run the module through reindent before submitting the final patch.
Thanks.

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 700178 [details] [diff] [review]
updated all valid pyflakes errors

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

Thank you!  In a separate patch, if you feel so inclined, you could reformat all these files to use 4-space indents instead of the current 2-space.

Comment 12

[Jonathan Griffin (:jgriffin)]

Ah, I just saw your comment #10, yes, re-indenting in the same patch is fine too.

Comment 13

[vignesh.sarma]

Attachment: reindent of the code to 4 space,

This doesn't contain the previous updates, this would have to be applied on the previous patch.

It doesn't change any thing other than re-indent the code.
Thanks.

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 700813 [details] [diff] [review]
reindent of the code to 4 space,

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

Thanks for cleaning up the extra whitespace as well.

Comment 15

[Jonathan Griffin (:jgriffin)]

Tests pass with the changes.

http://hg.mozilla.org/services/services-central/rev/4ffaf917d5c0
http://hg.mozilla.org/services/services-central/rev/1e80bc5cbeba

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/1e80bc5cbeba

FYI, next time you land in services-central, please add "[fixed in services]" to the whiteboard and don't resolve the bug until it is merged into m-c.