1063886 - Autophone - paydown technical debt

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: technical-debt.patch

Pay down Autophone's technical debt:
    * Make pyflake clean.
    * switch to jot.
    * Remove armv6 since it is no longer supported
    * fix runjob vs. run_job inconsistency in tests.
    * move test config file reading to PhoneTest constructor.
    * add missing import ConfigParser
    * remove PhoneTestMessage.JSONEncoder
    * Use AutophoneOptions object to encapsulate command and ini configuration data instead of user_cfg dictionary.
    * Use PhoneData object instead of phone_cfg dict to encapsulate phone data.
    * Use BuildMetadata object instead of build_metadata dict to encapsulate build data.
    ** Change BuildMetadata's attributes to be less verbose: build_dir to dir, build_type to type, androidprocname to app_name, blddate to date.
    * PhoneTestMessage/update_status
    ** Move phonetest.PhoneTestMessage to phonestatus, eliminate status_cb from PhoneTest constuctor
    ** Move phonetest.PhoneTestMessage.* status values to phonestatus.PhoneStatus
    ** change PhoneTestMessage signature to use phone_status, message arguments
    ** change worker status_update and phonetest set_status to update_status.
    ** Pass PhoneData to PhoneTestMessage, optionally pass BuildMetadata.
    * PhoneTest
    ** Use PhoneWorker.setup_job/teardown_job to set the worker_subprocess as an attribute on PhoneTest
    ** Add PhoneTest.build property to return PhoneTest.worker_subprocess.build
    * Remove build_metadata and worker_subprocess arguments where available as attributes.
    * Make sure to use phone_status kwarg on PhoneWorker.update_status
    * Remove redundant build.id from s1s2tests update_status messages.
    * Fix undefined use of proc in PhoneWorkerSubProcess._check_device.

tests/smoketest.py references the phone's ip address which we don't have available until bug 1062371 lands where we will pick up adb.py:get_ip_address. I'll fix when I do the patch for that bug.

runtestsremote.py would need more work to deal with changes to the unittests but I'm deferring that for now.

Comment 1

[Mark Côté [:mcote]]

Comment on attachment 8485361 [details] [diff] [review]
technical-debt.patch

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

I went pretty quickly over this; only found one problem and a few nits.  All easy to fix on commit.

::: autophone.py
@@ +25,2 @@
>  from mailer import Mailer
> +from manifestparser import TestManifest

Technically, PEP 8 specifies that external third-party imports should go between standard library imports and local imports.  There are a few files here that violate that.

@@ +208,4 @@
>                      if 'armv6' in build_url:
>                          incompatible_job = True
>                  else:
> +                    if 'x86' in build_url in build_url:

This doesn't look right...

::: tests/perftest.py
@@ +45,1 @@
>                                             key_id=self._jwt['id'])

Indentation.

::: tests/s1s2test.py
@@ +18,5 @@
>  
>  class S1S2Test(PerfTest):
>  
> +    def setup_job(self, worker_subprocess):
> +        PerfTest.setup_job(self, worker_subprocess)

I know this was in the original, but you don't need to override this if it's only going to be calling the superclass's function of the same name with the same params.

::: worker.py
@@ +132,5 @@
>          """These are status messages routed back from the autophone_queue
>          listener in the main AutoPhone class. There is probably a bit
>          clearer way to do this..."""
> +        if not self.last_status_msg or \
> +           msg.phone_status != self.last_status_msg.phone_status:

Normal way of handling long expressions is to wrap in parentheses instead of \ continuation.

@@ +181,4 @@
>                              log_level=self.loglevel,
>                              logger_name='autophone.worker.adb',
> +                            device_ready_retry_wait=self.options.device_ready_retry_wait,
> +                            device_ready_retry_attempts=self.options.device_ready_retry_attempts)

Is there weird indenting here?

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: review.patch

