add tests to run binaries under Valgrind

This was quite a bit of refactoring in test/; everything should be
more maintainable and robust in theory.

Also, valgrind.sh was fixed to use exec (so that signals are passed to
the underlying process); Valgrind command line options have also been
tweaked.

./ci.sh fails now, but that should be fixable.

5 files changed, 179 insertions, 78 deletions

diff --git a/Dockerfile b/Dockerfile
index e0eb0e6..a981556 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -4,7 +4,7 @@ ARG install_dir="/app/install"
 
 FROM base AS builder
 
-RUN build_deps='bash bsd-compat-headers build-base clang cmake libgit2-dev py3-pytest sqlite-dev' && \
+RUN build_deps='bash bsd-compat-headers build-base clang cmake coreutils libgit2-dev py3-pytest sqlite-dev valgrind' && \
     apk add -q --no-cache $build_deps
 
 ARG C_COMPILER=clang
diff --git a/src/valgrind.sh b/src/valgrind.sh
index 7223092..088c393 100755
--- a/src/valgrind.sh
+++ b/src/valgrind.sh
@@ -8,4 +8,9 @@ if ! command -v valgrind &> /dev/null; then
     exit 1
 fi
 
-valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose "$@"
+exec valgrind \
+    --leak-check=full \
+    --show-leak-kinds=all \
+    --track-origins=yes \
+    --trace-children=yes \
+    -- "$@"
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 5d16f4e..66b79e9 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -1,11 +1,17 @@
 find_package(Python3 REQUIRED COMPONENTS Interpreter)
 
