Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands

[Markus Armbruster <armbru@redhat.com>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/02/17 09:25, Laszlo Ersek wrote:
>
> Regarding your other email ("New QMP command without a test -> automatic
> NAK"), Ben did write a small test suite for this feature:
>
> [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation
>                             ID feature
>
> and it contains a test called "/vmgenid/vmgenid/query-monitor".
>
> That patch is not part of the pull request because the functional tests
> in the same suite only pass if you have an updated SeaBIOS blob in the
> tree as well. While the SeaBIOS patches have been committed, the update
> for the blob bundled with QEMU is still in progress. Thus the patch with
> the tests is being delayed.

Posting the testing part that depends on in-progress work only as RFC
sounds like a perfectly acceptable case of the "not practical" exception
mentioned "New QMP command without a test -> automatic NAK".

> So, it wasn't negligence, we simply missed this failure case because we
> were so focused on the long-awaited functionality. We didn't miss other
> failure cases that were a bit closer to the functionality.
>
> BTW, now that I look at said "/vmgenid/vmgenid/query-monitor" test case,
> it also doesn't seem to catch this error -- the monitor command is
> apparently not called without the device present.

I expect you to try to cover all QMP command error cases with automated
tests.  I don't expect you to always succeed.  We're all human :)

>                                                   So yeah, review
> focused specifically on QMP / QAPI bits is always welcome.

It's a struggle, to be honest.  Bits of QMP and QAPI are often buried
deep in patches dealing with other stuff.  QMP can usually be separated
into its own patch, but QAPI is just infrastructure, it *wants* to be
used.