(In reply to Mark Côté [:mcote] from comment #1)

> 
> ::: autophone.py
> @@ +25,2 @@
> >  from mailer import Mailer
> > +from manifestparser import TestManifest
> 
> Technically, PEP 8 specifies that external third-party imports should go
> between standard library imports and local imports.  There are a few files
> here that violate that.

I always get this wrong. I'm attaching the fixes as an additional patch. Let me know if it follows PEP 8 ok. I'm putting

standard imports
from standard imports

third party imports
from third pary imports

local imports
from local imports

Caught a bug in runtestsremote.py where I had a mozdevice.DMError left over as well.

> 
> @@ +208,4 @@
> >                      if 'armv6' in build_url:
> >                          incompatible_job = True
> >                  else:
> > +                    if 'x86' in build_url in build_url:
> 
> This doesn't look right...
> 

doh. Thanks.

> ::: tests/perftest.py
> @@ +45,1 @@
> >                                             key_id=self._jwt['id'])
> 

got it.

> Indentation.
> 
> ::: tests/s1s2test.py
> @@ +18,5 @@
> >  
> >  class S1S2Test(PerfTest):
> >  
> > +    def setup_job(self, worker_subprocess):
> > +        PerfTest.setup_job(self, worker_subprocess)
> 
> I know this was in the original, but you don't need to override this if it's
> only going to be calling the superclass's function of the same name with the
> same params.
> 

But I'm adding to the method, so I need to call the parent class's version, don't I?

> ::: worker.py
> @@ +132,5 @@
> >          """These are status messages routed back from the autophone_queue
> >          listener in the main AutoPhone class. There is probably a bit
> >          clearer way to do this..."""
> > +        if not self.last_status_msg or \
> > +           msg.phone_status != self.last_status_msg.phone_status:
> 
> Normal way of handling long expressions is to wrap in parentheses instead of
> \ continuation.
> 

Thanks. I couldn't remember which you prefered.

> @@ +181,4 @@
> >                              log_level=self.loglevel,
> >                              logger_name='autophone.worker.adb',
> > +                            device_ready_retry_wait=self.options.device_ready_retry_wait,
> > +                            device_ready_retry_attempts=self.options.device_ready_retry_attempts)
> 
> Is there weird indenting here?

I don't think so.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 8485908 [details] [diff] [review]
review.patch

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

Yup imports are much better.

::: autophone.py
@@ +17,4 @@
>  import socket
>  import sys
>  import threading
> +from multiprocessinghandlers import MultiprocessingStreamHandler, MultiprocessingTimedRotatingFileHandler

For long lines like this, the standard is like

from multiprocessinghandlers import (MultiprocessingStreamHandler,
                                     MultiprocessingTimedRotatingFileHandler)

... although that's probably still too long. But at least it's not as long as before. :)

::: worker.py
@@ +131,5 @@
>          """These are status messages routed back from the autophone_queue
>          listener in the main AutoPhone class. There is probably a bit
>          clearer way to do this..."""
> +        if (not self.last_status_msg or
> +           msg.phone_status != self.last_status_msg.phone_status):

Indenting. :)

Comment 4

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #2)
> Created attachment 8485908 [details] [diff] [review]
> review.patch
> 
> (In reply to Mark Côté [:mcote] from comment #1)
> > ::: tests/s1s2test.py
> > @@ +18,5 @@
> > >  
> > >  class S1S2Test(PerfTest):
> > >  
> > > +    def setup_job(self, worker_subprocess):
> > > +        PerfTest.setup_job(self, worker_subprocess)
> > 
> > I know this was in the original, but you don't need to override this if it's
> > only going to be calling the superclass's function of the same name with the
> > same params.
> > 
> 
> But I'm adding to the method, so I need to call the parent class's version,
> don't I?

Yeah sorry, I missed that there was more in that function.  All good.

> > @@ +181,4 @@
> > >                              log_level=self.loglevel,
> > >                              logger_name='autophone.worker.adb',
> > > +                            device_ready_retry_wait=self.options.device_ready_retry_wait,
> > > +                            device_ready_retry_attempts=self.options.device_ready_retry_attempts)
> > 
> > Is there weird indenting here?
> 
> I don't think so.

Yup you're right; never mind.

Comment 5

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/8292c841b53680dee6423ca675fc42cc3e96f8d8