893620 - Fix various issues in Autophone unittests and add ability to set prefs for tests

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

This patch:

* adds test_manifest and test_path to the unittest config files and test runner to allow us to set separate manifests for tests

* ensures log messages passed to the LogDecorator are converted to unicode to prevent UnicodeDecodeErrors on some log files.

* Increases DeviceManager's reboot_settling_time to 120 seconds to help improve stability.

* adds a check to make sure the test config files passed to the test runner actually exist.

* adds a prefs option to the config files and appends the appropriate --setpref arguments to the test runner if prefs is set.

* try up to 3 times to kill any existing fennec processes before starting the test runner.

* adjusts the mochitest chunking from 12 to 9 since autolog will only display 1 digit group numbers.

* adds a new config file for testing Mochitest Canvas with SkiaGL enabled.

Comment 1

[Bob Clary [:bc] (inactive)]

PS. Forgot to add that I have tested all of the test configs with this patch.

Comment 2

[Mark Côté [:mcote]]

Comment on attachment 775444 [details] [diff] [review]
patch v1

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

I didn't run this myself, but it looks good.  Just a few suggestions and nits.

Overall nit: a bunch of the new config options are missing spaces around the =.

::: configs/crashtests_settings.ini
@@ +11,3 @@
>  test_name = crashtest
> +test_manifest=crashtests.list
> +test_path=reftest/tests/testing/crashtest

Is it particularly useful to split the full path to the manifest into two options?  It doesn't seem like it below.  The few places where they are needed separately could easily be obtained via os.path.dirname()/os.path.basename().

::: logdecorator.py
@@ +27,5 @@
> +        except UnicodeDecodeError, e:
> +            etype, evalue, etraceback = sys.exc_info()
> +            extramessage = ''.join(traceback.format_exception(etype, evalue, etraceback))
> +            print extramessage
> +            print message

Are you still getting these errors occasionally?  Regardless, should probably be logged rather than printed.

::: tests/runtestsremote.py
@@ +185,1 @@
>  

Remove blank line.

@@ +185,4 @@
>  
>              test_args = [
>                  'mochitest/runtestsremote.py',
> +                '--robocop=%s/%s' % (test_parameters['test_path'], test_parameters['test_manifest']),

See above comment about whether we need two variables here, but, if so, this is probably better as

'--robocop=%s' % os.path.join(test_parameters['test_path'], test_parameters['test_manifest']),

Same with related changes below (and some existing options, didn't catch that before).

@@ +252,5 @@
>              '--log-file=%s-%s.log' % (test_parameters['test_name'], test_parameters['phone_ip_address']),
>              '--pidfile=%s-%s.pid' % (test_parameters['test_name'], test_parameters['phone_ip_address']),
>          ]
> +        for pref in test_parameters['prefs']:
> +            common_args.append('--setpref=%s' % pref)

If you wanted something slightly fancier, in one line, you could do

common_args.extend(['--setpref=%s' % pref for pref in test_parameters['prefs']])

@@ +434,5 @@
> +                                'Process %s exists. Attempt %d to kill.' % (
> +                                    test_parameters['androidprocname'], kill_attempt))
> +                            self.dm.killProcess(test_parameters['androidprocname'])
> +                            if not self.dm.processExist(test_parameters['androidprocname']):
> +                                break

I think an error log would be useful here if the process still exists after this loop finishes, just in case it causes other weird behaviour.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #2)
> Comment on attachment 775444 [details] [diff] [review]
> patch v1
> 
> Review of attachment 775444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't run this myself, but it looks good.  Just a few suggestions and
> nits.
> 
> Overall nit: a bunch of the new config options are missing spaces around the
> =.
> 

ok.

> ::: configs/crashtests_settings.ini
> @@ +11,3 @@
> >  test_name = crashtest
> > +test_manifest=crashtests.list
> > +test_path=reftest/tests/testing/crashtest
> 
> Is it particularly useful to split the full path to the manifest into two
> options?  It doesn't seem like it below.  The few places where they are
> needed separately could easily be obtained via
> os.path.dirname()/os.path.basename().
> 

