Re: [Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Andreas Färber]

Am 08.04.2015 um 15:42 schrieb Eduardo Habkost:
> (CCing Jiri Denemark in case he has anything to add about libvirt
> requirements)
> 
> On Wed, Apr 08, 2015 at 03:06:41PM +0200, Andreas Färber wrote:
>> Am 23.03.2015 um 20:37 schrieb Eduardo Habkost:
>>> Changes v4 -> v5:
>>>  * Rewrite fake kvm_arch_get_supported_cpuid() code
>>>  * Add new test to ensure KVM defaults won't override explicit command-line
>>>    options
>>>
>>> Changes v3 -> v4:
>>>  * Move target_words_bigendian() prototype back inside tests/x86-stub.c.
>>>
>>> Changes v2 -> v3:
>>>  * Extra KVM "host" CPU model test cases
>>>  * Move target_words_bigendian() prototype to exec-all.h
>>>
>>> Changes v1 -> v2:
>>>  * Make dependency list of test binary much simpler, now that cpus.o
>>>    was removed.
>>
>> I don't recall seeing the previous four versions of this... I think
>> adding unit tests for whole devices is the wrong approach. We had that
>> discussion for tmp105. Instead, using the QTest framework - be it
>> extending my pc-cpu-test or adding a new one - allows to reuse the
>> device infrastructure in-place in the actual executable without the need
>> for stubs and without risking to break the test suite by forgetting to
>> run make check after device changes or forcing work duplication onto others.
> 
> The problem with using the actual executable is that it requires
> defining the external interfaces before writing tests, and the whole
> point of the unit test is to be allow us to check if we are not doing
> anything wrong _while implementing_ those external interfaces. This was
> mentioned before, see:
>   Message-ID: <address@hidden>
>   http://article.gmane.org/gmane.comp.emulators.qemu/299411
> 
> Also, I don't even expect external interfaces to be available for some
> hooks that this test code require (e.g. changing the return value of
> kvm_arch_get_supported_cpuid()).
> 
> Personally, I want (and will) run this test after every change made on
> target-i386, even if the series is not accepted, but adding the test
> code to qemu.git would make life easier for everyone. Having to create a
> qemu-x86-unit-tests.git repository for it (even if temporarily) would be
> unfortunate.

Having code in qemu.git and having it in make check are two separate
things though, and my problem is with the latter.

> I don't understand the "forcing work duplication onto others" part.
> What kind of work duplication is being forced onto others?

Changes to random device etc. infrastructure can break your x86-specific
test case. If people don't expect such cross-subsystem effects then in
the worst case maintainers (me, mst, bonzini, armbru, ...) end up
sending a pull request to Peter, who then needs to reject it based on a
breaking x86 test case. When I work on PReP, I often just run make
check-qtest-ppc because the full suite takes much too long, and I expect
unit tests to be fine-granular like testing only an APIC ID API but not
the whole device infrastructure.

So please don't just think about your own requirements, think of the
consequences this design has for others. If tests don't get usage
because they are not run, that doesn't really help. For QTest we have
gcov for evaluating which APIs are covered, if you start from zero with
your own private framework now, you don't get that and we'll end up
having less ideal coverage in two separate places rather than increasing
coverage in the QTest framework we already have.

>> Specifically, you added properties to allow inspection of CPUID feature
>> bits that - as I understood - today no one (including libvirt) is using.
> 
> Not sure why this is related to the test code, [...]

Because that is *your* existing external interface that you could use to
implement your tests and thereby even give that interface some actual
testing and usage beyond my generic qom-test.

I don't care about libvirt requirements here, this is all about QEMU.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)