> Thanks for putting this together!
> 
> Most of the changes look fine. I have some comments below that need to
> be addressed we merge this.
>  review needs-fixing
> 
> > === modified file 'lava_dispatcher/actions/lava_android_test.py'
> >                  raise TimeoutError(
> > -                           "The test case(%s) on device(%s) times out" % (
> > -                                                test_name,
> session.dev_name))
> > +                    "The test case(%s) on device(%s) times out" % (
> > +                    test_name, session.dev_name))
> 
> Please use the opportunity to fix a typo here: s/times/timed/

Done.

> > -    def run(self, commands=[], command_file=None, parser=None, timeout=-1):
> > +    def run(self, commands=None, command_file=None, parser=None,
> timeout=-1):
> > +        if not commands:
> > +            commands = []
> 
> This should is not actually needed, since commands is only used below inside a
> `if commands:`. Maybe it makes clear that commands is expected to be a list,
> though.

If something other than a list is passed, the function will fail with or without the
added check. I think I'll remove the if and add a docstring instead.

> > === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> > -                                             'testdef': {'type': 'string',
> > -                                                         'optional': True}
> > -                                             },
> > +                                        {'git-repo': {'type': 'string',
> > +                                        'optional': True},
> > +                                        'bzr-repo': {'type': 'string',
> > +                                        'optional': True},
> > +                                        'revision': {'type': 'string',
> > +                                        'optional': True},
> > +                                        'testdef': {'type': 'string',
> > +                                        'optional': True}
> > +                                         },
> 
> 
> The indentation of this part does not look right at all.

Fixed. It needed a double indent in each case:

                                        {'git-repo': {'type': 'string',
                                                'optional': True},

 
> > === modified file 'lava_dispatcher/device/target.py'
> >      ubuntu_deployment_data = {
> >          'TESTER_PS1': "linaro-test [rc=$(echo \$?)]# ",
> >          'TESTER_PS1_PATTERN': "linaro-test \[rc=(\d+)\]# ",
> > @@ -86,7 +85,7 @@
> >          """
> >          raise NotImplementedError('power_on')
> >
> > -    def deploy_linaro(self, hwpack, rfs):
> > +    def deploy_linaro(self, hwpack, rfs, bootloader):
> >          raise NotImplementedError('deploy_image')
> >
> >      def deploy_android(self, boot, system, userdata):
> > @@ -130,14 +129,14 @@
> >          """ Powers on the target, returning a CommandRunner object and will
> >          power off the target when the context is exited
> >          """
> > -        proc = runner = None
> > +        self.proc = runner = None
> >          try:
> > -            proc = self.power_on()
> > -            runner = self._get_runner(proc)
> > +            self.proc = self.power_on()
> > +            runner = self._get_runner(self.proc)
> >              yield runner
> >          finally:
> > -            if proc and runner:
> > -                self.power_off(proc)
> > +            if self.proc and runner:
> > +                self.power_off(self.proc)
> >
> >      def _get_runner(self, proc):
> >          from lava_dispatcher.client.base import CommandRunner
> 
> it looks like you are changing the logic here. Did you already test this
> change?

It's confusing at this point - this change is linked with the _customize_bootloader function which uses a self.proc but that proc is never defined, using that proc in an expect.

I'll back this change out and put a FIXME in instead.
 
> > === modified file 'lava_dispatcher/device/vexpress.py'
> > --- lava_dispatcher/device/vexpress.py        2013-04-05 21:58:28 +0000
> > +++ lava_dispatcher/device/vexpress.py        2013-07-16 16:30:42 +0000
> > @@ -27,6 +27,7 @@
> >  from lava_dispatcher.device.master import MasterImageTarget
> >  from lava_dispatcher.errors import CriticalError
> >
> > +
> >  class VexpressTarget(MasterImageTarget):
> >
> >      def __init__(self, context, config):
> > @@ -37,7 +38,7 @@
> >          if (self.config.uefi_image_filename is None or
> >              self.config.vexpress_uefi_path is None or
> >              self.config.vexpress_uefi_backup_path is None or
> > -            self.config.vexpress_usb_mass_storage_device is None):
> > +                self.config.vexpress_usb_mass_storage_device is None):
> >
> >              raise CriticalError(
> >                  "Versatile Express devices must specify all "
> 
> This does not look right to me.

Correct, the two lines above also need the indent. I'll fix that.

Thanks, I'll update the merge proposal.