Closed
Bug 1079923
Opened 10 years ago
Closed 9 years ago
Autophone - update unittests to fix bitrot
Categories
(Testing Graveyard :: Autophone, defect)
Testing Graveyard
Autophone
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bc, Assigned: bc)
References
Details
Attachments
(4 files)
14.70 KB,
patch
|
Details | Diff | Splinter Review | |
210.73 KB,
patch
|
mcote
:
review+
|
Details | Diff | Splinter Review |
28.03 KB,
patch
|
Details | Diff | Splinter Review | |
218.27 KB,
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
They are needed to run the video tests which are an extremely high priority.
Comment 3•10 years ago
|
||
Ah sorry, the relationship between the existing unit tests and the new video tests was not clear to me.
Assignee | ||
Comment 4•9 years ago
|
||
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>
Attachment #8536008 -
Flags: review?(mcote)
Comment 5•9 years ago
|
||
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.
Attachment #8536008 -
Flags: review?(mcote) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
(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. :)
Assignee | ||
Comment 8•9 years ago
|
||
* 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.
Assignee | ||
Comment 9•9 years ago
|
||
This is what I'll check in.
Assignee | ||
Comment 10•9 years ago
|
||
https://github.com/mozilla/autophone/commit/b8f2f2603faa637322856dda6aeadf30533d57a7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•9 years ago
|
||
Missed this in the new Logcat class... https://github.com/mozilla/autophone/commit/27d6d21429cb6866f91d5ebccea0afcbf0e17c5a
Updated•2 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•