1079923 - Autophone - update unittests to fix bitrot

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: wip.patch

The Autophone unittests no longer run due to changes in the mochitest and reftest test runners as well as the due to the switch from using SUTAgent to ADB.

The use of Autolog and the replacement logparser also need to be replaced with the current supported approaches and provisions made for reporting to Treeherder and uploading the logs.

The mochitest and reftest runners will also need updating to support using adb instead of sut.

This patch at least gets the tests running.

Comment 1

[Mark Côté [:mcote]]

How much do we care about these, since we have them running in emulators?  I think this is probably pretty low priority, if a priority at all.

Comment 2

[Bob Clary [:bc] (inactive)]

They are needed to run the video tests which are an extremely high priority.

Comment 3

[Mark Côté [:mcote]]

Ah sorry, the relationship between the existing unit tests and the new video tests was not clear to me.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-1079923-v1.patch

move get_logcat from PerfTest to PhoneTest
change phonetest logcat to accumulate logcat output
submit test logs to s3
move loading config files from setup_job to __init__ for phonetest, perftests
move unittest config file init into UnitTest.__init__, elminate the use of config_files to include multiple unittests.
move some build related initialized back to setup_job since we will not have the build until the job is available.
change test manifest config to a space separated list
add chunking to top level
add PhoneTestResult to make the regular tests and unittest have semi common interface for errors
move treeherder stuff into autophonetreeherder and hook up bug search
new ini mochitest manifests
use sut based dm for test runners
add PhoneTest.buildername property for use in Treeherder
local fix for Bug 1110817 - [mozdevice] - adb_android.is_device_ready - UnboundLocalError: local variable 'data' referenced before assignment

previous test runs under Android 2.3 API9 opt, Android 4.0 API10+ opt in

<https://treeherder.allizom.org/#/jobs?repo=mozilla-central&revision=0cf461e62ce5&filter-searchStr=autophone>

<https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=2e0a0c0d7685&filter-searchStr=autophone>

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 8536008 [details] [diff] [review]
bug-1079923-v1.patch

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

I didn't go over this with a fine-toothed comb, but it looks good.  Some nits/suggestions/questions:

::: configs/crashtests_settings.ini
@@ +19,3 @@
>  
> +[treeherder]
> +job_name = Autophone Reftest

Crashtest?

::: configs/robocoptests_settings.ini
@@ +17,5 @@
>  # How many chunks for the test
> +total_chunks = 4
> +
> +[treeherder]
> +job_name = Autophone Mochitest

Robocop?

::: phonetest.py
@@ +52,3 @@
>          self.state = None # phonestatus.TestState
> +        self.result = None # phonestatus.TestResult
> +        self.test_result = PhoneTestResult()

Do we really need both of these now?  Kind of confusing.

@@ +76,4 @@
>          if output != 'device':
>              raise ADBError('PhoneTest:_check_device: Failed')
>  
> +    def _init_logcat(self):

Almost wonder if it's worth putting the logcat functions and vars into their own class...

::: requirements.txt
@@ +1,4 @@
>  -e hg+http://hg.mozilla.org/automation/logparser#egg=logparser
>  -e git+https://github.com/mozilla/treeherder-client.git#egg=treeherder_client
>  beautifulsoup4
> +boto>=2.32.1

Do we need treeherder in here too?

::: s3.py
@@ +1,1 @@
> +# modeled after https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/mixins/treeherder.py

Need a license header.

@@ +1,3 @@
> +# modeled after https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/mixins/treeherder.py
> +
> +# use log file keys based off of the original build url

Are these FIXMEs/TODOs?

@@ +27,5 @@
> +        self.conn = None
> +        self._logger = logger
> +
> +        try:
> +            self.conn = boto.s3.connection.S3Connection(self.access_key_id, self.access_secret_key)

I think something we've seen a few times is the eventual realization that we might want to instantiate an object without automatically connecting.  Balancing YAGNI and future-proofing, maybe this should be a property, instantiated on demand, the first time it's used?

::: tests/runtestsremote.py
@@ +135,5 @@
> +            raise Exception('Job configuration %s does not specify a test' %
> +                            self.config_file)
> +        try:
> +            self.runtest()
> +

Superfluous blank line.

@@ +136,5 @@
> +                            self.config_file)
> +        try:
> +            self.runtest()
> +
> +        except:

