704321 - cleanup PerfConfigurator handling of the command line

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

There are several minor stnanks with PerfConfigurator handling of the
command line.  The program name should be gotten from the parser
instead of parsed itself (which is done wrongly anyway, should use
os.path.basename). argv is passed into main as None and is not further
used; should come from sys.argv and actually be used.  The Usage
exception is not used and should go away.  And PerfConfigurator should
inherit from object:
http://docs.python.org/reference/datamodel.html#new-style-and-classic-classes

Comment 1

[Jeff Hammel]

Attachment: cleanup PerfConfigurator

This patch introduces the cleanup from bug 698507 and removes some trailing whitespace

Comment 2

[William Lachance (:wlach)]

Comment on attachment 575995 [details] [diff] [review]
cleanup PerfConfigurator

Looks like nice straightforward cleanup.

Comment 3

[Jeff Hammel]

This can be landed whenever.  I don't know when I'll be able to test as I've been having trouble with the staging environment, but if someone else wants to land first, that would be nice.

Comment 4

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/5ae66b11330f

tested locally and does not seem to have any platform dependent elements

Comment 5

[Jeff Hammel]

reopening; the remotePerfConfigurtator signature of verifyCommandLine does not return anything: 
http://hg.mozilla.org/build/talos/annotate/5ae66b11330f/talos/remotePerfConfigurator.py#l187
however, the base class instance does:
http://hg.mozilla.org/build/talos/annotate/5ae66b11330f/talos/PerfConfigurator.py#l485
since parse_args relies on a consistent function return value, this breaks with parse_args which will then return None for options since parse_args uses the return value of verifyCommandLine which may clean up options as it does for PerfConfigurator

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I know there is no simple way to test everything, but we need to take ownership of this and not have so many regressions.  Running all the tests in staging solve the desktop problem, all we need to test is the remote stuff.  For the remote stuff you can easily test this locally with a python sut agent or using a real device.  

If you are changing the uploading to a graph server logic, obviously you don't have the test the command line handler, but if you do change something related to the commandline handler, please grep through the code for all related instances and verify that those instances will not be affected by the change.

Comment 7

[Jeff Hammel]

Attachment: fix for remotePerfConfigurator.py

Comment 8

[Jeff Hammel]

Comment on attachment 578287 [details] [diff] [review]
fix for remotePerfConfigurator.py

I've been able to run remotePerfConfigurator.py with this patch and the resulting file with run_tests.py does launch tests on my tablet. I'm still familiarizing myself with the remoteTesting setup, but I think this part of the regression is fixed.  I have also run through the ateam staging environment with all green results, though the linux test slave is down so I was unable to test there.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 578287 [details] [diff] [review]
fix for remotePerfConfigurator.py

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

::: talos/remotePerfConfigurator.py
@@ +195,2 @@
>      try:
> +        options, args = parser.parse_args(argv)

does this call verifyCommandLine()?

Comment 10

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 578287 [details] [diff] [review] [diff] [details] [review]
> fix for remotePerfConfigurator.py
> 
> Review of attachment 578287 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: talos/remotePerfConfigurator.py
> @@ +195,2 @@
> >      try:
> > +        options, args = parser.parse_args(argv)
> 
> does this call verifyCommandLine()?

Yes: http://hg.mozilla.org/build/talos/file/5ae66b11330f/talos/PerfConfigurator.py#l464

In fact, this is why it was failing before

Comment 11

[Jeff Hammel]

follow up pushed: http://hg.mozilla.org/build/talos/rev/b7176a4b2717