-add_test(
-    NAME integration_tests
-    COMMAND Python3::Interpreter -m pytest
-        "${CMAKE_CURRENT_SOURCE_DIR}"
-        --server-binary "$<TARGET_FILE:server>"
-        --worker-binary "$<TARGET_FILE:worker>"
-        --client-binary "$<TARGET_FILE:client>"
-        --project-version "${PROJECT_VERSION}"
+set(args
+    "${CMAKE_CURRENT_SOURCE_DIR}"
+    --server-binary "$<TARGET_FILE:server>"
+    --worker-binary "$<TARGET_FILE:worker>"
+    --client-binary "$<TARGET_FILE:client>"
+    --project-version "${PROJECT_VERSION}")
+
+add_test(NAME integration_tests
+    COMMAND Python3::Interpreter -m pytest ${args}
+    WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
+
+add_test(NAME integration_tests_with_valgrind
+    COMMAND Python3::Interpreter -m pytest ${args}
+        --valgrind-binary "${CMAKE_CURRENT_SOURCE_DIR}/../src/valgrind.sh"
     WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
diff --git a/test/conftest.py b/test/conftest.py
index 33e94c5..fa5f36b 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -9,23 +9,27 @@ import random
 
 from pytest import fixture
 
-from .lib.process import run, run_async
+from .lib.process import CmdLine, CmdLineBuilder, Runner
 
 
-class CmdLineOption:
-    def __init__(self, codename, help_string):
+class Param:
+    def __init__(self, codename, help_string, required=True):
         self.codename = codename
         self.help_string = help_string
+        self.required = required
 
     @property
     def cmd_line(self):
         return f"--{self.codename.replace('_', '-')}"
 
+    def add_to_parser(self, parser):
+        parser.addoption(self.cmd_line, required=self.required, help=self.help_string)
 
-class CmdLineBinary(CmdLineOption):
-    def __init__(self, name):
+
+class ParamBinary(Param):
+    def __init__(self, name, **kwargs):
         self.name = name
-        super().__init__(self.get_code_name(), self.get_help_string())
+        super().__init__(self.get_code_name(), self.get_help_string(), **kwargs)
 
     def get_code_name(self):
         return f'{self.name}_binary'
@@ -38,25 +42,34 @@ class CmdLineBinary(CmdLineOption):
         return f'{self.basename} binary path'
 
 
-CMD_LINE_BINARIES = [CmdLineBinary(name) for name in ('server', 'worker', 'client')]
+BINARY_PARAMS = [
+    ParamBinary(name) for name in ('server', 'worker', 'client')
+]
+
+PARAM_VALGRIND = ParamBinary('valgrind', required=False)
 
 
-class CmdLineVersion(CmdLineOption):
+class ParamVersion(Param):
     def __init__(self):
         super().__init__('project_version', 'project version')
 
 
-CMD_LINE_VERSION = CmdLineVersion()
-CMD_LINE_OPTIONS = CMD_LINE_BINARIES + [CMD_LINE_VERSION]
+PARAM_VERSION = ParamVersion()
+
+PARAMS = list(BINARY_PARAMS)
+PARAMS += [
+    PARAM_VALGRIND,
+    PARAM_VERSION,
+]
 
 
 def pytest_addoption(parser):
-    for opt in CMD_LINE_OPTIONS:
-        parser.addoption(opt.cmd_line, required=True, help=opt.help_string)
+    for opt in PARAMS:
+        opt.add_to_parser(parser)
 
 
 def pytest_generate_tests(metafunc):
-    for opt in CMD_LINE_OPTIONS:
+    for opt in PARAMS:
         if opt.codename in metafunc.fixturenames:
             metafunc.parametrize(opt.codename, metafunc.config.getoption(opt.codename))
 
@@ -68,10 +81,14 @@ def rng():
 
 class Paths:
     def __init__(self, pytestconfig):
-        for binary in CMD_LINE_BINARIES:
-            path = pytestconfig.getoption(binary.codename)
-            logging.info('%s path: %s', binary.basename, path)
-            setattr(self, binary.codename, path)
+        for opt in BINARY_PARAMS:
+            setattr(self, opt.codename, None)
+        for opt in BINARY_PARAMS:
+            path = pytestconfig.getoption(opt.codename)
+            if path is None:
+                continue
+            logging.info('%s path: %s', opt.basename, path)
+            setattr(self, opt.codename, path)
 
 
 @fixture(scope='session')
@@ -80,8 +97,17 @@ def paths(pytestconfig):
 
 
 @fixture(scope='session')
+def process_runner(pytestconfig):
+    runner = Runner()
+    valgrind = pytestconfig.getoption(PARAM_VALGRIND.codename)
+    if valgrind is not None:
+        runner.add_wrapper(CmdLine(valgrind))
+    return runner
+
+
+@fixture(scope='session')
 def version(pytestconfig):
-    return pytestconfig.getoption(CMD_LINE_VERSION.codename)
+    return pytestconfig.getoption(PARAM_VERSION.codename)
 
 
 @fixture
@@ -94,17 +120,31 @@ def sqlite_path(tmp_path):
     return os.path.join(tmp_path, 'cimple.sqlite')
 
 
+class CmdLineServer(CmdLine):
+    def log_line_means_launched(self, line):
+        return line.endswith('Waiting for new connections')
+
+
+class CmdLineWorker(CmdLine):
+    def log_line_means_launched(self, line):
+        return line.endswith('Waiting for a new command')
+
+
 @fixture
-def server(paths, server_port, sqlite_path):
-    with run_async(paths.server_binary, '--port', server_port, '--sqlite', sqlite_path) as server:
+def server(process_runner, paths, server_port, sqlite_path):
+    args = ['--port', server_port, '--sqlite', sqlite_path]
+    cmd_line = CmdLineServer(paths.server_binary, *args)
+    with process_runner.run_async(cmd_line) as server:
         yield
     assert server.returncode == 0
 
 
 @fixture
-def workers(paths, server_port):
-    args = [paths.worker_binary, '--host', '127.0.0.1', '--port', server_port]
-    with run_async(*args) as worker1, run_async(*args) as worker2:
+def workers(process_runner, paths, server_port):
+    args = ['--host', '127.0.0.1', '--port', server_port]
+    cmd_line = CmdLineWorker(paths.worker_binary, *args)
+    with process_runner.run_async(cmd_line) as worker1, \
+         process_runner.run_async(cmd_line) as worker2:
         yield
     assert worker1.returncode == 0
     assert worker2.returncode == 0
@@ -115,14 +155,8 @@ def server_and_workers(server, workers):
     yield
 
 
-class Client:
-    def __init__(self, binary):
-        self.binary = binary
-
-    def run(self, *args):
-        return run(self.binary, *args)
-
-
 @fixture
-def client(paths):
-    return Client(paths.client_binary)
+def client(process_runner, paths, server_port):
+    args = ['--port', server_port]
+    cmd_line = CmdLineBuilder(process_runner, paths.client_binary, *args)
+    return cmd_line
diff --git a/test/lib/process.py b/test/lib/process.py
index f757167..428cead 100644
--- a/test/lib/process.py
+++ b/test/lib/process.py
@@ -6,9 +6,9 @@
 from contextlib import contextmanager
 import logging
 import os
+import shutil
 import subprocess
-from threading import Thread
-import time
+from threading import Event, Thread
 
 
 _COMMON_ARGS = {
@@ -19,32 +19,23 @@ _COMMON_ARGS = {
 }
 
 
-def _canonicalize_process_args(binary, *args):
-    binary = os.path.abspath(binary)
-    argv = list(args)
-    argv = [binary] + argv
-    return binary, argv
-
-
-def _log_process_args(binary, argv):
-    if argv:
-        logging.info('Executing binary %s with arguments: %s', binary, ' '.join(argv[1:]))
-    else:
-        logging.info('Executing binary %s', binary)
-
-
 class LoggingThread(Thread):
     def __init__(self, process):
         self.process = process
+        self.launched_event = Event()
         target = lambda pipe: self.consume(pipe)
         super().__init__(target=target, args=[process.stdout])
 
     def consume(self, pipe):
-        for line in iter(pipe):
+        for line in pipe:
+            line = line.removesuffix('\n')
             logging.info('%s: %s', self.process.log_id, line)
+            if self.process.cmd_line.log_line_means_launched(line):
+                self.launched_event.set()
 
     def __enter__(self):
         self.start()
+        self.launched_event.wait()
         return self
 
     def __exit__(self, *args):
@@ -52,17 +43,50 @@ class LoggingThread(Thread):
         self.join()
 
 
-class Process(subprocess.Popen):
-    def __init__(self, binary, *args):
-        binary, argv = _canonicalize_process_args(binary, *args)
-        _log_process_args(binary, argv)
+class CmdLine:
+    @staticmethod
+    def which(binary):
+        if os.path.split(binary)[0]:
+            # shutil.which('bin/bash') doesn't work.
+            return os.path.abspath(binary)
+        path = shutil.which(binary)
+        if path is None:
+            raise RuntimeError("couldn't find a binary: " + binary)
+        return path
+
+    def __init__(self, binary, *args, name=None):
+        binary = self.which(binary)
+        argv = [binary] + list(args)
 
         self.binary = binary
-        self.name = os.path.basename(binary)
+        self.argv = argv
 
-        super().__init__(argv, **_COMMON_ARGS)
-        # TODO: figure out how to remove this.
-        time.sleep(1)
+        if name is None:
+            name = os.path.basename(binary)
+        self.process_name = name
+
+    def log_line_means_launched(self, line):
+        return True
+
+    @classmethod
+    def wrap(cls, outer, inner):
+        return cls(outer.argv[0], *outer.argv[1:], *inner.argv, name=inner.process_name)
+
+    def log_process_start(self):
+        if len(self.argv) > 1:
+            logging.info('Executing binary %s with arguments: %s', self.binary, ' '.join(self.argv[1:]))
+        else:
+            logging.info('Executing binary %s', self.binary)
+
+
+class Process(subprocess.Popen):
+    def __init__(self, cmd_line):
+        self.cmd_line = cmd_line
+
+        cmd_line.log_process_start()
+        self.name = cmd_line.process_name
+
+        super().__init__(cmd_line.argv, **_COMMON_ARGS)
         logging.info('Process %s launched', self.log_id)
 
     @property
@@ -90,15 +114,47 @@ class Process(subprocess.Popen):
         self.wait(timeout=3)
 
 
-@contextmanager
-def run_async(binary, *args):
-    with Process(binary, *args) as process, \
-         LoggingThread(process):
-        yield process
+class Runner:
+    @staticmethod
+    def unbuffered():
+        return CmdLine('stdbuf', '-o0')
+
+    def __init__(self):
+        self.wrappers = []
+        self.add_wrapper(self.unbuffered())
+
+    def add_wrapper(self, cmd_line):
+        self.wrappers.append(cmd_line)
+
+    def _wrap(self, cmd_line):
+        for wrapper in self.wrappers:
+            cmd_line = cmd_line.wrap(wrapper, cmd_line)
+        return cmd_line
+
+    def run(self, cmd_line):
+        cmd_line = self._wrap(cmd_line)
+        cmd_line.log_process_start()
+        result = subprocess.run(cmd_line.argv, **_COMMON_ARGS)
+        return result.returncode, result.stdout
+
+    @contextmanager
+    def run_async(self, cmd_line):
+        cmd_line = self._wrap(cmd_line)
+        with Process(cmd_line) as process, LoggingThread(process):
+            yield process
+
+
+class CmdLineBuilder:
+    def __init__(self, runner, binary, *args):
+        self.runner = runner
+        self.binary = binary
+        self.args = list(args)
+
+    def _build(self, *args):
+        return CmdLine(self.binary, *self.args, *args)
 
+    def run(self, *args):
+        return self.runner.run(self._build(*args))
 
-def run(binary, *args):
-    binary, argv = _canonicalize_process_args(binary, *args)
-    _log_process_args(binary, argv)
-    result = subprocess.run(argv, **_COMMON_ARGS)
-    return result.returncode, result.stdout
+    def run_async(self, *args):
+        return self.runner.run_async(self._build(*args))