Re: [Qemu-devel] [PATCH v2] QemuOpt: add unit tests

[Eric Blake <eblake@redhat.com>]

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

On 03/17/2014 05:10 PM, Leandro Dorileo wrote:
> Cover basic aspects and API usage for QemuOpt. The current implementation
> covers the API's planned to be changed by Chunyan Liu in his QEMUOptionParameter
> replacement/cleanup job.
> 
> Other APIs should be covered in future improvements.
> 
> Signed-off-by: Leandro Dorileo <l@dorileo.org>

Right here is where you should stick a --- marker.

> 
> Changes:
>   v2:
>      + fixed comments;
>      + make use of g_assert_cmpstr();
>      + use error_abort instead of a local_err for qemu_opts_absorb_qdict();
>      + asserts on QemuOptsList (empty and list name);
>      + added test_qemu_opt_unset();
>      + asserts on qemu_opt_*_set() return;
>      + added test_qemu_opts_reset();
>      + added test_qemu_opts_set();
> ---

It's okay to have a duplicate one; but the main point is that the v2
changelog is useful to reviewers but not to the git log; and anything
after the --- marker gets omitted by 'git am' when a maintainer accepts
your patch into their pull request.

>  tests/Makefile         |   3 +
>  tests/test-qemu-opts.c | 455 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 458 insertions(+)
>  create mode 100644 tests/test-qemu-opts.c
> 
> +static void test_qemu_find_opts(void)
> +{
> +    QemuOptsList *list;
> +
> +    register_opts();
> +
> +    /* we have an "opts_list_01" option, should return it */
> +    list = qemu_find_opts("opts_list_01");
> +    g_assert(list != NULL);
> +    g_assert_cmpstr(list->name, ==, "opts_list_01");
> +}
> +
> +static void test_qemu_opts_create(void)
> +{
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +
> +    register_opts();
> +
> +    list = qemu_find_opts("opts_list_01");
> +    g_assert(list != NULL);
> +    g_assert(QTAILQ_EMPTY(&list->head));
> +    g_assert_cmpstr(list->name, ==, "opts_list_01");

This test starts as a duplicate of test_qemu_find_opts; I guess that's
okay (having both tests means a bit more insight into what broke if one
fails but the other passes).

> +
> +static void test_qemu_opt_unset(void)
> +{
> +    QemuOpts *opts;
> +    const char *value;
> +    int ret;
> +
> +    /* dinamically initialized (parsed) opts */

s/dinamically/dynamically/

That's trivially fixable, so feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]