Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[John Snow <jsnow@redhat.com>]

On 08/06/2014 07:30 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
>>> This patch series introduces a number of small fixes and tweaks to
>>> help support an AHCI test suite that in the future I hope to expand
>>> to a fuller regression suite to help guide the development of the
>>> AHCI device support under, in particular, the Q35 machine type in QEMU.
>>>
>>> Paolo Bonzini has contributed a number of cleanup and refactoring patches
>>> that support changes to the PIO setup FIS packet construction code, which
>>> is necessary for testing ths specification adherence of the IDENTIFY command,
>>> which issues its data exclusively via PIO mechanisms.
>>>
>>> The ahci-test code being checked in represents a minimum of functionality
>>> needed in order to issue and receive commands from the AHCI HBA under the
>>> libqos / qtest environment.
>>>
>>> In V2, as detailed below, these tests are not currently expected to pass.
>>> I will post a complementary patch outside of this set that highlights
>>> the exact set of tests that will not pass, which can help verify at least
>>> the portions of these tests that do work correctly.
>>>
>>> Assertions that currently fail:
>>>      - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>>>      - Boot-time values of the PxTFD register, which should not have valid
>>>        data until after a D2H FIS is received, but does in Qemu 2.1
>>>      - Boot-time values of the PxSIG register, which should have a specific
>>>        placeholder signature until the first D2H FIS is received, but is
>>>        currently blank.
>>>      - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>>>        command exhausts the given PRDT, but is not seen.
>>
>> I guess these are the assertion failures:
>> ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed
>> ((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
>> GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
>> **
>> ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed
>> ((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
>> GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
>> **
>> ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed
>> ((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
>> GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d
>>
>> Why publish this patch series if the test fails?  We can't merge failing
>> tests.
>
> Correct.
>
> What I do when I want to start some bug fixing work with tests is to
> write the tests to expect the actual, incorrect behavior, with a
> greppable comment documenting the correct behavior.  Then clean that up
> as the bugs get fixed.
>

I thought it was valid to submit a failing test if... well, the behavior 
was wrong. Stefan said no warnings, so I took that to mean "This should 
fail." I didn't think it was too strange to have a failing test for 
something that was not feature complete.

So, if it's not appropriate to have a failing test at any stage 
(Regressions only?) now's a good time to let me know how you would like 
me to accomplish no warnings but have the tests pass. In my V1 I did 
just print a "WARN" string which was reasonable greppable to find the 
failure cases.

My next guess at something workable would be to stick the assertions 
behind a bool that could be toggled on/off via a flag that could be 
toggled with --all or similar to hit the expected failure cases. No 
warnings inside of the test harness, no failures, and cases could be 
found by grepping the name of the boolean and/or some accompanying comment.

--j