Merge into trunk : pep8 : Code : LAVA Dispatcher

Status:	Merged
Approved by:	Neil Williams on 2013-07-18
Approved revision:	677
Merged at revision:	638
Proposed branch:	lp://staging/~codehelp/lava-dispatcher/pep8
Merge into:	lp://staging/lava-dispatcher
Diff against target:	2308 lines (+351/-313) 43 files modified doc/conf.py (+10/-9) doc/examples/plugins/demo-action-plugin/demo_action_plugin/foo.py (+2/-1) doc/examples/plugins/demo-action-plugin/setup.py (+3/-3) doc/external_measurement.rst (+1/-1) lava/dispatcher/commands.py (+3/-2) lava_dispatcher/actions/__init__.py (+3/-2) lava_dispatcher/actions/android_install_cts_medias.py (+2/-2) lava_dispatcher/actions/boot_control.py (+8/-4) lava_dispatcher/actions/deploy.py (+9/-9) lava_dispatcher/actions/launch_control.py (+10/-7) lava_dispatcher/actions/lava_android_test.py (+31/-32) lava_dispatcher/actions/lava_test_shell.py (+27/-23) lava_dispatcher/client/base.py (+13/-10) lava_dispatcher/client/lmc_utils.py (+26/-27) lava_dispatcher/client/targetdevice.py (+1/-1) lava_dispatcher/config.py (+7/-1) lava_dispatcher/context.py (+2/-1) lava_dispatcher/device/capri.py (+7/-4) lava_dispatcher/device/fastboot.py (+3/-3) lava_dispatcher/device/fastmodel.py (+13/-13) lava_dispatcher/device/ipmi_pxe.py (+17/-22) lava_dispatcher/device/k3v2.py (+7/-4) lava_dispatcher/device/master.py (+20/-21) lava_dispatcher/device/nexus10.py (+5/-2) lava_dispatcher/device/qemu.py (+3/-3) lava_dispatcher/device/sdmux.py (+1/-1) lava_dispatcher/device/target.py (+4/-4) lava_dispatcher/device/uefi.py (+1/-1) lava_dispatcher/device/vexpress.py (+5/-4) lava_dispatcher/downloader.py (+8/-9) lava_dispatcher/job.py (+23/-23) lava_dispatcher/lava_test_shell.py (+26/-27) lava_dispatcher/signals/__init__.py (+0/-2) lava_dispatcher/signals/duration.py (+2/-2) lava_dispatcher/signals/shellhooks.py (+8/-2) lava_dispatcher/tarballcache.py (+6/-6) lava_dispatcher/test_data.py (+3/-2) lava_dispatcher/tests/__init__.py (+1/-0) lava_dispatcher/tests/helper.py (+6/-1) lava_dispatcher/tests/test_config.py (+5/-5) lava_dispatcher/tests/test_device_version.py (+8/-6) lava_dispatcher/utils.py (+6/-6) setup.py (+5/-5)
To merge this branch:	bzr merge lp://staging/~codehelp/lava-dispatcher/pep8
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Antonio Terceiro		2013-07-17	Approve on 2013-07-17
Review via email: mp+175231@code.staging.launchpad.net

This proposal supersedes a proposal from 2013-07-16.

Description of the change

The complete PEP8 set of changes in preparation for the MultiNode merge.

http://cards.linaro.org/browse/LAVA-485

There are a variety of changes here - a lot of whitespace changes and:

