From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2921 invoked by alias); 9 Jul 2018 19:27:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 2912 invoked by uid 89); 9 Jul 2018 19:27:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=whoever, sk:environ X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Jul 2018 19:27:42 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C83A48163C5F; Mon, 9 Jul 2018 19:27:40 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3AF017C4B; Mon, 9 Jul 2018 19:27:40 +0000 (UTC) Subject: Re: [RFA_v3 8/8] Add a self-test for cli-utils.c To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20180624183708.888-1-philippe.waroquiers@skynet.be> <20180624183708.888-9-philippe.waroquiers@skynet.be> From: Pedro Alves Message-ID: <1c35216b-2124-7c53-226e-d96da6170ea8@redhat.com> Date: Mon, 09 Jul 2018 19:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180624183708.888-9-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-07/txt/msg00226.txt.bz2 On 06/24/2018 07:37 PM, Philippe Waroquiers wrote: > tests added for: > * number_or_range_parser > In particular, it tests the cur_tok when parsing is finished. > > * parse_flags > > * parse_flags_qcs Thanks much for adding unit tests. That's really great. A small request below, but this looks good to me otherwise. > > gdb/ChangeLog > 2018-06-05 Philippe Waroquiers > > * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add > unittests/cli-utils-selftests.c > * unittests/cli-utils-selftests.c: New file. > --- > gdb/Makefile.in | 1 + > gdb/unittests/cli-utils-selftests.c | 220 ++++++++++++++++++++++++++++ > 2 files changed, 221 insertions(+) > create mode 100644 gdb/unittests/cli-utils-selftests.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 354a6361b7..f8cdf9a560 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -416,6 +416,7 @@ SUBDIR_PYTHON_CFLAGS = > > SUBDIR_UNITTESTS_SRCS = \ > unittests/array-view-selftests.c \ > + unittests/cli-utils-selftests.c \ > unittests/common-utils-selftests.c \ > unittests/environ-selftests.c \ > unittests/format_pieces-selftests.c \ > diff --git a/gdb/unittests/cli-utils-selftests.c b/gdb/unittests/cli-utils-selftests.c > new file mode 100644 > index 0000000000..99414f097e > --- /dev/null > +++ b/gdb/unittests/cli-utils-selftests.c > @@ -0,0 +1,220 @@ > +/* Unit tests for the cli-utils.c file. > + > + Copyright (C) 2018 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "defs.h" > +#include "cli/cli-utils.h" > +#include "selftest.h" > + > +namespace selftests { > +namespace cli_utils { > + > +static void > +test_number_or_range_parser () > +{ > + number_or_range_parser one ("1"); > + > + SELF_CHECK (one.finished () == false); > + SELF_CHECK (one.get_number () == 1); > + SELF_CHECK (one.finished () == true); Curious that you write "== false", "== true" throughout instead of: SELF_CHECK (!one.finished ()); SELF_CHECK (one.finished ()); I don't mind here, it just stood out. > + SELF_CHECK (strcmp (one.cur_tok (), "") == 0); > + > + number_or_range_parser one_after ("1 after"); > + > + SELF_CHECK (one_after.finished () == false); > + SELF_CHECK (one_after.get_number () == 1); > + SELF_CHECK (one_after.finished () == true); > + SELF_CHECK (strcmp (one_after.cur_tok (), "after") == 0); > + > + number_or_range_parser one_three ("1-3"); > + > + for (int i = 1; i < 4; i++) { "{" goes on next line. Appears in several places. > +static void > +test_parse_flags () > +{ > + const char *flags = "abc"; > + const char *non_flags_args = "non flags args"; > + int res; > + > + const char *t1 = "-a -a non flags args"; > + > + SELF_CHECK (parse_flags (&t1, flags) == 1); > + SELF_CHECK (parse_flags (&t1, flags) == 1); > + SELF_CHECK (strcmp (t1, non_flags_args) == 0); > + > + const char *t2 = "-c -b -c -b -c non flags args"; > + > + SELF_CHECK (parse_flags (&t2, flags) == 3); > + SELF_CHECK (parse_flags (&t2, flags) == 2); > + SELF_CHECK (parse_flags (&t2, flags) == 3); > + SELF_CHECK (parse_flags (&t2, flags) == 2); > + SELF_CHECK (parse_flags (&t2, flags) == 3); > + SELF_CHECK (strcmp (t2, non_flags_args) == 0); > + > + const char *t3 = non_flags_args; > + > + SELF_CHECK (parse_flags (&t3, flags) == 0); > + SELF_CHECK (strcmp (t3, non_flags_args) == 0); > + > + const char *t4 = "-c -b -x -y -z -c"; > + const char *orig_t4 = t4; > + orig_t4 appears unused. > + SELF_CHECK (parse_flags (&t4, flags) == 3); > + SELF_CHECK (parse_flags (&t4, flags) == 2); > + SELF_CHECK (strcmp (t4, "-x -y -z -c") == 0); > + > + const char *t5 = "-c -cb -c"; > + const char *orig_t5 = t5; orig_t5 appears unused. > + > + SELF_CHECK (parse_flags (&t5, flags) == 3); > + SELF_CHECK (parse_flags (&t5, flags) == 0); > + SELF_CHECK (strcmp (t5, "-cb -c") == 0); Could you add short single-line comments indicating what the different parts of the function are testing? I.e., some guidance into what detail the t1 section is testing, what detail the t2 section is testing, etc. I think that likely helps whoever needs to extend the testcase in future. I'd suggest wrapping such sections of these functions in their own scope, which both helps visually distinguish the sections, and avoids variable spillage between the sections too. See e.g., run_tests in array-view-selftests.c. Thanks, Pedro Alves