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

[Leandro Dorileo <l@dorileo.org>]

On Mon, Mar 17, 2014 at 10:16:12AM -0600, Eric Blake wrote:
> On 03/16/2014 03:18 PM, Leandro Dorileo wrote:
> > Cover basic aspects and API usage for QemuOpt. The current implementation
> > covers the API's planed to be changed by Chunyan Liu in his QEMUOptionParameter
> 
> s/planed/planned/
> 
> > replacement/cleanup job.
> > 
> > Other APIs should be covered in future improvements.
> > 
> > Signed-off-by: Leandro Dorileo <l@dorileo.org>
> > ---
> >  tests/Makefile         |   3 +
> >  tests/test-qemu-opts.c | 309 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 312 insertions(+)
> >  create mode 100644 tests/test-qemu-opts.c
> > 
> 
> > +
> > +static void test_find_unknown_opts(void)
> > +{
> > +    QemuOptsList *list;
> > +
> > +    register_opts();
> > +
> > +    /** should not return anything, we don't known "unknown" option */
> 
> s/known/have an/

ok.

> 
> Here, and throughout the file: /** */ comments are special to doc
> markup, and should only be needed prior to declaring a function.  Within
> a function body, use the simpler /* */ comment.
> 
> > +static void test_qemu_find_opts(void)
> > +{
> > +    QemuOptsList *list;
> > +
> > +    register_opts();
> > +
> > +    /** we have an rtc option, should return it */
> > +    list = qemu_find_opts("opts_list_01");
> > +    g_assert(list != NULL);
> 
> Should you do any further testing on the contents of the returned list?

ok. I'll see the API.

> 
> > +static void test_qemu_opt_get(void)
> > +{
> 
> > +    /** now we have set str2, should know about it */
> > +    opt = qemu_opt_get(opts, "str2");
> > +    g_assert(opt != NULL && !strcmp(opt, "value"));
> 
> Isn't g_assert_cmpstr nicer for this, as in:
> 
> g_assert_cmpstr(opt, ==, "value");


Yep, sure, I wasn't awere of g_assert_cmpstr, thanks.

> 
> > +static void test_qemu_opt_get_size(void)
> > +{
> > +    QemuOptsList *list;
> 
> > +
> > +    qemu_opts_absorb_qdict(opts, dict, &local_err);
> > +    g_assert(local_err == NULL);
> 
> shorter as:
> 
> qemu_opts_absorb_qdict(opts, dict, &error_abort);

I see.

> 
> Overall, I like where this is headed; looking forward to v2.
> 

Thanks for reviewing...

-- 
Leandro Dorileo