Re: [RFA_v3 8/8] Add a self-test for cli-utils.c

[Pedro Alves <palves@redhat.com>]

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  <philippe.waroquiers@skynet.be>
> 
> 	* 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 <http://www.gnu.org/licenses/>.  */
> +
> +#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