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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 08/06/2014 07:30 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <address@hidden> 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.

I reiterate my advice to simply assert the actual behavior, even though
it's incorrect, with a greppable comment documenting the correct
behavior.  No printing of warnings.  No flags to toggle assertions.

To illustrate what I mean: commit e2ec3f9.  It fixes a bunch of bugs,
and as a side effect replaces some incorrect behavior by other incorrect
behavior.  This is all visible in the patch to tests/.