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

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 04:05:59PM +0200, Andreas Färber wrote:
> 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.

Understood, so let's understand the problem and try to solve it. If we
can't solve it by now, I will be happy to leave it out of "make check".

> 
> > 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.

I want to understand how exactly this could happen. Is this just about
the stubs, or are there additional problems I am not seeing?

> 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.

This looks like a general problem with all unit tests for C files that
have too many dependencies?

Note that I do not want to test the whole device infrastructure: I just
want to test the feature flag initialization and filtering code that is
inside target-i386/cpu.c. And the most important part is: I want to do
it before making changes to that code (so having to heavily refactor the
code before writing the tests is not an option for me).

So, my current problem is: I want to be able to write unit tests for
functions that are currently inside target-i386/cpu.c without the
problems you mentioned. Is there a way to do that?

> 
> 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.

I am all for increasing QTest coverage when possible, I don't want a
private framework. Right now I just want to write unit tests that check
how a (preferably small) chunk of code behaves, before starting making
changes to implement additional external interfaces.

> 
> >> 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 understand that part of your argument. The existing interface is
necessary but not sufficient for what I want to test. See, for example,
my comment about kvm_arch_get_supported_cpuid() above.

-- 
Eduardo