Re: [PATCH 12/12] [NOT FOR MERGE] tests/qtest: Introduce qtest_validate_args

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/02/2023 16.04, Fabiano Rosas wrote:
>> The QEMU binary can be built with a varied set of features/devices
>> which are opaque to the tests. Add a centralized point for parsing and
>> validating the command line.
>> 
>> Tests can now be skipped with the following pattern:
>> 
>> qts = qtest_init(args);
>> if (!qts) {
>>      return;
>> }
>> 
>> For now, the only validation is that the -device options all
>> correspond to devices that are actually present in the build.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> Would this be better than checking for missing devices in individual
>> tests?
>
> This is certainly an interesting idea! ... some things still bug me, though:
>
> - We still need to change all the calling sites (to check for
>    !qts) ... so the effort seems to be in a similar ballpark as
>    adding qtest_has_device() to the various problematic tests

Just notice that this series does not cover _all_ -device uses, only the
ones that refer to devices disabled by --without-default-devices. So we
might need to come back to this when some CONFIG changes, new devices
are added, new tests, etc.

> - This will now call qtest_has_device for each and every device
>    in the parameter list, even if it is not necessary. And at
>    least the first call to qtest_has_device() is rather expensive
>    since it has to fire up a separate QEMU to retrieve the list
>    of supported the devices. So adding this to all tests might
>    cause a slow-down to the tests...

Yes, that was my main concern. We could have something like this patch
but as a helper that tests can call. Initially, I had thought of:

if (qtest_validate_args(args)) {
   qts = qtest_init(args);
}

> - It could maybe even hide bugs if you don't look closely, e.g.
>    if you have a typo in the device name in a test, the test then
>    gets skipped automatically instead of failing ... ok, that's
>    unlikely for new tests where you look closely, but still, it
>    gives me slightly bad feeling.

I agree. In fact I have been looking into making the same change (as
this patch) in the avocado tests, which of course all fail
without-default-devices. There it's considerably simpler (because
Python), but I'm still thinking about how to avoid hiding a legitimate
failure.

> So I think I rather tend to go for explicit calls to qtest_has_device() as 
> you did in your first 11 patches.

Ok, I'll send just them for v2, unless anyone else has something to say.