Patchwork [RFAv2,2/2] Add a selftest that checks documentation invariants.

login
register
mail settings
Submitter Philippe Waroquiers
Date Aug. 3, 2019, 1:39 p.m.
Message ID <20190803133921.20154-3-philippe.waroquiers@skynet.be>
Download mbox | patch
Permalink /patch/33948/
State New
Headers show

Comments

Philippe Waroquiers - Aug. 3, 2019, 1:39 p.m.
Several approaches were discussed (mail or irc) to verify the invariants of
the GDB help documentation : checking with apropos ., modifying add_cmd
to do the check and output a warning, implement maintenance check-doc.

A selftest was finally chosen as:
  * this can be run on demand, including by users if they want
    to check user defined commands.
  * it does not interact with the normal behaviour of apropos, define,
    python, ...
    (such as output warnings when a user defines a command help that
     does not respect the doc).
  * when the selftest runs, it checks the user defined and python
    defined commands currently defined.

gdb/ChangeLog
	* unittests/help-doc-selftests.c: New file.
	* Makefile.in: Add the new file.
---
 gdb/Makefile.in                    |   1 +
 gdb/unittests/help-doc-selftests.c | 106 +++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)
 create mode 100644 gdb/unittests/help-doc-selftests.c
Tom Tromey - Aug. 6, 2019, 6:33 p.m.
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> gdb/ChangeLog
Philippe> 	* unittests/help-doc-selftests.c: New file.
Philippe> 	* Makefile.in: Add the new file.

Thanks for doing this.

