Re: [PATCH 12/22] tests/qtest/migration: Split TLS tests from migration-test.c

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Nov 06, 2024 at 10:05:59AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Nov 05, 2024 at 03:08:27PM -0300, Fabiano Rosas wrote:
> >> The migration-test.c file has become unwieldy large. It's quite
> >> confusing to navigate with all the test definitions mixed with hook
> >> definitions. The TLS tests make this worse with ifdef'ery.
> >> 
> >> Since we're planning on having a smaller set of tests to run as smoke
> >> testing on all architectures, I'm taking the time to split some tests
> >> into their own file.
> >> 
> >> Move the TLS tests into a file of their own.
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >>  tests/qtest/meson.build                  |   8 +-
> >>  tests/qtest/migration-test.c             | 788 +---------------------
> >>  tests/qtest/migration/migration-common.h |   6 +
> >>  tests/qtest/migration/tls-tests.c        | 790 +++++++++++++++++++++++
> >>  4 files changed, 803 insertions(+), 789 deletions(-)
> >>  create mode 100644 tests/qtest/migration/tls-tests.c
> >
> >
> >> diff --git a/tests/qtest/migration/migration-common.h b/tests/qtest/migration/migration-common.h
> >> index 8d0081c698..c546e92259 100644
> >> --- a/tests/qtest/migration/tls-tests.c
> >> +++ b/tests/qtest/migration/tls-tests.c
> >
> >
> >> +
> >> +void migration_test_add_tls(MigrationTestEnv *env)
> >> +{
> >> +    tmpfs = env->tmpfs;
> >> +
> >> +    migration_test_add("/migration/precopy/unix/tls/psk",
> >> +                       test_precopy_unix_tls_psk);
> >> +
> >
> > ...snip...
> >
> >> +}
> >
> > Looking at this, and considering the later patch which introduces
> > 'make qtest-<subsystem>' support, I wonder if we actually need to
> > have a single 'migration-test' binary. Why not just add a main()
> > method to this  test-tests.c, and have a 'migration-test-tls'
> > binary ?
> 
> I did that initially, but then I realised it duplicates the -qmp, -util
> and -common helpers into every new test binary. With the current split,
> that would be 7x.
> 
> Another point is that we can't then implement the smoke tests like in
> this series, we'd have to make every migration-foo-test.c choose between
> smoke or full tests individually. Which is doable, but it seemed against
> your and Alex's suggestions of having 2 separate binaries.
> 
> If we're fine with the duplication in the build, I could go back to that
> approach. Each main() function would need to look like this:
>       
>     if (g_test_thorough() || env->has_kvm) {
>         /* add all tests */
>         migration_test_add();
>         migration_test_add();
>         ...
>     } else {
>         /* only the smoke suite */
>         migration_test_add_smoke();
>     }
> 
> >
> > "make qtest-migration" would provoide a way to run the same level
> > of functionality seen when everything was in one 'migration-test'
> > binary.
> 
> Yes... but not quite, because running all tests with gdb or valgrind
> attached is something that I do frequently, e.g.:
> 
> PYTHON=$(which python3.11) \
> QTEST_QEMU_BINARY_SRC=\'valgrind -q --leak-check=full --show-leak-kinds=definite,indirect \
> --sim-hints=lax-ioctls --suppressions=${BASEDIR}/valgrind-suppressions ./qemu-system-${ARCH}\' \
> QTEST_QEMU_BINARY=./qemu-system-${ARCH} ./tests/qtest/migration-test -r /${ARCH}/migration || exit 1
> 
> QTEST_QEMU_BINARY=./qemu-system-x86_64 QTEST_QEMU_BINARY_SRC='gdb -q
> --ex "set pagination off" --ex "set print thread-events off" --ex
> "handle SIGUSR1 noprint" --ex "handle SIGPIPE noprint" --ex "run" --ex
> "quit \$_exitcode" --args ./qemu-system-x86_64'
> ./tests/qtest/migration-test
> 
> This could probably be done with meson directly, if that happens to fit
> their opinionated view of the world, but I'd rather not delve into that.
> On the other hand, maybe it's not a big deal and I can live with only
> running a group of tests with the -r or -p flag instead of all of them.

Ok, what you have proposed here is a clear improvement of the status
quo, and doesn't block any of the suggestions I made. So lets just
not complicate this patch series further.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|