Re: [PATCH v4 3/3] test: Convert build tests to exeter

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8257 bytes --]

On Tue, Aug 19, 2025 at 04:27:54PM +0200, Stefano Brivio wrote:
> On Thu,  7 Aug 2025 21:32:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Convert the tests in build/all to be based on exeter.  The new version of
> > the tests is more robust than the original, since it makes a temporary copy
> > of the source tree so will not be affected by concurrent manual builds.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  test/build/all      | 61 --------------------------------
> >  test/build/build.py | 84 +++++++++++++++++++++++++++++++++++++++++++++
> >  test/run            |  8 ++---
> >  3 files changed, 88 insertions(+), 65 deletions(-)
> >  delete mode 100644 test/build/all
> >  create mode 100755 test/build/build.py
> > 
> > diff --git a/test/build/all b/test/build/all
> > deleted file mode 100644
> > index 1f79e0d8..00000000
> > --- a/test/build/all
> > +++ /dev/null
> > @@ -1,61 +0,0 @@
> > -# SPDX-License-Identifier: GPL-2.0-or-later
> > -#
> > -# PASST - Plug A Simple Socket Transport
> > -#  for qemu/UNIX domain socket mode
> > -#
> > -# PASTA - Pack A Subtle Tap Abstraction
> > -#  for network namespace/tap device mode
> > -#
> > -# test/build/all - Build targets, one by one, then all together, check output
> > -#
> > -# Copyright (c) 2021 Red Hat GmbH
> > -# Author: Stefano Brivio <sbrivio@redhat.com>
> > -
> > -htools	make cc rm uname getconf mkdir cp rm man
> > -
> > -test	Build passt
> > -host	make clean
> > -check	! [ -e passt ]
> > -host	CFLAGS="-Werror" make passt
> > -check	[ -f passt ]
> > -
> > -test	Build pasta
> > -host	make clean
> > -check	! [ -e pasta ]
> > -host	CFLAGS="-Werror" make pasta
> > -check	[ -h pasta ]
> > -
> > -test	Build qrap
> > -host	make clean
> > -check	! [ -e qrap ]
> > -host	CFLAGS="-Werror" make qrap
> > -check	[ -f qrap ]
> > -
> > -test	Build all
> > -host	make clean
> > -check	! [ -e passt ]
> > -check	! [ -e pasta ]
> > -check	! [ -e qrap ]
> > -host	CFLAGS="-Werror" make
> > -check	[ -f passt ]
> > -check	[ -h pasta ]
> > -check	[ -f qrap ]
> > -
> > -test	Install
> > -host	mkdir __STATEDIR__/prefix
> > -host	prefix=__STATEDIR__/prefix make install
> > -check	[ -f __STATEDIR__/prefix/bin/passt ]
> > -check	[ -h __STATEDIR__/prefix/bin/pasta ]
> > -check	[ -f __STATEDIR__/prefix/bin/qrap ]
> > -check	man -M __STATEDIR__/prefix/share/man -W passt
> > -check	man -M __STATEDIR__/prefix/share/man -W pasta
> > -check	man -M __STATEDIR__/prefix/share/man -W qrap
> > -
> > -test	Uninstall
> > -host	prefix=__STATEDIR__/prefix make uninstall
> > -check	! [ -f __STATEDIR__/prefix/bin/passt ]
> > -check	! [ -h __STATEDIR__/prefix/bin/pasta ]
> > -check	! [ -f __STATEDIR__/prefix/bin/qrap ]
> > -check	! man -M __STATEDIR__/prefix/share/man -W passt 2>/dev/null
> > -check	! man -M __STATEDIR__/prefix/share/man -W pasta 2>/dev/null
> > -check	! man -M __STATEDIR__/prefix/share/man -W qrap 2>/dev/null
> > diff --git a/test/build/build.py b/test/build/build.py
> > new file mode 100755
> > index 00000000..12bb82d8
> > --- /dev/null
> > +++ b/test/build/build.py
> > @@ -0,0 +1,84 @@
> > +#! /usr/bin/env python3
> > +#
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# PASST - Plug A Simple Socket Transport
> > +#  for qemu/UNIX domain socket mode
> > +#
> > +# PASTA - Pack A Subtle Tap Abstraction
> > +#  for network namespace/tap device mode
> > +#
> > +# test/build/build.py - Test build and install targets
> > +#
> > +# Copyright Red Hat
> > +# Author: David Gibson <david@gibson.dropbear.id.au>
> > +
> > +import contextlib
> > +import os
> > +from pathlib import Path
> > +import subprocess
> > +import tempfile
> > +from typing import Iterable, Iterator
> > +
> > +import exeter
> > +
> > +def sh(cmd):
> > +    subprocess.run(cmd, shell=True)
> > +
> > +
> > +@contextlib.contextmanager
> > +def clone_sources() -> Iterator[str]:
> > +    os.chdir('..')  # Move from test/ to repo base
> > +    with tempfile.TemporaryDirectory(ignore_cleanup_errors=False) as tmpdir:
> 
> I guess I see the advantage of this syntax, it's still a bit less
> obvious to me than a mere sequence of steps and an explicit cleanup
> function.

