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

[Leandro Dorileo]

Hi Eric,

On Fri, Mar 21, 2014 at 08:37:40AM -0600, Eric Blake wrote:
> 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 <address@hidden>
> 
> 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.

I would say that I even know about the --- marker, but have misplaced it... :(

> 
> >  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:

Ok...

> 
> Reviewed-by: Eric Blake <address@hidden>


Thanks for reviewing...

-- 
Leandro Dorileo