There are no specific exceptions we can listen for here?  Might this not potentially get into an infinite loop?

::: tests/s1s2test.py
@@ +25,5 @@
> +        PerfTest.__init__(self, phone, options,
> +                          config_file=config_file,
> +                          enable_unittests=enable_unittests,
> +                          test_devices_repos=test_devices_repos,
> +                          chunk=chunk)

Since S1S2Test has exactly the same arguments as PerfTest, I don't think you need to define S1S2Test's constructor at all.

::: worker.py
@@ +14,2 @@
>  import time
> +import tempfile

Why was this moved down? "tempfile" comes before "time", alphabetically.

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté [:mcote] from comment #5)
> Comment on attachment 8536008 [details] [diff] [review]
> bug-1079923-v1.patch
> 
> Review of attachment 8536008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't go over this with a fine-toothed comb, but it looks good.  Some
> nits/suggestions/questions:
> 
> ::: configs/crashtests_settings.ini
> @@ +19,3 @@
> >  
> > +[treeherder]
> > +job_name = Autophone Reftest
> 
> Crashtest?
> 

Probably Ok. I'll run a quick test to make sure and if it is ok, I'll change it.

> ::: configs/robocoptests_settings.ini
> @@ +17,5 @@
> >  # How many chunks for the test
> > +total_chunks = 4
> > +
> > +[treeherder]
> > +job_name = Autophone Mochitest
> 
> Robocop?
> 

ditto.

> ::: phonetest.py
> @@ +52,3 @@
> >          self.state = None # phonestatus.TestState
> > +        self.result = None # phonestatus.TestResult
> > +        self.test_result = PhoneTestResult()
> 
> Do we really need both of these now?  Kind of confusing.
> 

It is a bit confusing. The result is for use when the overall test fails in some way while the test_result is for the results either parsed from the unit test logs (or the emulated pass/fails from the SmokeTest and PerfTests). They are kind of independent. Let me think about it some more. I knew you would flag this one! ;-)

> @@ +76,4 @@
> >          if output != 'device':
> >              raise ADBError('PhoneTest:_check_device: Failed')
> >  
> > +    def _init_logcat(self):
> 
> Almost wonder if it's worth putting the logcat functions and vars into their
> own class...
> 

Wouldn't be hard. It kind of makes sense to encapsulate it.

> ::: requirements.txt
> @@ +1,4 @@
> >  -e hg+http://hg.mozilla.org/automation/logparser#egg=logparser
> >  -e git+https://github.com/mozilla/treeherder-client.git#egg=treeherder_client
> >  beautifulsoup4
> > +boto>=2.32.1
> 
> Do we need treeherder in here too?
> 

We have the treeherder-client. What else is there?

> ::: s3.py
> @@ +1,1 @@
> > +# modeled after https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/mixins/treeherder.py
> 
> Need a license header.
> 

doh.

> @@ +1,3 @@
> > +# modeled after https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/mixins/treeherder.py
> > +
> > +# use log file keys based off of the original build url
> 
> Are these FIXMEs/TODOs?
> 

Yeah. Initial notes. removed.

> @@ +27,5 @@
> > +        self.conn = None
> > +        self._logger = logger
> > +
> > +        try:
> > +            self.conn = boto.s3.connection.S3Connection(self.access_key_id, self.access_secret_key)
> 
> I think something we've seen a few times is the eventual realization that we
> might want to instantiate an object without automatically connecting. 
> Balancing YAGNI and future-proofing, maybe this should be a property,
> instantiated on demand, the first time it's used?
> 

Ok. Sounds good.

> ::: tests/runtestsremote.py
> @@ +135,5 @@
> > +            raise Exception('Job configuration %s does not specify a test' %
> > +                            self.config_file)
> > +        try:
> > +            self.runtest()
> > +
> 
> Superfluous blank line.
> 
> @@ +136,5 @@
> > +                            self.config_file)
> > +        try:
> > +            self.runtest()
> > +
> > +        except:
> 
> There are no specific exceptions we can listen for here?  Might this not
> potentially get into an infinite loop?
> 

Pretty sure the worst case is it would retry the job up to the phone_retry_limit. If we would like to narrow down the number of exceptions, I'd need more time to make sure that I have all the possible cases covered. I'd rather not delay this any more than I have to. If you like, I can file a follow up bug to clean that up.

