Peter Xu <pet...@redhat.com> writes: > On Thu, Dec 19, 2024 at 04:31:04PM -0300, Fabiano Rosas wrote: >> We shouldn't change stuff that's also used by the rest of the >> community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These >> must continue to work the same. > > I see what I overlook; it's used much more than I thought in qtest and we > also have a CI for it.. So ok, let's keep at least QEMU_TEST_FLAKY_TESTS. > > But again, I don't think it matters much even if we rename it, it means the > flaky CI test won't run these two migration tests, but that's not the end > of the world either, if you see what I meant. CI relies on the normal > tests rather than flaky tests to present. > > We should be able to move in / take out FLAKY tests at will, as that's not > what CI is really relying on. Here renaming the macro in migration test > almost means we take both out. > >> >> We can say: "Internally we don't allow slow and flaky to be in the smoke >> set". >> >> We cannot say: "In migration-test QEMU_TEST_FLAKY_TESTS is actually >> QEMU_MIG_TEST_FLAKY, -m slow is actually QEMU_MIG_TEST_EXTRA, -m slow >> just implies KVM and -m quick implies TCG. Easy peasy!" >> >> > >> >> endif >> >> >> >> cmdline invocations: >> >> ./migration-test # runs smoke, i.e. level 1 >> >> ./migration-test -m slow # runs smoke only, no slow tests in >> >> the smoke set >> >> FLAKY=1 ./migration-test # runs smoke only, no flaky tests in >> >> the smoke set >> >> >> >> ./migration-test --full # runs full set, i.e. level 2 >> >> ./migration-test --full -m slow # runs full set + slow tests >> >> FLAKY=1 ./migration-test --full # runs full set + flaky tests >> >> Don't see this^ as a matrix of --full and -m. This is identical to what >> we have today, with the addition of a flag that determines the amount of >> tests run. We could call it other names if we want: >> >> --size small/large >> --testset smoke/full >> >> >> >> >> I made the first one like that so the compat tests in CI now run less >> >> tests. We don't need full set during compat because that job is about >> >> catching changes in device code. It would also make the argument easier >> >> to enable the compat job for all migration-test-supported archs. >> >> >> >> >> >> >> >> I think the best we can do is have a qtest_migration_level_<ARCH> and >> >> >> set it for every arch. >> >> >> >> >> >> Also note that we must keep plain 'migration-test' invocation working >> >> >> because of the compat test. >> >> > >> >> > We won't break it if we only switch to levels, right? >> >> > >> >> > Btw, I also don't know why we need to. IIRC the compat test runs the >> >> > test >> >> > in previous release (but only feeds the new QEMU binary to the old >> >> > migration-test)? I think that's one reason why we decided to use the >> >> > old >> >> > migration-test (so we won't have new tests ran on compat tests, which >> >> > is a >> >> > loss), just to avoid any change in migration-test will break the compat >> >> > test.. so I assume that should be fine regardless.. >> >> >> >> I meant we shouldn't break the command line invocation: >> >> >> >> ./tests/qtest/migration-test -p <test_name> >> >> >> >> As in, we cannot change the test name or add mandatory flags. Otherwise >> >> we have a discrepancy betweem what the CI job is calling vs. what the >> >> build actually provides. We run the tests from the previous build, but >> >> the CI job is current. >> > >> > I failed to follow here. Our CI doesn't hardcode any <test_name>, right? >> > It should invoke "migration-test" in the old build, feeding new QEMU binary >> > to it, run whatever are there in the old test. >> >> Yes, but we have the words "migration-test" in the CI .yaml *today*. It >> doesn't matter if it invokes the tests from the last release. If we >> changed the name to "migration-foobar", then the CI job continues to >> work up until 10.0 is released. It then breaks immediately in the first >> commit of 10.0.50 because the previous release will now have the >> "migration-foobar" name while the CI still calls for "migration-test". > > Not fair at all: nobody suggested to rename the test! >
Right, you suggested new comand line options (level) and I simply *mentioned* that we should pay attention to not affect that CI job. The rest of the exchange is just me trying to clarify. I was not using this as an argument against your idea. >> >> Basically the point is that CI .yaml changes take effect immediately >> while test cmdline changes only take effect (in CI) on the next release. > > Yes, but so far the "API" is the test name only (and actually not.. more > below), and at least no path involved. That's why I want to make sure > we're on the same page. So looks like at least "what tests to run by > default", and "full path of each of the test case" can still change. > > Here's the "more below" part: logically if we want we can change the name > of migration-test. We need to teach the CI on which version of QEMU to use > which program to test in the compat tests. It isn't really hard (a git-tag > -> prog-name hash), it's just unnecessary to change the test name at all. > Sure, we could add code around it if we wanted indeed. >> >> > >> >> >> >> Another point that is more of an annoyance is that the migration-test >> >> invocation not being stable affectes debug, bisect, etc. When debugging >> >> the recent multifd regression I already had to keep changing from >> >> /multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions. >> > >> > So I could have missed something above, and I can understand this adds >> > burden to bisections. However I do prefer we can change test path any way >> > we want (even if in most cases we shouldn't ever touch them, and we should >> > still try to not change them as frequent). IOW we also need to consider >> > the overhead of keeping test paths to be part of ABI as well. >> >> It's annoying, that's all. Makes me 1% more grumpy. > > I'm not going to change any.. but keeping it a protocol is another thing. > > So to summarize.. > > My plan (after adjustment, keeping the name of QEMU_TEST_FLAKY_TESTS) is to > introduce QEMU_TEST_EXTRA_TESTS (renamed following FLAKY) and cover the > three current "slow" tests. Then we stick with -m for the new quick/slow, > which maps to tcg/kvm directly. That saves the extra --full parameter. > > The diff v.s. your plan is, afaict, you prefer introducing yet another > --full parameter. > > Yours is good that we stick with the whole test in compat tests with no > extra change, which is a benefit indeed. It's the other way around. If we add --full, then compat will be !full, so it will start running less tests. This is a "breaking change" of sorts, but since the whole point of the series is to run less stuff in general, then I think it's aligned with the plan. > > If go with my plan, the default compat behavior will start to change after > 10.0 released. We need to do one more step if we still prefer the wholeset > run in compat test, which is at the start of 10.1, we send a patch to > change compat test's parameter from none to "-m slow". > > Feel free to go ahead with whatever you prefer. To me, the more important > bit is whether we both think that one program is better than two, and also > that means decouple host setup v.s. tests. I don't have a strong opinion > otherwise.. Yeah, I can't deal with subverting -m slow. My initial version had this and it didn't felt right. Maybe in 2025 I'll be a new man and we can do differently. Let's see =)
