From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR0t2-0007gS-05 for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:57:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WR0sx-0004Sy-3a for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:57:31 -0400 Received: from mail-oa0-f49.google.com ([209.85.219.49]:46990) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR0sw-0004So-VT for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:57:27 -0400 Received: by mail-oa0-f49.google.com with SMTP id h16so2595710oag.8 for ; Fri, 21 Mar 2014 07:57:26 -0700 (PDT) Date: Fri, 21 Mar 2014 14:56:11 +0000 From: Leandro Dorileo Message-ID: <20140321145611.GD22259@dorilex> References: <1395097833-3021-1-git-send-email-l@dorileo.org> <532C4EB4.6030506@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <532C4EB4.6030506@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] QemuOpt: add unit tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Wenchao Xia , Stefan Hajnoczi , Markus Armbruster , qemu-devel@nongnu.org, Anthony Liguori , Chunyan Liu , Andreas =?iso-8859-1?Q?F=E4rber?= 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 > > 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 Thanks for reviewing... -- Leandro Dorileo