Code review comment for lp:~terceiro/lava-dispatcher/device-version

Antonio Terceiro <email address hidden> writes:

> This is the dispatcher part of the device version support implementation.
>
> You will notice that the master image part is not done, since I could
> not get my board to work yet. In the current state of the code, master
> image devices will always return 'unknown' as their device versions.

This looks mostly fine, thanks! Just a couple of comments.

> === modified file 'lava_dispatcher/device/qemu.py' ---
> lava_dispatcher/device/qemu.py 2012-10-17 16:52:05 +0000 +++
> lava_dispatcher/device/qemu.py 2012-10-18 17:49:26 +0000 @@ -20,6
> +20,7 @@
>
> import contextlib
> import logging
> +import subprocess
>
> from lava_dispatcher.device.target import (
> Target
> @@ -76,4 +77,10 @@
> proc = logging_spawn(qemu_cmd, logfile=self.sio, timeout=1200)
> return proc
>
> + def get_device_version(self):
> + try:
> + return subprocess.check_output(["dpkg-query", "--showformat=${Version}", "--show", "qemu-system"])
> + except subprocess.CalledProcessError:
> + return "unknown"

Not sure this is the right thing to do -- should we not parse the
-version of the qemu binary we actually use, which might not be the
packaged version?

> target_class = QEMUTarget
>

> === added file 'lava_dispatcher/tests/test_device_version.py'
> --- lava_dispatcher/tests/test_device_version.py 1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/tests/test_device_version.py 2012-10-18 17:49:26 +0000
> @@ -0,0 +1,59 @@
> +# Copyright (C) 2012 Linaro Limited
> +#
> +# Author: Antonio Terceiro <email address hidden>
> +#
> +# This file is part of LAVA Dispatcher.
> +#
> +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# LAVA Dispatcher 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 General Public License
> +# along with this program; if not, see <http://www.gnu.org/licenses>.
> +
> +from unittest import TestCase
> +import re
> +from lava_dispatcher.tests.helper import LavaDispatcherTestCase, create_device_config
> +
> +from lava_dispatcher.device.target import Target
> +from lava_dispatcher.device.qemu import QEMUTarget
> +from lava_dispatcher.device.fastmodel import FastModelTarget
> +
> +def _create_fastmodel():
> + config = create_device_config('fastmodel01', { 'device_type': 'fastmodel', 'simulator_binary': '/path/to/fastmodel', 'license_server': 'foo.local' })
> + target = FastModelTarget(None, config)
> + return target

Could you call this _create_fastmodel_target() do you think?

> +class TestDeviceVersion(LavaDispatcherTestCase):
> +
> + def test_base(self):
> + target = Target(None, None)
> + assert(type(target.get_device_version()) is str)

It would be more Pythonic to write this as:

self.assertIsInstance(target.get_device_version(), str)

> + def test_qemu(self):
> + target = QEMUTarget(None, None)
> + device_version = target.get_device_version()
> + assert(re.search('^[0-9.]+', device_version))
> +
> + def test_fastmodel(self):
> + banner = "\n".join([
> + "Fast Models [7.1.36 (May 17 2012)]",
> + "Copyright 2000-2012 ARM Limited.",
> + "All Rights Reserved.",
> + "Top component name: RTSM_VE_Cortex_A15x1_A7x1"
> + ])
> + target = _create_fastmodel()
> + version = target._parse_fastmodel_version(banner)
> + self.assertEqual('7.1.36', version)
> +
> + def test_fastmodel_wrong_format(self):
> + client = _create_fastmodel()
> + version = client._parse_fastmodel_version('random string')
> + self.assertEqual('unknown', version)
> +

Thanks for writing tests for the new functionality!