Hi Milo,

Thanks for this. Some of my comments are related to the questions we already
talked about earlier today, but I will also mention them here so others can
also follow. 

 review needs-fixing

> === added file 'lava/base_command.py'
> --- lava/base_command.py	1970-01-01 00:00:00 +0000
> +++ lava/base_command.py	2013-06-17 13:33:27 +0000

I would put this somewhere like lava/helper/command.py instead

Maybe when we start working on the other items (testdef/script) we realize that
we can move all of them in the lava.helper namespace instead of creating
lava.testdef and lava.script.

> @@ -0,0 +1,80 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Milo Casagrande <email address hidden>
> +#
> +# This file is part of lava-tool.
> +#
> +# lava-tool is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-tool is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Base command class common to lava commands series."""
> +
> +import subprocess
> +
> +from lava.config import InteractiveConfig
> +from lava.tool.command import Command
> +from lava.tool.errors import CommandError
> +
> +
> +class BaseCommand(Command):
> +    """Base command class for all lava commands."""
> +    def __init__(self, parser, args):
> +        super(BaseCommand, self).__init__(parser, args)
> +        self.config = InteractiveConfig(
> +            force_interactive=self.args.interactive)
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(BaseCommand, cls).register_arguments(parser)
> +        parser.add_argument("-i", "--interactive",
> +                            action='store_true',
> +                            help=("Forces asking for input parameters even if "
> +                                  "we already have them cached."))

As we talked ealier, I think it's better to do it the other way around: assume
interactivity always, and have a --non-interactive option to force not asking
for input.

