Re: [PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Thomas Huth <thuth@redhat.com>]

On 22/03/2021 18.48, Lukas Straub wrote:
> On Mon, 22 Mar 2021 17:00:23 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 22/03/2021 08.35, Lukas Straub wrote:
>>> On Mon, 22 Mar 2021 06:20:50 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>    
>>>> On 22/03/2021 00.31, Lukas Straub wrote:
>>>>> Use the normal yank code instead of stubs in relevant tests to
>>>>> increase coverage and to ensure that registering and unregistering
>>>>> of yank instances and functions is done correctly.
>>>>>
>>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>>>>> ---
>>>>>     tests/qtest/meson.build | 6 +++---
>>>>>     tests/unit/meson.build  | 4 ++--
>>>>>     2 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>>> index 66ee9fbf45..40e1f495f7 100644
>>>>> --- a/tests/qtest/meson.build
>>>>> +++ b/tests/qtest/meson.build
>>>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
>>>>>     qtests = {
>>>>>       'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>>>>>       'cdrom-test': files('boot-sector.c'),
>>>>> -  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
>>>>> +  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'],
>>>>>       'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>>>>> -  'migration-test': files('migration-helpers.c'),
>>>>> +  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
>>>>>       'pxe-test': files('boot-sector.c'),
>>>>>       'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
>>>>>       'tpm-crb-swtpm-test': [io, tpmemu_files],
>>>>
>>>> Is this really necessary for the qtests? I can understand the change for the
>>>> unit tests, but the qtests are separate programs where I could not imagine
>>>> that they use the yank functions in any way?
>>>
>>> Yes, it is necessary. While the yank functions are not called in these tests,
>>> it still checks that registering and unregistering of yank instances and
>>> functions is done correctly. I.e. That no yank functions are registered before
>>> the instance, that the yank instance is only unregistered after all functions
>>> where unregistered, that the same instance is not registered twice and that
>>> the yank instance actually exists before it is unregistered.
>>
>> Now you even confused me more. Could you elaborate a little bit? If none of
>> the functions are called by the test, which part of yank.c is excercised
>> here at all? Could you give a more detailed example? The only thing I could
>> imagine is yank_init(), but that does not look like something we need to
>> check in a qtest ?
> 
> Oh, sorry. I meant yank's concept of a yank function here. It works this way:
> The different subsystems first register a yank instance. So in this case
> when starting migration in the test, the migration code first registers a
> yank instance. Then, it registers _yank functions_ with this instance, for
> for example to shutdown a socket.

But these are the qtest, separate stand-alone programs. The migration code 
of QEMU (i.e. the code in the main "migration" folder) is not linked into 
these binaries. Doing something like:

  grep -r yank tests/qtest/migration-test

should give you zero results. Thus it IMHO does not make sense to add the 
yank.c to these tests here.

Having said that, it seems like the qos-test is linking against the chardev 
code and thus might use indirectly the yank code there. So you maybe might 
want to add it to the qos-test instead?

  Thomas