mostly required for the non-robocop mochitests as they are really distinct.
the reftest-style tests and the robocop tests could just use a single manifest path.

at one point it seemed clearer to me in the manifest and code to always specify both but i really don't think so anymore.

> ::: logdecorator.py
> @@ +27,5 @@
> > +        except UnicodeDecodeError, e:
> > +            etype, evalue, etraceback = sys.exc_info()
> > +            extramessage = ''.join(traceback.format_exception(etype, evalue, etraceback))
> > +            print extramessage
> > +            print message
> 
> Are you still getting these errors occasionally?  Regardless, should
> probably be logged rather than printed.
> 

No, I think this has worked *so far*. I'm concerned about attempting to use the logger again and hitting the same UnicodeDecodeError. This way, we don't terminate because of a logging issue and we capture the offending output so we can test any patches against it.

> ::: tests/runtestsremote.py
> @@ +185,1 @@
> >  
> 
> Remove blank line.
> 
> @@ +185,4 @@
> >  
> >              test_args = [
> >                  'mochitest/runtestsremote.py',
> > +                '--robocop=%s/%s' % (test_parameters['test_path'], test_parameters['test_manifest']),
> 
> See above comment about whether we need two variables here, but, if so, this
> is probably better as
> 
> '--robocop=%s' % os.path.join(test_parameters['test_path'],
> test_parameters['test_manifest']),
> 
> Same with related changes below (and some existing options, didn't catch
> that before).
> 
> @@ +252,5 @@
> >              '--log-file=%s-%s.log' % (test_parameters['test_name'], test_parameters['phone_ip_address']),
> >              '--pidfile=%s-%s.pid' % (test_parameters['test_name'], test_parameters['phone_ip_address']),
> >          ]
> > +        for pref in test_parameters['prefs']:
> > +            common_args.append('--setpref=%s' % pref)
> 
> If you wanted something slightly fancier, in one line, you could do
> 
> common_args.extend(['--setpref=%s' % pref for pref in
> test_parameters['prefs']])
> 
> @@ +434,5 @@
> > +                                'Process %s exists. Attempt %d to kill.' % (
> > +                                    test_parameters['androidprocname'], kill_attempt))
> > +                            self.dm.killProcess(test_parameters['androidprocname'])
> > +                            if not self.dm.processExist(test_parameters['androidprocname']):
> > +                                break
> 
> I think an error log would be useful here if the process still exists after
> this loop finishes, just in case it causes other weird behaviour.

The runtest will terminate with an error due to the already  running process but I can log it.

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #3)

> > ::: logdecorator.py
> > @@ +27,5 @@
> > > +        except UnicodeDecodeError, e:
> > > +            etype, evalue, etraceback = sys.exc_info()
> > > +            extramessage = ''.join(traceback.format_exception(etype, evalue, etraceback))
> > > +            print extramessage
> > > +            print message
> > 
> > Are you still getting these errors occasionally?  Regardless, should
> > probably be logged rather than printed.
> > 
> 
> No, I think this has worked *so far*. I'm concerned about attempting to use
> the logger again and hitting the same UnicodeDecodeError. This way, we don't
> terminate because of a logging issue and we capture the offending output so
> we can test any patches against it.

Note that this does return the exception which is logged even though the original message is dumped to stdout/.out.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

I think I addressed everything except the prints in the logdecorator.

pushed as https://github.com/mozilla/autophone/commit/19b7f6aea21c27f6fa4d0a3b0875e229f320b2ef

I also took the opportunity to

 rename tests/{mochitests_maniftest.ini => mochitests_manifest.ini}
 rename tests/{mochitests_skia_maniftest.ini => mochitests_skia_manifest.ini}

https://github.com/mozilla/autophone/commit/1b30fde2e877d318aa0cd634b5a8f33a45ad9cd0