> +
> +    @classmethod
> +    def get_dispatcher_paths(cls):
> +        """Tries to get the dispatcher paths from lava-dispatcher.
> +
> +        :return A list of paths.
> +        """
> +        try:
> +            from lava_dispatcher.config import write_path
> +            return write_path()
> +        except ImportError:
> +            raise CommandError("Cannot find lava-dispatcher installation.")
> +
> +    @classmethod
> +    def get_devices(cls):
> +        """Gets the devices list from the dispatcher.
> +
> +        :return A list of DeviceConfig.
> +        """
> +        try:
> +            from lava_dispatcher.config import get_devices
> +            return get_devices()
> +        except ImportError:
> +            raise CommandError("Cannot find lava-dispatcher installation.")
> +
> +    @classmethod
> +    def run(cls, cmd_args):
> +        """Runs the supplied command args.
> +
> +        :param cmd_args: The command, and its optional arguments, to run.
> +        :return The command execution return code.
> +        """
> +        if not isinstance(cmd_args, list):
> +            cmd_args = list(cmd_args)
> +        try:
> +            return subprocess.check_call(cmd_args)
> +        except subprocess.CalledProcessError:
> +            raise CommandError("Error running the following command: "
> +                               "{0}".format(" ".join(cmd_args)))
> 
> === modified file 'lava/job/__init__.py'
> --- lava/job/__init__.py	2013-06-17 13:33:27 +0000
> +++ lava/job/__init__.py	2013-06-17 13:33:27 +0000
> @@ -21,12 +21,13 @@
>  
>  from lava.job.templates import Parameter
>  
> +
>  class Job:
> -
>      def __init__(self, template):
>          self.data = deepcopy(template)
>  
>      def fill_in(self, config):
> +
>          def insert_data(data):
>              if isinstance(data, dict):
>                  keys = data.keys()
> @@ -44,4 +45,3 @@
>  
>      def write(self, stream):
>          stream.write(json.dumps(self.data, indent=4))
> -
> 
> === modified file 'lava/job/commands.py'
> --- lava/job/commands.py	2013-06-17 13:33:27 +0000
> +++ lava/job/commands.py	2013-06-17 13:33:27 +0000
> @@ -16,40 +16,34 @@
>  # You should have received a copy of the GNU Lesser General Public License
>  # along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
>  
> -from os.path import exists
> -
> -from lava.config import InteractiveConfig
> +"""
> +LAVA job commands.
> +"""
> +
> +import os
> +import sys
> +import xmlrpclib
> +
> +from lava.base_command import BaseCommand
> +
> +from lava.config import Parameter
>  from lava.job import Job
> -from lava.job.templates import *
> -from lava.tool.command import Command, CommandGroup
> +from lava.job.templates import (
> +    BOOT_TEST,
> +)
> +from lava.tool.command import CommandGroup
>  from lava.tool.errors import CommandError
> -
>  from lava_tool.authtoken import AuthenticatingServerProxy, KeyringAuthBackend
> -import xmlrpclib
> +from lava_tool.utils import has_command
> +
>  
>  class job(CommandGroup):
> -    """
> -    LAVA job file handling
> -    """
> -
> +    """LAVA job file handling."""
>      namespace = 'lava.job.commands'
>  
> -class BaseCommand(Command):
> -
> -    def __init__(self, parser, args):
> -        super(BaseCommand, self).__init__(parser, args)
> -        self.config = InteractiveConfig(force_interactive=self.args.interactive)
> -
> -    @classmethod
> -    def register_arguments(cls, parser):
> -        super(BaseCommand, cls).register_arguments(parser)
> -        parser.add_argument(
> -            "-i", "--interactive",
> -            action='store_true',
> -            help=("Forces asking for input parameters even if we already "
> -                  "have them cached."))
>  
>  class new(BaseCommand):
> +    """Creates a new job file."""
>  
>      @classmethod
>      def register_arguments(cls, parser):
> @@ -57,20 +51,22 @@
>          parser.add_argument("FILE", help=("Job file to be created."))
>  
>      def invoke(self):
> -        if exists(self.args.FILE):
> -            raise CommandError('%s already exists' % self.args.FILE)
> +        if os.path.exists(self.args.FILE):
> +            raise CommandError('{0} already exists.'.format(self.args.FILE))
>  
> -        with open(self.args.FILE, 'w') as f:
> -            job = Job(BOOT_TEST)
> -            job.fill_in(self.config)
> -            job.write(f)
> +        with open(self.args.FILE, 'w') as job_file:
> +            job_instance = Job(BOOT_TEST)
> +            job_instance.fill_in(self.config)
> +            job_instance.write(job_file)
>  
>  
>  class submit(BaseCommand):
> +    """Submits the specified job file."""
> +
>      @classmethod
>      def register_arguments(cls, parser):
>          super(submit, cls).register_arguments(parser)
> -        parser.add_argument("FILE", help=("The job file to submit"))
> +        parser.add_argument("FILE", help=("The job file to submit."))
>  
>      def invoke(self):
>          jobfile = self.args.FILE
> @@ -85,10 +81,61 @@
>                                             auth_backend=KeyringAuthBackend())
>          try:
>              job_id = server.scheduler.submit_job(jobdata)
> -            print "Job submitted with job ID %d" % job_id
> -        except xmlrpclib.Fault, e:
> -            raise CommandError(str(e))
> +            print >> sys.stdout, "Job submitted with job ID {0}".format(job_id)
> +        except xmlrpclib.Fault, exc:
> +            raise CommandError(str(exc))
> +
>  
>  class run(BaseCommand):
> +    """Runs the specified job file on the local dispatcher."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(run, cls).register_arguments(parser)
> +        parser.add_argument("FILE", help=("The job file to submit."))
> +
> +    @classmethod
> +    def _choose_device(cls, devices):
> +        """Let the user choose the device to use.
> +
> +        :param devices: The list of available devices.
> +        :return The selected device.
> +        """
> +        devices_len = len(devices)
> +        output_list = []
> +        for device, number in zip(devices, range(1, devices_len + 1)):
> +            output_list.append("\t{0}. {1}\n".format(number, device.hostname))
> +
> +        print >> sys.stdout, ("More than one local device found. "
> +                              "Please choose one:\n")
> +        print >> sys.stdout, "".join(output_list)
> +
> +        while True:
> +            try:
> +                user_input = raw_input("Device number to use: ").strip()
> +
> +                if user_input in [str(x) for x in range(1, devices_len + 1)]:
> +                    return devices[int(user_input) - 1].hostname
> +                else:
> +                    continue
> +            except EOFError:
> +                user_input = None
> +            except KeyboardInterrupt:
> +                sys.exit(-1)

I think we should move all the user interaction asking for input to the
config/parameter classes. This way we use the existing mechanism to store
answers in configuration and ease users' life.

Also, if we delegate the actual prompting/reading logic to the parameter class,
we can subclass it to get specialized input prompts. Think of parameters that
are composed of multiple values, etc.

> +
>      def invoke(self):
> -        print("hello world")
> +        if os.path.isfile(self.args.FILE):
> +            if has_command("lava-dispatch"):
> +                devices = self.get_devices()
> +                if devices:
> +                    if len(devices) > 1:
> +                        device = self._choose_device(devices)
> +                    else:
> +                        device = devices[0].hostname
> +                    self.run(["lava-dispatch", "--target", device,
> +                              self.args.FILE])
> +            else:
> +                raise CommandError("Cannot find lava-dispatcher installation.")
> +        else:
> +            raise CommandError("The file '{0}' does not exists. or is not "
> +                               "a file.".format(self.args.FILE))
> 
> === modified file 'lava/job/tests/test_commands.py'
> --- lava/job/tests/test_commands.py	2013-06-17 13:33:27 +0000
> +++ lava/job/tests/test_commands.py	2013-06-17 13:33:27 +0000
> @@ -20,74 +20,108 @@
>  Unit tests for the commands classes
>  """
>  
> -from argparse import ArgumentParser
>  import json
> -from os import (
> -    makedirs,
> -    removedirs,
> -)
> -from os.path import(
> -    exists,
> -    join,
> -)
> -from shutil import(
> -    rmtree,
> -)
> -from tempfile import mkdtemp
> +import os
> +import shutil
> +import sys
> +import tempfile
> +
> +from mock import MagicMock, patch
>  from unittest import TestCase
>  
> -from lava.config import NonInteractiveConfig
> -from lava.job.commands import *
> +from lava.config import NonInteractiveConfig, Parameter
> +
> +from lava.job.commands import (
> +    new,
> +    run,
> +)
> +
>  from lava.tool.errors import CommandError
>  
> -from mocker import Mocker
> -
> -def make_command(command, *args):
> -    parser = ArgumentParser(description="fake argument parser")
> -    command.register_arguments(parser)
> -    the_args = parser.parse_args(*args)
> -    cmd = command(parser, the_args)
> -    cmd.config = NonInteractiveConfig({ 'device_type': 'foo', 'prebuilt_image': 'bar' })
> -    return cmd
>  
>  class CommandTest(TestCase):
>  
>      def setUp(self):
> -        self.tmpdir = mkdtemp()
> +        # Fake the stdout.
> +        self.original_stdout = sys.stdout
> +        sys.stdout = open("/dev/null", "w")
> +        self.original_stderr = sys.stderr
> +        sys.stderr = open("/dev/null", "w")
> +        self.original_stdin = sys.stdin