I agree that it's less obvious, but I think the advantages are worth
it.  Namely that it will correctly run the cleanup in nearly all cases
of an interrupted test (not a SIGKILL, obviously).

Arguably, it's not a good trade off in this simple case.  However once
we get to real network tests, it's pretty common to have multiple
nested layers of setup, each with their own teardown.  Sometimes you
need something torn down, but it's only set up partway through the
test, or you want something for the first part of the test but not
after, so it needs to be torn down only if interrupted at certain
points.  Manually keeping track of that quickly becomes really
painful, I really want to use this technique there.

Given that I intend to use it there, I think it makes sense to use it
here too: after all it will be even less obvious if there are only
hard examples of it and no easy ones.

> In any case, it's clear enough to copy and paste if needed, and if one
> has the possibility to define tests as you showed in 2/3 (even just as
> an emergency / quick experiment thing), I think it's absolutely fine.
> 
> > +        sh(f"cp --parents -d $(git ls-files) {tmpdir}")
> > +        os.chdir(tmpdir)
> > +        yield tmpdir
> > +
> > +
> > +def test_make(target: str, outputs: Iterable[str]) -> None:
> 
> I probably mentioned this offline but it's hard to figure out that this
> produces a list of executable paths as output.

It doesn't, which kind of reinforces your point.  It takes a list of
files it expects the make to generate.

> A comment or a different
> argument name (binary_output_files?) might help. A simple list might be
> a bit more obvious than Iterable, I suppose.

Yeah, I'll work on all of the above, and try to make it clearer.

> 
> > +    outpaths = [Path(o) for o in outputs]
> > +    with clone_sources():
> > +        for o in outpaths:
> > +            assert not o.exists(), f"{o} existed before make"
> > +        sh(f'make {target} CFLAGS="-Werror"')
> > +        for o in outpaths:
> > +            assert o.exists(), f"{o} wasn't made"
> > +        sh('make clean')
> > +        for o in outpaths:
> > +            assert not o.exists(), f"{o} existed after make clean"
> > +
> > +
> > +exeter.register('make_passt', test_make, 'passt', ['passt'])
> > +exeter.register('make_pasta', test_make, 'pasta', ['pasta'])
> > +exeter.register('make_qrap', test_make, 'qrap', ['qrap'])
> > +exeter.register('make_all', test_make, 'all', ['passt', 'pasta', 'qrap'])
> > +
> > +
> > +@exeter.test
> > +def test_install_uninstall() -> None:
> > +    with clone_sources():
> > +        with tempfile.TemporaryDirectory(ignore_cleanup_errors=False) \
> > +             as prefix:
> > +            bindir = Path(prefix) / 'bin'
> > +            mandir = Path(prefix) / 'share/man'
> > +            progs = ['passt', 'pasta', 'qrap']
> > +
> > +            # Install
> > +            sh(f'make install CFLAGS="-Werror" prefix={prefix}')
> > +
> > +            for prog in progs:
> > +                exe = bindir / prog
> > +                assert exe.is_file(), f"{exe} does not exist as a regular file"
> > +                sh(f'man -M {mandir} -W {prog}')
> > +
> > +            # Uninstall
> > +            sh(f'make uninstall prefix={prefix}')
> > +
> > +            for prog in progs:
> > +                exe = bindir / prog
> > +                assert not exe.exists(), f"{exe} exists after uninstall"
> > +                sh(f'! man -M {mandir} -W {prog}')
> 
> This looks almost as simple and obvious as the shell script equivalent,
> which is quite remarkable.

Hooray!

f"" strings and that sh() wrapper help substantially.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]