On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
> >> diff --git a/tests/qtest/migration-test-smoke.c
> >> b/tests/qtest/migration-test-smoke.c
> >> new file mode 100644
> >> index 0000000000..ff2d72881f
> >> --- /dev/null
> >> +++ b/tests/qtest/migration-test-smoke.c
> >> @@ -0,0 +1,39 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "libqtest.h"
> >> +#include "migration/test-framework.h"
> >> +#include "qemu/module.h"
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> + MigrationTestEnv *env;
> >> + int ret;
> >> +
> >> + g_test_init(&argc, &argv, NULL);
> >> + env = migration_get_env();
> >> + module_call_init(MODULE_INIT_QOM);
> >> +
> >> + if (env->has_kvm) {
> >> + g_test_message(
> >> + "Smoke tests already run as part of the full suite on KVM
> >> hosts");
> >> + goto out;
> >> + }
> >
> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
> >
> > If to rethink our goal, we have two requirements:
> >
> > (1) We want to categorize migration tests, so some are quick, some are
> > slow, some might be flacky. Maybe more, but it's about putting one
> > test into only one bucket, and there're >1 buckets.
>
> It's true that the smoke test should never have slow or flaky tests, but
> we can't use this categorization for anything else. IOW, what you
> describe here is not a goal. If a test is found to be slow we put it
> under slow and it will only run with -m slow/thorough, that's it. We can
> just ignore this.
I could have missed something, but I still think it's the same issue. In
general, I think we want to provide different levels of tests, like:
- Level 1: the minimum set of tests (aka, the "smoke" idea here)
- Level 2: normal set of tests (aka, whatever we used to run by default)
- Level 3: slow tests (aka, only ran with '-m slow' before)
- Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)
Then we want to run level1 test only in tcg, and level1+2 in kvm. We can
only trigger level 1-3 or level 1-4 in manual tests.
We used to have different way to provide the level idea, now I think we can
consider provide that level in migration-test in one shot. Obviously it's
more than quick/slow so I don't think we can reuse "-m", but we can add our
own test level "--level" parameter, so --level N means run all tests lower
than level N, for example.
>
> >
> > (2) We want to run only a small portion of tests on tcg, more tests on
> > kvm.
>
> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
> hosts are the ones where it's actually important to ensure all migration
> features work OK. Non-KVM will only care about save/restore of
> snapshots. Therefore we don't need to have all tests running with TCG,
> only the smoke set.
>
> And "smoke set" is arbitrary, not tied to speed, but of course no slow
> tests please (which already happens because we don't pass -m slow to
> migration-test-smoke).
>
> >
> > Ideally, we don't need two separate main test files, do we?
> >
> > I mean, we can do (1) with the existing migration-test.c, with the help of
> > either gtest's "-m" or something we invent. The only unfortunate part is
> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
> > "slow".. while we don't yet have real "perf" tests. It means we only have
> > two buckets if we want to reuse gtest's "-m".
> >
> > Maybe it's enough? If not, we can implement >2 categories in whatever
> > form, either custom argv/argc cmdline, or env variable.
> >
> > Then, if we always categorize one test (let me try to not reuse glib's
> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
>
> It's either normal or slow. Because we only know a test is only after it
> bothers us.
So I wonder if we can provide four levels, as above.. and define it for
each test in migration-test.
>
> > migration-test that have different level of tests. We can invoke
> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
> > "migration-test --mode SLOW" if kvm is supported.
>
> This is messy due to how qtest/meson.build works. Having two tests is
> the clean change. Otherwise we'll have to add "if migration-test" or
> create artificial test names to be able to restrict the arguments that
> are passed to the test per arch.
Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
but yeah if anyone is not happy with it we can rethink. I just want to
know whether it's still acceptable.
I tried to code it up, it looks like this:
====8<====
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c5a70021c5..5bec33b627 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -392,6 +392,12 @@ if dbus_display
qtests += {'dbus-display-test': [dbus_display1, gio]}
endif
+if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
+ has_kvm = true
+else
+ has_kvm =false
+endif
+
qtest_executables = {}
foreach dir : target_dirs
if not dir.endswith('-softmmu')
@@ -434,11 +440,21 @@ foreach dir : target_dirs
test: executable(test, src, dependencies: deps)
}
endif
+ test_args = ['--tap', '-k']
+ if test == 'migration-test'
+ if host_os == 'linux' and cpu == target_base and has_kvm
+ # Only run full migration test if host kvm supported
+ test_args += ['-m', 'thorough']
+ else
+ test_args += ['-m', 'quick']
+ endif
+ endif
+
test('qtest-@0@/@1@'.format(target_base, test),
qtest_executables[test],
depends: [test_deps, qtest_emulator, emulator_modules],
env: qtest_env,
- args: ['--tap', '-k'],
+ args: test_args,
protocol: 'tap',
timeout: slow_qtests.get(test, 60),
priority: slow_qtests.get(test, 60),
====8<====
I still used "-m" but just to show the idea. I also wonder whether other
tests would have similar demands.. otherwise are we destined to not be able
to use qtest cmdline at all as long as we use meson?
>
> I also *think* we cannot have anything extra in argv because gtester
> expects to be able to parse those.
We can definitely hijack argv/argc before passing it over to glib.
>
> >
> > Would this be nicer? At least we can still run a pretty fast smoke / FAST
> > test even on kvm. Basically, untangle accel v.s. "test category".
>
> We could just remove the restriction from migration-test-smoke if that's
> an issue.
Not the only issue, but the idea of it. In general, IMHO it'll be good we
don't attach host info to a test program.
IOW, I want to keep the test in a way so that we'll be able to run whatever
level of test on whatever host, at least when when I run migration-test
manually.
So e.g. I also want to be able to run full set of tests on TCG too whenever
needed. So I still feel like we mangled these two issues together which
might be unnecessarily.
--
Peter Xu