Are you not touching stdin on purpose?

> +
> +        self.device = "panda02"
> +
> +        self.tmpdir = tempfile.mkdtemp()
> +        self.tmpfile = tempfile.NamedTemporaryFile(delete=False)
> +        self.parser = MagicMock()
> +        self.args = MagicMock()
> +        self.args.interactive = MagicMock(return_value=False)
> +        self.args.FILE = self.tmpfile.name
> +
> +        self.device_type = Parameter('device_type')
> +        self.prebuilt_image = Parameter('prebuilt_image',
> +                                        depends=self.device_type)
> +        self.config = NonInteractiveConfig(
> +            {'device_type': 'foo', 'prebuilt_image': 'bar'})
>  
>      def tearDown(self):
> -        rmtree(self.tmpdir)
> +        sys.stdin = self.original_stdin
> +        sys.stdout = self.original_stdout
> +        sys.stderr = self.original_stderr
> +        os.unlink(self.tmpfile.name)
> +        shutil.rmtree(self.tmpdir)
>  
>      def tmp(self, filename):
> -        return join(self.tmpdir, filename)
> +        """Returns a path to a non existent file.
> +
> +        :param filename: The name the file should have.
> +        :return A path.
> +        """
> +        return os.path.join(self.tmpdir, filename)
> +
>  
>  class JobNewTest(CommandTest):
>  
> +    def setUp(self):
> +        super(JobNewTest, self).setUp()
> +        self.args.FILE = self.tmp("new_file.json")
> +        self.new_command = new(self.parser, self.args)
> +        self.new_command.config = self.config
> +
> +    def tearDown(self):
> +        super(JobNewTest, self).tearDown()
> +        if os.path.exists(self.args.FILE):
> +            os.unlink(self.args.FILE)
> +
>      def test_create_new_file(self):
> -        f = self.tmp('file.json')
> -        command = make_command(new, [f])
> -        command.invoke()
> -        self.assertTrue(exists(f))
> +        self.new_command.invoke()
> +        self.assertTrue(os.path.exists(self.args.FILE))
>  
>      def test_fills_in_template_parameters(self):
> -        f = self.tmp('myjob.json')
> -        command = make_command(new, [f])
> -        command.invoke()
> +        self.new_command.invoke()
>  
> -        data = json.loads(open(f).read())
> +        data = json.loads(open(self.args.FILE).read())
>          self.assertEqual(data['device_type'], 'foo')
>  
> -    def test_wont_overwriteexisting_file(self):
> -        existing = self.tmp('existing.json')
> -        with open(existing, 'w') as f:
> +    def test_wont_overwrite_existing_file(self):
> +        with open(self.args.FILE, 'w') as f:
>              f.write("CONTENTS")
> -        command = make_command(new, [existing])
> -        with self.assertRaises(CommandError):
> -            command.invoke()
> -        self.assertEqual("CONTENTS", open(existing).read())
> -
> -class JobSubmitTest(CommandTest):
> -
> -    def test_receives_job_file_in_cmdline(self):
> -        cmd = make_command(new, ['FOO.json'])
> -        self.assertEqual('FOO.json', cmd.args.FILE)
> +
> +        self.assertRaises(CommandError, self.new_command.invoke)
> +        self.assertEqual("CONTENTS", open(self.args.FILE).read())
> +
> +
> +class JobRunTest(CommandTest):
> +
> +    def test_invoke_raises_0(self):
> +        # Users passes a non existing job file to the run command.
> +        self.args.FILE = self.tmp("test_invoke_raises_0.json")
> +        command = run(self.parser, self.args)
> +        self.assertRaises(CommandError, command.invoke)
> +
> +    @patch("lava.job.commands.has_command", new=MagicMock(return_value=False))
> +    def test_invoke_raises_1(self):
> +        # Users passes a valid file to the run command, but she does not have
> +        # the dispatcher installed.
> +        command = run(self.parser, self.args)
> +        self.assertRaises(CommandError, command.invoke)

maybe you have to do some rebasing before we merge this branch here, but the
above snippet looks to me like you are deleting one test class that is already
there (JobSubmitTest).

-- 
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org