[Buildroot] [PATCH v3 1/5] support/testing: core testing infrastructure

Luca Ceresoli luca at lucaceresoli.net
Sun May 7 21:07:31 UTC 2017


Hi,

this the review I had written a few days ago. Since the patch has just
been applied, I've reworded it as a list of proposed improvements and
request for clarifications.

On 20/03/2017 21:36, Thomas Petazzoni wrote:
> This commit adds the core of a new testing infrastructure that allows to
> perform runtime testing of Buildroot generated systems. This
> infrastructure uses the Python unittest logic as its foundation.

Good we have this on master now!

Things that I already commented about and for which I plan to send patches:
- remove smart_open
- document code
- better reporting

[...]

> diff --git a/support/testing/infra/__init__.py b/support/testing/infra/__init__.py
> new file mode 100644
> index 0000000..c3f645c
> --- /dev/null
> +++ b/support/testing/infra/__init__.py
> @@ -0,0 +1,89 @@
> +import contextlib
> +import os
> +import re
> +import sys
> +import tempfile
> +import subprocess
> +from urllib2 import urlopen, HTTPError, URLError
> +
> +ARTIFACTS_URL = "http://autobuild.buildroot.net/artefacts/"

Since you renamed artefacts to artifacts, it's probably good to rename
in the URL too (and update the server). Sure it's a minor nit, but since
it's an URL that should stay available for long, better having it fixed
as soon as possible.

[...]

> diff --git a/support/testing/infra/emulator.py b/support/testing/infra/emulator.py
> new file mode 100644
> index 0000000..7c476cb
> --- /dev/null
> +++ b/support/testing/infra/emulator.py
> @@ -0,0 +1,135 @@
> +import socket
> +import subprocess
> +import telnetlib
> +
> +import infra
> +import infra.basetest
> +
> +# TODO: Most of the telnet stuff need to be replaced by stdio/pexpect to discuss
> +# with the qemu machine.
> +class Emulator(object):
> +
> +    def __init__(self, builddir, downloaddir, logtofile):
> +        self.qemu = None
> +        self.__tn = None
> +        self.downloaddir = downloaddir
> +        self.log = ""
> +        self.log_file = "{}-run.log".format(builddir)
> +        if logtofile is None:
> +            self.log_file = None
> +
> +    # Start Qemu to boot the system
> +    #
> +    # arch: Qemu architecture to use
> +    #
> +    # kernel: path to the kernel image, or the special string
> +    # 'builtin'. 'builtin' means a pre-built kernel image will be
> +    # downloaded from ARTEFACTS_URL and suitable options are
> +    # automatically passed to qemu and added to the kernel cmdline. So
> +    # far only armv5, armv7 and i386 builtin kernels are available.
> +    # If None, then no kernel is used, and we assume a bootable device
> +    # will be specified.
> +    #
> +    # kernel_cmdline: array of kernel arguments to pass to Qemu -append option
> +    #
> +    # options: array of command line options to pass to Qemu
> +    #
> +    def boot(self, arch, kernel=None, kernel_cmdline=None, options=None):
> +        if arch in ["armv7", "armv5"]:
> +            qemu_arch = "arm"
> +        else:
> +            qemu_arch = arch
> +
> +        qemu_cmd = ["qemu-system-{}".format(qemu_arch),
> +                    "-serial", "telnet::1234,server",
> +                    "-display", "none"]
> +
> +        if options:
> +            qemu_cmd += options
> +
> +        if kernel_cmdline is None:
> +            kernel_cmdline = []
> +
> +        if kernel:
> +            if kernel == "builtin":
> +                if arch in ["armv7", "armv5"]:
> +                    kernel_cmdline.append("console=ttyAMA0")
> +
> +                if arch == "armv7":
> +                    kernel = infra.download(self.downloaddir,
> +                                            "kernel-vexpress")
> +                    dtb = infra.download(self.downloaddir,
> +                                         "vexpress-v2p-ca9.dtb")
> +                    qemu_cmd += ["-dtb", dtb]
> +                    qemu_cmd += ["-M", "vexpress-a9"]
> +                elif arch == "armv5":
> +                    kernel = infra.download(self.downloaddir,
> +                                            "kernel-versatile")
> +                    qemu_cmd += ["-M", "versatilepb"]
> +
> +            qemu_cmd += ["-kernel", kernel]

I'm OK with the "builtin" logic, but I really dislike it being
hard-coded in Emulator.boot(). It's acceptable for the moment since we
have only very few builtin kernels. Should we add more in the future, I
think we should at least have a sort of "database" of builtin kernels,
perhaps in the form of an associative array.

> diff --git a/support/testing/run-tests b/support/testing/run-tests
> new file mode 100755
> index 0000000..339bb66
> --- /dev/null
> +++ b/support/testing/run-tests
> @@ -0,0 +1,83 @@
> +#!/usr/bin/env python2
> +import argparse
> +import sys
> +import os
> +import nose2
> +
> +from infra.basetest import BRTest
> +
> +def main():
> +    parser = argparse.ArgumentParser(description='Run Buildroot tests')
> +    parser.add_argument('testname', nargs='*',
> +                        help='list of test cases to execute')
> +    parser.add_argument('--list', '-l', action='store_true',
> +                        help='list of available test cases')
> +    parser.add_argument('--all', '-a', action='store_true',
> +                        help='execute all test cases')
> +    parser.add_argument('--stdout', '-s', action='store_true',
> +                        help='log everything to stdout')
> +    parser.add_argument('--output', '-o',
> +                        help='output directory')
> +    parser.add_argument('--download', '-d',
> +                        help='download directory')
> +    parser.add_argument('--keep', '-k',
> +                        help='keep build directories',
> +                        action='store_true')

Stylish note: I would put the one-letter form before the long form. This
is not only my personal taste, but also what the manpages usually do,
and what Python does with the automatically-added -h/--help parameter:

  $ ./support/testing/run-tests
  [...]
  optional arguments:
    -h, --help            show this help message and exit
    --list, -l            list of available test cases

I'll send a patch.

-- 
Luca


More information about the buildroot mailing list