[libvirt] [PATCH 2/9] tests: qemuxml2xml: Always use different output file

Mon Feb 8 15:27:41 UTC 2016

On Fri, Feb 05, 2016 at 03:37:59PM -0500, Cole Robinson wrote:
>On 02/05/2016 02:24 PM, Laine Stump wrote:
>> On 01/28/2016 03:30 PM, Cole Robinson wrote:
>>> Most qemuxml2xml tests expect that the input XML is unchanged after
>>> parsing. This is unlike 99% of new qemu configs in the wild, which after
>>> initial parsing end up with stable PCI device addresses. The xml2xml bit
>>> doesn't currently hit that code path though, so most XML testing indeed
>>> does not change.
>>>
>>> Future patches will add that PCI address bits, which means most test cases
>>> will have different output. So let's do away with the hardcoded same vs
>>> different test split, and always track a separate output file. Tests can
>>> still have same input and output, it just necessitates 2 separate XML files.
>>> ---
>>>   .../qemuxml2xmlout-aarch64-aavmf-virtio-mmio.xml   |  49 +++++
>>
>> If there's going to be a ton of new files added anyway, this is a good chance
>> to shorten the names of all the files (and avoid needing to do it to so many
>> more files later). I've always thought it was pointless to have a file called
>>
>>    qemuxml2xmloutdata/qemuxml2xmlout-blah.xml
>>
>> why not just:
>>
>>    qemuxml2xmloutdata/blah.xml
>>
>> ?? I recall there being problems with running make rpm (or maybe make
>> distcheck?) because some filename in one of these directories was beyond the
>> tarfile name length limit or something like that.
>>
>
>Yes I agree. But I don't want to get bogged down in this discussion. After
>these patches are resolved I'll start a thread to discuss it, since we should
>just do it for all test cases in one go IMO.
>

Once again, sorry for bootstrapping the bikeshedding discussion.

>> I went back and read Martin's opinion about adding all these files, and your
>> response. It's true that we aren't *explicitly* testing the PCI address
>> assignment directly in the xml2xml tests now. It does end up being tested in
>> the xml2argvtests though (except for devices that are at fixed PCI addresses
>> which don't show up on the qemu commandline, e.g.  builtin IDE controller in
>> 440fx or builtin SATA on q35). Of course it would be a lot easier to see
>> what's going on if the XML files could be compared, rather than trying to
>> compare qemu commandlines and backtracking to find the offending device in the
>> XML :-)
>>
>> Do you know what percentage of these files end up being different between
>> source and result after all your changes are done? If it's all/most of them,
>> then I think it would be a lot of extra effort for no gain to setup symbolic
>> links now only to have them broken a few commits later.
>>
>
>Patch 8 touches 283 xml2xmlout files. There's 290 total. Basically any test
>case with a device is going to be affected.
>
>> (A bit of thinking out loud follows...)
>>
>> Anyway, even once we add the PCI address assignment into the qemuxml2xmltest,
>> we still may have a considerable number of tests that have the PCI address in
>> the source xml already anyway. Also, I think too many of the tests have been
>> created with the following formula:
>>
>> 1) copy testfiles for some random existing test
>> 2) add on a line or two that tests the new feature
>>
>> This has the upside of testing combinations of items that might not otherwise
>> be tested, but there's nothing methodical about it, and we're ending up
>> testing the same bits hundreds of times in exactly the same way. *AND* (the
>> most important) if something is purposefully changed in one of those bits that
>> is unnecessarily copied to a couple hundred tests (e.g. the output now has a
>> PCI address in the element), then that change must be accompanied by changes
>> to hundreds of test output files.
>>
>> It might be useful to eliminate a bunch of this duplication of tests. But of
>> course that would be *very* tedious, and the potential for accidentally
>> removing a useful and unique combination would be high (especially if we fell
>> to the temptation of letting a newcomer do such a trimming as a "starter"
>> project).
>>
>
>Completely agreed, I alluded to this elsewhere. The test suite has massive
>redundancy.
>

Yes.  I have also few ideas related to parsing and different phases in
command line creation that could use a good dose of laxative and then
few proper rubs here and there.  However I, personally, don't fancy
those bookkeeping jobs, so I'm missing a motivation right now with all
the other things going on.

>It's not hard to track what tests are actually hitting though, using gcov for
>tracking code coverage. I just don't think anyone is really checking up on it.
>This is something I'd like to look at and better document in the future, I
>think test suite improvements could be a good source of bitesize/easyfix type
>tasks.
>
>> (anyway...)
>>
>> It would be nice if we could move all the tests that are there merely for
>> checking genericxml2xml over to that test in advance of this change in order
>> to reduce the churn, but I understand your reluctance to do that - it will
>> involve a lot more subjective decisions about what was the purpose of various
>> test cases (and for some of those the only clue you may have is the name of
>> the data file), so it seems like a large annoying bookkeeping job when what
>> you really need is to make ARM work correctly :-)
>>
>
>FWIW I like large annoying bookkeeping jobs :) But yeah I need to get 'Real
>Work' done in the meantime. As you've pointed out there's lots of areas we can
>improve in the test suite I'm trying to not get fully sucked down the rabbit
>hole just yet. I do want to come back to it though
>
>> Since we already have hundreds of test files, and this seems to be moving in
>> the right direction (although taking a nasty side-step), I'm inclined to ACK
>> this patch. We should maybe put it to some kind of informal vote, though, just
>> to make sure everyone's concerns are properly addressed (and maybe someone
>> will come up with a bright idea)
>
>Thanks, I agree another ACK is a good idea.
>

I didn't mean to stall this series.  I'm fine with this going in,
especially if we clean it up afterwards.

So if you also copy the missing files, of course (current list of test
names for easier work and checking provided below), I can safely say ACK
here.

Martin

disk-cdrom-empty
disk-drive-boot-cdrom
disk-drive-boot-disk
disk-drive-cache-directsync
disk-drive-cache-unsafe
disk-drive-cache-v2-none
disk-drive-cache-v2-wb
disk-drive-cache-v2-wt
disk-drive-error-policy-enospace
disk-drive-error-policy-stop
disk-drive-error-policy-wreport-rignore
disk-drive-network-gluster
disk-drive-network-rbd-auth
disk-drive-network-rbd-ceph-env
disk-drive-network-rbd-ipv6
disk-drive-network-rbd
disk-drive-network-sheepdog
migrate
misc-uuid
nographics-vga
qemu-ns-no-env
restore-v2
watchdog
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160208/831865d4/attachment-0001.sig>