Philippe> +static void
Philippe> +broken_doc_invariant (const char *prefix, const char *name, const char *msg)
Philippe> +{
Philippe> +  fprintf_filtered (gdb_stdout,
Philippe> +		    "help doc broken invariant: command '%s%s' help doc %s\n",
Philippe> +		    prefix, name, msg);

Normally I'd probably complain about being i18n-unfriendly here, but TBH
I don't think that matters much for unit tests.

Philippe> +  /* Walk through the commands.  */
Philippe> +  for (c=commandlist;c;c=c->next)

This needs some spaces.

Philippe> +      while (*p && *p != '\n')
Philippe> +	p++;

I think this could just be "p = strchr (p, '\n')".

This is ok with those things fixed.

Tom
Philippe Waroquiers - Aug. 6, 2019, 10:21 p.m.
On Tue, 2019-08-06 at 12:33 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> gdb/ChangeLog
> Philippe> 	* unittests/help-doc-selftests.c: New file.
> Philippe> 	* Makefile.in: Add the new file.
> 
> Thanks for doing this.
> 
> Philippe> +static void
> Philippe> +broken_doc_invariant (const char *prefix, const char *name, const char *msg)
> Philippe> +{
> Philippe> +  fprintf_filtered (gdb_stdout,
> Philippe> +		    "help doc broken invariant: command '%s%s' help doc %s\n",
> Philippe> +		    prefix, name, msg);
> 
> Normally I'd probably complain about being i18n-unfriendly here, but TBH
> I don't think that matters much for unit tests.
> 
> Philippe> +  /* Walk through the commands.  */
> Philippe> +  for (c=commandlist;c;c=c->next)
fixed.

> 
> This needs some spaces.
> 
> Philippe> +      while (*p && *p != '\n')
> Philippe> +	p++;
> 
> I think this could just be "p = strchr (p, '\n')".
I have kept the code above, as strchr returns NULL if no \n
is found, but it is normal to have a doc with just the
first line (not terminated by a LF).
In this case, we must still check that we have a . before
the terminating null byte.

I have put the following comment to clarify:
      /* Position p on the first LF, or on terminating null byte.  */
      while (*p && *p != '\n')
	p++;

> 
> This is ok with those things fixed.
I have pushed after having fixed the missing spaces and added
the comment.

Thanks for the reviews

Philippe
Tom Tromey - Aug. 7, 2019, 2:05 a.m.
Philippe> I have kept the code above, as strchr returns NULL if no \n
Philippe> is found, but it is normal to have a doc with just the
Philippe> first line (not terminated by a LF).
Philippe> In this case, we must still check that we have a . before
Philippe> the terminating null byte.

Thanks, that makes sense.

Tom

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 01ff0c0c20..b689769e13 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -414,6 +414,7 @@  SUBDIR_UNITTESTS_SRCS = \
 	unittests/environ-selftests.c \
 	unittests/format_pieces-selftests.c \
 	unittests/function-view-selftests.c \
+	unittests/help-doc-selftests.c \
 	unittests/lookup_name_info-selftests.c \
 	unittests/memory-map-selftests.c \
 	unittests/memrange-selftests.c \
diff --git a/gdb/unittests/help-doc-selftests.c b/gdb/unittests/help-doc-selftests.c
new file mode 100644
index 0000000000..8335ad581e
--- /dev/null
+++ b/gdb/unittests/help-doc-selftests.c
@@ -0,0 +1,106 @@ 
+/* Self tests for help doc for GDB, the GNU debugger.
+
+   Copyright (C) 2019 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-cmds.h"
+#include "cli/cli-decode.h"
+#include "gdbsupport/selftest.h"
+
+namespace selftests {
+namespace help_doc_tests {
+
+static unsigned int nr_failed_invariants;
+
+/* Report a broken invariant and increments nr_failed_invariants.  */
+
+static void
+broken_doc_invariant (const char *prefix, const char *name, const char *msg)
+{
+  fprintf_filtered (gdb_stdout,
+		    "help doc broken invariant: command '%s%s' help doc %s\n",
+		    prefix, name, msg);
+  nr_failed_invariants++;
+}
+
+/* Recursively walk the commandlist structures, and check doc invariants:
+   - The first line of the doc must end with a '.'.
+   - the doc must not end with a new line.
+  If an invariant is not respected, produce a message and increment
+  nr_failed_invariants.
+  Note that we do not call SELF_CHECK in this function, as we want
+  all commands to be checked before making the test fail.  */
+
+static void
+check_doc (struct cmd_list_element *commandlist, const char *prefix)
+{
+  struct cmd_list_element *c;
+
+  /* Walk through the commands.  */
+  for (c=commandlist;c;c=c->next)
+    {
+      /* Checks the doc has a first line terminated with a '.'.  */
+      const char *p = c->doc;
+
+      while (*p && *p != '\n')
+	p++;
+      if (p == c->doc)
+	broken_doc_invariant
+	  (prefix, c->name,
+	   "is missing the first line terminated with a '.' character");
+      else if (*(p-1) != '.')
+	broken_doc_invariant
+	  (prefix, c->name,
+	   "first line is not terminated with a '.' character");
+
+      /* Checks the doc is not terminated with a new line.  */
+      if (c->doc[strlen (c->doc) - 1] == '\n')
+	broken_doc_invariant
+	  (prefix, c->name,
+	   "has a superfluous trailing end of line");
+
+      /* Check if this command has subcommands and is not an
+	 abbreviation.  We skip checking subcommands of abbreviations
+	 in order to avoid duplicates in the output.  */
+      if (c->prefixlist != NULL && !c->abbrev_flag)
+	{
+	  /* Recursively call ourselves on the subcommand list,
+	     passing the right prefix in.  */
+	  check_doc (*c->prefixlist, c->prefixname);
+	}
+    }
+}
+
+static void
+help_doc_invariants_tests ()
+{
+  nr_failed_invariants = 0;
+  check_doc (cmdlist, "");
+  SELF_CHECK (nr_failed_invariants == 0);
+}
+
+} /* namespace help_doc_tests */
+} /* namespace selftests */
+
+void
+_initialize_help_doc_selftests ()
+{
+  selftests::register_test
+    ("help_doc_invariants",
+     selftests::help_doc_tests::help_doc_invariants_tests);
+}