0: Fixing mutable default arguments (var=[])
1: Fixing variables which shadow keywords (dir -> from_dir)
2: Fixing docstrings markup (change ''' to """)
3: Fix some equality operators which should be 'is' instead of '=='
4: Some functions fail to resolve due to missing declarations in the base class.

Updated for changes during first review by Antonio.

Revision history for this message

Antonio Terceiro (terceiro) wrote on 2013-07-17: Posted in a previous version of this proposal

#

Download full text (20.0 KiB)

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'
> --- lava_dispatcher/actions/lava_android_test.py 2013-04-23 10:33:49 +0000
> +++ lava_dispatcher/actions/lava_android_test.py 2013-07-16 16:30:42 +0000
> @@ -42,13 +42,12 @@
> 'test_name': {'type': 'string'},
> 'option': {'type': 'string', 'optional': True},
> 'timeout': {'type': 'integer', 'optional': True},
> - },
> + },
> 'additionalProperties': False,
> - }
> + }
>
> def test_name(self, test_name, option=None, timeout=-1):
> - return super(cmd_lava_android_test_run, self).test_name() + \
> - ' (%s)' % test_name
> + return super(cmd_lava_android_test_run, self).test_name() + ' (%s)' % test_name
>
> def run(self, test_name, option=None, timeout=-1):
> #Make sure in test image now
> @@ -56,9 +55,9 @@
> with self.client.android_tester_session() as session:
> bundle_name = generate_bundle_file_name(test_name)
> cmds = ["lava-android-test", 'run', test_name,
> - '-s', session.dev_name,
> - '-o', '%s/%s.bundle' % (self.context.host_result_dir,
> - bundle_name)]
> + '-s', session.dev_name,
> + '-o', '%s/%s.bundle' % (self.context.host_result_dir,
> + bundle_name)]
> if option is not None:
> cmds.extend(['-O', option])
> if timeout != -1:
> @@ -72,8 +71,8 @@
> t.join()
> if rc == 124:
> 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/

> elif rc != 0:
> raise OperationFailed(
> "Failed to run test case(%s) on device(%s) with return "
> @@ -86,15 +85,15 @@
> 'type': 'object',
> 'properties': {
> 'commands': {'type': 'array', 'items': {'type': 'string'},
> - 'optional': True},
> + 'optional': True},
> 'command_file': {'type': 'string', 'optional': True},
> 'parser': {'type': 'string', 'optional': True},
> 'timeout': {'type': 'integer', 'optional': True},
> - },
> + },
> 'additionalProperties': False,
> - }
> + }
>
> - def test_name(self, commands=[], command_file=None, parser=None,
> + def test_name(self, commands=None, command_file=None, parser=None,
> timeout=-1):
> if commands:
> return '%s (commands=[%s])' % (
> @@ -102,10 +101,11 @@
> ','....

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'
> --- lava_dispatcher/actions/lava_android_test.py	2013-04-23 10:33:49 +0000
> +++ lava_dispatcher/actions/lava_android_test.py	2013-07-16 16:30:42 +0000
> @@ -42,13 +42,12 @@
>              'test_name': {'type': 'string'},
>              'option': {'type': 'string', 'optional': True},
>              'timeout': {'type': 'integer', 'optional': True},
> -            },
> +        },
>          'additionalProperties': False,
> -        }
> +    }
>  
>      def test_name(self, test_name, option=None, timeout=-1):
> -        return super(cmd_lava_android_test_run, self).test_name() + \
> -               ' (%s)' % test_name
> +        return super(cmd_lava_android_test_run, self).test_name() + ' (%s)' % test_name
>  
>      def run(self, test_name, option=None, timeout=-1):
>          #Make sure in test image now
> @@ -56,9 +55,9 @@
>          with self.client.android_tester_session() as session:
>              bundle_name = generate_bundle_file_name(test_name)
>              cmds = ["lava-android-test", 'run', test_name,
> -                     '-s', session.dev_name,
> -                     '-o', '%s/%s.bundle' % (self.context.host_result_dir,
> -                                             bundle_name)]
> +                    '-s', session.dev_name,
> +                    '-o', '%s/%s.bundle' % (self.context.host_result_dir,
> +                                            bundle_name)]
>              if option is not None:
>                  cmds.extend(['-O', option])
>              if timeout != -1:
> @@ -72,8 +71,8 @@
>              t.join()
>              if rc == 124:
>                  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/

>              elif rc != 0:
>                  raise OperationFailed(
>                      "Failed to run test case(%s) on device(%s) with return "
> @@ -86,15 +85,15 @@
>          'type': 'object',
>          'properties': {
>              'commands': {'type': 'array', 'items': {'type': 'string'},
> -                          'optional': True},
> +                         'optional': True},
>              'command_file': {'type': 'string', 'optional': True},
>              'parser': {'type': 'string', 'optional': True},
>              'timeout': {'type': 'integer', 'optional': True},
> -            },
> +        },
>          'additionalProperties': False,
> -        }
> +    }
>  
> -    def test_name(self, commands=[], command_file=None, parser=None,
> +    def test_name(self, commands=None, command_file=None, parser=None,
>                    timeout=-1):
>          if commands:
>              return '%s (commands=[%s])' % (
> @@ -102,10 +101,11 @@
>                  ','.join(commands))
>          elif command_file:
>              return '%s (command-file=%s)' % (
> -                super(cmd_lava_android_test_run_custom, self).test_name(),
> -               command_file)
> +                super(cmd_lava_android_test_run_custom, self).test_name(), command_file)
>  
> -    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.

>          #Make sure in test image now
>          self.check_lava_android_test_installed()
>          if commands or command_file:
> @@ -134,8 +134,8 @@
>                  rc = self.context.run_command(cmds)
>                  if rc == 124:
>                      raise TimeoutError(
> -                               "The test (%s) on device(%s) times out." % (
> -                                            ' '.join(cmds), session.dev_name))
> +                        "The test (%s) on device(%s) times out." % (
> +                        ' '.join(cmds), session.dev_name))
>                  elif rc != 0:
>                      raise OperationFailed(
>                          "Failed to run test custom case[%s] on device(%s)"
> @@ -144,26 +144,24 @@
>  
>  
>  class cmd_lava_android_test_run_monkeyrunner(AndroidTestAction):
> -    '''
> +    """
>      This action is added to make doing the monkeyrunner script test more easily
>      from android build page. With this action, we only need to specify the url
>      of the repository where the monkeyrunner script are stored.
>      Then lava-android-test will run all the monkeyrunner scripts in that
>      repository, and help to gather all the png files genereated when run
> -    '''
> +    """
>      parameters_schema = {
>          'type': 'object',
>          'properties': {
>              'url': {'type': 'string'},
>              'timeout': {'type': 'integer', 'optional': True},
> -            },
> +        },
>          'additionalProperties': False,
> -        }
> +    }
>  
>      def test_name(self, url=None, timeout=-1):
> -        return '%s (url=[%s])' % (
> -             super(cmd_lava_android_test_run_monkeyrunner, self).test_name(),
> -                url)
> +        return '%s (url=[%s])' % (super(cmd_lava_android_test_run_monkeyrunner, self).test_name(), url)
>  
>      def run(self, url=None, timeout=-1):
>          #Make sure in test image now
> @@ -181,9 +179,7 @@
>              logging.info("Execute command on host: %s" % (' '.join(cmds)))
>              rc = self.context.run_command(cmds)
>              if rc == 124:
> -                raise TimeoutError(
> -                       "Failed to run monkeyrunner test url[%s] "
> -                       "on device(%s)" % (url, session.dev_name))
> +                raise TimeoutError("Failed to run monkeyrunner test url[%s] on device(%s)" % (url, session.dev_name))
>              elif rc != 0:
>                  raise OperationFailed(
>                      "Failed to run monkeyrunner test url[%s] on device(%s)"
> @@ -201,9 +197,9 @@
>              'tests': {'type': 'array', 'items': {'type': 'string'}},
>              'option': {'type': 'string', 'optional': True},
>              'timeout': {'type': 'integer', 'optional': True},
> -            },
> +        },
>          'additionalProperties': False,
> -        }
> +    }
>  
>      def run(self, tests, option=None, timeout=2400):
>          self.check_lava_android_test_installed()
> @@ -222,8 +218,7 @@
>                  if rc == 124:
>                      raise OperationFailed(
>                          "The installation of test case(%s)"
> -                        " on device(%s) times out" % (test,
> -                                                     session.dev_name))
> +                        " on device(%s) times out" % (test, session.dev_name))
>                  elif rc != 0:
>                      raise OperationFailed(
>                          "Failed to install test case(%s) on device(%s) with "
> 
> === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> --- lava_dispatcher/actions/lava_test_shell.py	2013-06-21 15:48:44 +0000
> +++ lava_dispatcher/actions/lava_test_shell.py	2013-07-16 16:30:42 +0000
> @@ -127,7 +127,6 @@
>  from lava_dispatcher import utils
>  
>  from lava_dispatcher.actions import BaseAction
> -from lava_dispatcher.client.base import wait_for_prompt
>  from lava_dispatcher.device.target import Target
>  from lava_dispatcher.downloader import download_image
>  
> @@ -156,6 +155,7 @@
>  # 755 file permissions
>  XMOD = stat.S_IRWXU | stat.S_IXGRP | stat.S_IRGRP | stat.S_IXOTH | stat.S_IROTH
>  
> +
>  def _get_testdef_git_repo(testdef_repo, tmpdir, revision):
>      cwd = os.getcwd()
>      gitdir = os.path.join(tmpdir, 'gittestrepo')
> @@ -241,6 +241,8 @@
>  
>      def load_from_repo(self, testdef_repo):
>          tmpdir = utils.mkdtemp(self.tmpbase)
> +        repo = None
> +        info = None
>          if 'git-repo' in testdef_repo:
>              repo = _get_testdef_git_repo(
>                  testdef_repo['git-repo'], tmpdir, testdef_repo.get('revision'))
> @@ -253,6 +255,8 @@
>              name = testdef_repo['bzr-repo'].replace('lp:', '').split('/')[-1]
>              info = _bzr_info(testdef_repo['bzr-repo'], repo, name)
>  
> +        if not repo or not info:
> +            logging.debug("Unable to identify specified repository. %s" % testdef_repo)
>          test = testdef_repo.get('testdef', 'lavatest.yaml')
>          with open(os.path.join(repo, test), 'r') as f:
>              logging.info('loading test definition ...')
> @@ -273,7 +277,7 @@
>              'branch_vcs': 'bzr',
>              'branch_revision': revno,
>              'branch_url': url,
> -            }
> +        }
>      finally:
>          os.chdir(cwd)
>  
> @@ -289,7 +293,7 @@
>              'branch_vcs': 'git',
>              'branch_revision': commit_id,
>              'branch_url': url,
> -            }
> +        }
>      finally:
>          os.chdir(cwd)
>  
> @@ -344,7 +348,8 @@
>                  # have non-reproducible behavior because it may rely on
>                  # bzr whoami value, presence of ssh keys, etc.
>                  subprocess.check_call(['bzr', 'branch', repo],
> -                    env={'BZR_HOME': '/dev/null', 'BZR_LOG': '/dev/null'})
> +                                      env={'BZR_HOME': '/dev/null',
> +                                           'BZR_LOG': '/dev/null'})
>                  name = repo.replace('lp:', '').split('/')[-1]
>                  self._sw_sources.append(_bzr_info(repo, name, name))
>  
> @@ -412,8 +417,8 @@
>              f.write('read\n')
>              steps = self.testdef['run'].get('steps', [])
>              if steps:
> -              for cmd in steps:
> -                  f.write('%s\n' % cmd)
> +                for cmd in steps:
> +                    f.write('%s\n' % cmd)
>              f.write('echo "<LAVA_SIGNAL_ENDRUN $TESTRUN_ID $UUID>"\n')
>              f.write('#wait for an ack from the dispatcher\n')
>              f.write('read\n')
> @@ -456,22 +461,22 @@
>              'testdef_repos': {'type': 'array',
>                                'items': {'type': 'object',
>                                          'properties':
> -                                            {'git-repo': {'type': 'string',
> -                                                          'optional': True},
> -                                             'bzr-repo': {'type': 'string',
> -                                                          'optional': True},
> -                                             'revision': {'type': 'string',
> -                                                          'optional': True},
> -                                             '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.

>                                          'additionalProperties': False},
>                                'optional': True
>                                },
>              'timeout': {'type': 'integer', 'optional': True},
> -            },
> +        },
>          'additionalProperties': False,
> -        }
> +    }
>  
>      def run(self, testdef_urls=None, testdef_repos=None, timeout=-1):
>          target = self.client.target_device
> @@ -499,11 +504,11 @@
>  
>      def _keep_running(self, runner, timeout, signal_director):
>          patterns = [
> -                '<LAVA_TEST_RUNNER>: exiting',
> -                pexpect.EOF,
> -                pexpect.TIMEOUT,
> -                '<LAVA_SIGNAL_(\S+) ([^>]+)>',
> -                ]
> +            '<LAVA_TEST_RUNNER>: exiting',
> +            pexpect.EOF,
> +            pexpect.TIMEOUT,
> +            '<LAVA_SIGNAL_(\S+) ([^>]+)>',
> +        ]
>  
>          idx = runner._connection.expect(patterns, timeout=timeout)
>          if idx == 0:
> @@ -609,4 +614,3 @@
>              prefix='lava-test-shell', suffix='.bundle', dir=rdir)
>          with os.fdopen(fd, 'w') as f:
>              DocumentIO.dump(f, bundle)
> -
> 
> === modified file 'lava_dispatcher/client/base.py'
> --- lava_dispatcher/client/base.py	2013-05-22 16:15:22 +0000
> +++ lava_dispatcher/client/base.py	2013-07-16 16:30:42 +0000
> @@ -86,7 +86,7 @@
>          self.match_id = None
>          self.match = None
>  
> -    def wait_for_prompt(self, timeout = -1):
> +    def wait_for_prompt(self, timeout=-1):
>          wait_for_prompt(self._connection, self._prompt_str, timeout)
>  
>      def run(self, cmd, response=None, timeout=-1,
> @@ -227,7 +227,7 @@
>          self.wait_until_attached()
>  
>      def _setup_adb_over_usb(self):
> -        self.run('getprop ro.serialno', response = ['[0-9A-Fa-f]{16}'])
> +        self.run('getprop ro.serialno', response=['[0-9A-Fa-f]{16}'])
>          self.dev_name = self.match.group(0)
>  
>      def disconnect(self):
> @@ -350,6 +350,7 @@
>          self.proc = None
>          # used for apt-get in lava-test.py
>          self.aptget_cmd = "apt-get"
> +        self.target_device = None
>  
>      @contextlib.contextmanager
>      def tester_session(self):
> @@ -412,11 +413,17 @@
>              self.proc.sendline("export http_proxy=%s" % lava_proxy)
>              self.proc.expect(prompt_str, timeout=30)
>              self.aptget_cmd = ' '.join([self.aptget_cmd,
> -                "-o Acquire::http::proxy=%s" % lava_proxy])
> +                                        "-o Acquire::http::proxy=%s" % lava_proxy])
>  
>      def boot_master_image(self):
>          raise NotImplementedError(self.boot_master_image)
>  
> +    def _boot_linaro_image(self):
> +        pass
> +
> +    def _boot_linaro_android_image(self):
> +        pass
> +
>      def boot_linaro_image(self):
>          """
>          Reboot the system to the test image
> @@ -441,7 +448,7 @@
>  
>              try:
>                  wait_for_prompt(self.proc, TESTER_PS1_PATTERN, timeout=timeout)
> -            except (pexpect.TIMEOUT) as e:
> +            except pexpect.TIMEOUT as e:
>                  msg = "Timeout waiting for boot prompt: %s" % e
>                  logging.info(msg)
>                  attempts += 1
> @@ -462,7 +469,7 @@
>          return utils.mkdtemp(self.context.config.lava_image_tmpdir)
>  
>      def get_test_data_attachments(self):
> -        '''returns attachments to go in the "lava_results" test run'''
> +        """returns attachments to go in the "lava_results" test run"""
>          return []
>  
>      def retrieve_results(self, result_disk):
> @@ -546,7 +553,6 @@
>          if not in_linaro_android_image:
>              raise OperationFailed("booting into android test image failed")
>  
> -
>          #check if the adb connection can be created.
>          #by adb connect dev_ip command
>          if adb_check:
> @@ -565,7 +571,7 @@
>                  session.wait_home_screen()
>          except:
>              # ignore home screen exception if it is a health check job.
> -            if not (self.context.job_data.has_key("health_check") and self.context.job_data["health_check"] == True):
> +            if not ('health_check' in self.context.job_data and self.context.job_data["health_check"] is True):

isn't `x is True` equivalent to `x`? Or do we need to explicitly only accept
True as a truth value there?

>                  raise
>              else:
>                  logging.info("Skip raising exception on the home screen has not displayed for health check jobs")
> @@ -582,7 +588,6 @@
>          session.run("netcfg %s dhcp" % self.config.default_network_interface, timeout=300)
>          session.run("ifconfig " + self.config.default_network_interface, timeout=20)
>  
> -
>      def _enable_adb_over_tcp(self):
>          logging.info("Enabling ADB over TCP")
>          session = AndroidTesterCommandRunner(self)
> @@ -595,5 +600,3 @@
>          logging.info("Disabling adb over USB")
>          session = AndroidTesterCommandRunner(self)
>          session.run('echo 0>/sys/class/android_usb/android0/enable')
> -
> -
> 
> === modified file 'lava_dispatcher/device/target.py'
> --- lava_dispatcher/device/target.py	2013-06-28 02:52:46 +0000
> +++ lava_dispatcher/device/target.py	2013-07-16 16:30:42 +0000
> @@ -24,8 +24,7 @@
>  import re
>  
>  from lava_dispatcher.client.lmc_utils import (
> -    image_partition_mounted,
> -    )
> +    image_partition_mounted)
>  import lava_dispatcher.utils as utils
>  
>  
> @@ -49,7 +48,7 @@
>          'TESTER_PS1': ANDROID_TESTER_PS1,
>          'TESTER_PS1_PATTERN': ANDROID_TESTER_PS1,
>          'TESTER_PS1_INCLUDES_RC': False,
> -        }
> +    }
>      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?

> === 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.

> @@ -160,7 +161,7 @@
>          #
>          # --no-anchored matches the name inside any directory in the tarball.
>          self.context.run_command('tar --no-anchored -xaf %s -C %s %s' % (tarball, tmpdir,
> -                                                               uefi_on_image))
> +                                                                         uefi_on_image))
>  
>          uefi_on_image = os.path.join(tmpdir, uefi_on_image)
>          test_uefi = os.path.join(tmpdir, 'uefi.bin')
>

review: Needs Fixing

Revision history for this message

Neil Williams (codehelp) wrote on 2013-07-17: Posted in a previous version of this proposal

#

Download full text (4.9 KiB)

> 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()
>...

LAVA Dispatcher

Merge lp://staging/~codehelp/lava-dispatcher/pep8 into lp://staging/lava-dispatcher

Commit message

Description of the change

Preview Diff

Subscribers