On Wed, Nov 06, 2024 at 10:05:59AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berra...@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 <faro...@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 :|
