Re: [PATCH] tests: improve performance of device-introspect-test

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> On 09/07/2020 13:59, Daniel P. Berrangé wrote:
>> On Thu, Jul 09, 2020 at 01:44:45PM +0200, Laurent Vivier wrote:
>>> On 09/07/2020 13:28, Daniel P. Berrangé wrote:
>>>> Total execution time with "-m slow" and x86_64 QEMU, drops from 3
>>>> minutes 15 seconds, down to 54 seconds.
>>>>
>>>> Individual tests drop from 17-20 seconds, down to 3-4 seconds.
>>>>
>>>> The cost of this change is that any QOM bugs resulting in the test
>>>> failure will not be directly associated with the device that caused
>>>> the failure. The test case is not frequently identifying such bugs
>>>> though, and the cause is likely easily visible in the patch series
>>>> that causes the failure. So overall the shorter running time is
>>>> considered the more important factor.
>>>
>>> You don't report the test to test_device_intro_none() and
>>> test_device_intro_abstract(): is it intended ?
>> 
>> Since neither of those tests will result in any device being created there
>> didn't seem any point in chceking the qtree output.
>> 
>> IIUC, both of those tests should result in an error being reported from
>> the device_add command, but I see nothing actually validates that is the
>> case. 
>
> I think the purpose of these tests is precisely to ensure nothing is
> created. This is why they check the qtree and not the reported error.
>
> Markus?

Before I answer the question, let me provide a bit of background.

The tests we're discussing exercise QMP command device-list-properties.
We're trying to catch two kinds of bugs:

* device introspection crashes or hangs outright, and

* device introspection has unwelcome side effects.

We're vulnerable to such bugs since device introspection has to create
and destroy an instance (consequence of QOM's design), and the device
code that gets run on creation and destruction can do anything.

To catch crashs or hangs, we execute one introspection, throwing away
its result.

Catching general side effects is impossible, so we instead catch special
side effects that (a) have a history of actual bugs, and (b) are easy to
observe: changes to the qdev tree visible in info qtree, and changes to
the QOM composition tree visible in info qom-tree.

Now I'm ready to answer the question.

test_device_intro_none() covers introspection of nonexistent device type
"nonexistent".  device-list-properties is expected to fail without
creating a device instance, let alone run device code.

test_device_intro_abstract() covers introspection of abstract device
type "abstract".  device-list-properties is again expected to fail
without creating a device instance.

The test neglects to cover "type exists, but is not a device" (my
fault).  Same thing.

In all these cases, side effects on "info qtree" and "info qom-tree" in
the (tiny amount of) generic code they run are theoretically possible,
but vanishingly unlikely.  If generic code ever gets confused enough to
somehow create an instance, all bets are off, of course.

Daniel's patch loses us some insurance against this kind of confusion.
Do we care?

I guess the insurance is nice to have when it costs basically nothing.