> ::: tests/s1s2test.py
> @@ +25,5 @@
> > +        PerfTest.__init__(self, phone, options,
> > +                          config_file=config_file,
> > +                          enable_unittests=enable_unittests,
> > +                          test_devices_repos=test_devices_repos,
> > +                          chunk=chunk)
> 
> Since S1S2Test has exactly the same arguments as PerfTest, I don't think you
> need to define S1S2Test's constructor at all.
> 

I don't understand. S1S2Test must initialize its specific configuration and must initialize its PerfTest configuration and PhoneTest configuration.

> ::: worker.py
> @@ +14,2 @@
> >  import time
> > +import tempfile
> 
> Why was this moved down? "tempfile" comes before "time", alphabetically.

Not intentional. Fixed.

Comment 7

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #6)
> > ::: phonetest.py
> > @@ +52,3 @@
> > >          self.state = None # phonestatus.TestState
> > > +        self.result = None # phonestatus.TestResult
> > > +        self.test_result = PhoneTestResult()
> > 
> > Do we really need both of these now?  Kind of confusing.
> > 
> 
> It is a bit confusing. The result is for use when the overall test fails in
> some way while the test_result is for the results either parsed from the
> unit test logs (or the emulated pass/fails from the SmokeTest and
> PerfTests). They are kind of independent. Let me think about it some more. I
> knew you would flag this one! ;-)

I understand that they do different things, but perhaps they need to be better named and maybe combined into one object.

> > ::: requirements.txt
> > @@ +1,4 @@
> > >  -e hg+http://hg.mozilla.org/automation/logparser#egg=logparser
> > >  -e git+https://github.com/mozilla/treeherder-client.git#egg=treeherder_client
> > >  beautifulsoup4
> > > +boto>=2.32.1
> > 
> > Do we need treeherder in here too?
> 
> We have the treeherder-client. What else is there?

Ah sorry, I forgot that we already have treeherder support; I was thinking we were adding it in this patch.

> > @@ +136,5 @@
> > > +                            self.config_file)
> > > +        try:
> > > +            self.runtest()
> > > +
> > > +        except:
> > 
> > There are no specific exceptions we can listen for here?  Might this not
> > potentially get into an infinite loop?
> > 
> 
> Pretty sure the worst case is it would retry the job up to the
> phone_retry_limit. If we would like to narrow down the number of exceptions,
> I'd need more time to make sure that I have all the possible cases covered.
> I'd rather not delay this any more than I have to. If you like, I can file a
> follow up bug to clean that up.

Yeah maybe file a follow-up bug.

> > ::: tests/s1s2test.py
> > @@ +25,5 @@
> > > +        PerfTest.__init__(self, phone, options,
> > > +                          config_file=config_file,
> > > +                          enable_unittests=enable_unittests,
> > > +                          test_devices_repos=test_devices_repos,
> > > +                          chunk=chunk)
> > 
> > Since S1S2Test has exactly the same arguments as PerfTest, I don't think you
> > need to define S1S2Test's constructor at all.
> > 
> 
> I don't understand. S1S2Test must initialize its specific configuration and
> must initialize its PerfTest configuration and PhoneTest configuration.

Yeah nevermind; I didn't realize you had merged setup_job() in as well.  I blame Splinter. :)

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: review.patch

* moved TestState from phonestatus.py to autophonetreeherder.py since it is 
  only used there.

* eliminated PhoneTest.state

* Moved TestResult attributes to PhoneTestResult

* Added status attribute to PhoneTestResult to record the TestResult attributes
  independently from the pass/fail/todo information.

* Changed the AutophoneTreeherder submit methods' argument test_result to 
  test_status.

* Moved PhoneTestMessage to worker.py since it is only used there.

* Encapsulated PhoneTest's logcat methods into a new class Logcat

* added license to s3.py

* Changed the S3Bucket.bucket attribute to a compute on demand property, 
  eliminating the connection attribute entirely.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: bug-1079923-v2.patch

This is what I'll check in.

Comment 10

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/b8f2f2603faa637322856dda6aeadf30533d57a7

Comment 11

[Bob Clary [:bc] (inactive)]

Missed this in the new Logcat class...
https://github.com/mozilla/autophone/commit/27d6d21429cb6866f91d5ebccea0afcbf0e17c5a