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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4343 bytes --]

On Wed, Aug 06, 2014 at 12:50:23PM -0400, John Snow wrote:
> 
> 
> 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.

Note for the mailing list: John and I had an IRC chat where I mentioned
that the git history must always be bisectable, which is the fundamental
reason why failing tests cannot be merged.  My suggestion is to
post-pone tests that are not fixed by this patch series, and I like
Markus idea to assert the incorrect behavior (with a comment) in the
meantime, too.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]