Re: [PATCH] gdb/testsuite: handle invalid .exp names passed in TESTS

[Bruno Larsen <blarsen@redhat.com>]

Hi Andrew,

Once again, very well timed patch as I had just noticed this problem! 
Other than a typo (response inlined), my only comment is that, since you 
are already renaming check-single, what do you think about renaming to 
check-single-thread or check-sequential?

My reasoning is that when first reading the message and diff, I thought 
this rule would test a single test, which left me confused for a minute.

Either way, I like the patch, and encourage you to approve it!

-- 
Cheers,
Bruno

On 10/09/2022 18:43, Andrew Burgess via Gdb-patches wrote:
> I ran some tests like:
>
>    $ make check-gdb TESTS="gdb.base/break.exp"
>
> then, then I went to rerun the tests later, I managed to corrupt the
> command line, like this:
>
>    $ make check-gdb TESTS="gdb.base/breakff.exp"
>
> the make command did exit with an error, but DejaGnu appeared to
> report that every test passed!  The tail end of the output looks like
> this:
>
>    Illegal Argument "no-matching-tests-found"
>    try "runtest --help" for option list
>    		=== gdb Summary ===
>
>    # of expected passes		115
>    /tmp/build/gdb/gdb version  13.0.50.20220831-git -nw -nx -iex "set height 0" -iex "set width 0" -data-directory /tmp/build/gdb/testsuite/../data-directory
>
>    make[3]: *** [Makefile:212: check-single] Error 1
>    make[3]: Leaving directory '/tmp/build/gdb/testsuite'
>    make[2]: *** [Makefile:161: check] Error 2
>    make[2]: Leaving directory '/tmp/build/gdb/testsuite'
>    make[1]: *** [Makefile:1916: check] Error 2
>    make[1]: Leaving directory '/tmp/build/gdb'
>    make: *** [Makefile:13565: check-gdb] Error 2
>
> For a while, I didn't spot that DejaGnu had failed at all, I saw the
> 115 passes, and thought everything had run correctly - though I was
> puzzled that make was reporting an error.
>
> What happens is that in gdb/testsuite/Makefile, in the check-single
> rule, we first run DejaGnu, then run the dg-add-core-file-count.sh
> script, and finally, we use sed to extract the results from the
> gdb.sum file.
>
> In my case, with the invalid test name, DejaGnu fails, but the
> following steps are still run, the final result, the 115 passes, is
> then extracted from the pre-existing gdb.sum file.
>
> If I use 'make -jN' then the 'check-parallel' rule, rather than the
> 'check-single' rule is used.  In this case the behaviour is slightly
> different, the tail end of the output now looks like this:
>
>    No matching tests found.
>
>    make[4]: Leaving directory '/tmp/build/gdb/testsuite'
>    find: ‘outputs’: No such file or directory
>    Usage: ../../../src/gdb/testsuite/../../contrib/dg-extract-results.py [-t tool] [-l variant-list] [-L] log-or-sum-file ...
>
>        tool           The tool (e.g. g++, libffi) for which to create a
>                       new test summary file.  If not specified then output
>                       is created for all tools.
>        variant-list   One or more test variant names.  If the list is
>                       not specified then one is constructed from all
>                       variants in the files for <tool>.
>        sum-file       A test summary file with the format of those
>                       created by runtest from DejaGnu.
>        If -L is used, merge *.log files instead of *.sum.  In this
>        mode the exact order of lines may not be preserved, just different
>        Running *.exp chunks should be in correct order.
>    find: ‘outputs’: No such file or directory
>    Usage: ../../../src/gdb/testsuite/../../contrib/dg-extract-results.py [-t tool] [-l variant-list] [-L] log-or-sum-file ...
>
>        tool           The tool (e.g. g++, libffi) for which to create a
>                       new test summary file.  If not specified then output
>                       is created for all tools.
>        variant-list   One or more test variant names.  If the list is
>                       not specified then one is constructed from all
>                       variants in the files for <tool>.
>        sum-file       A test summary file with the format of those
>                       created by runtest from DejaGnu.
>        If -L is used, merge *.log files instead of *.sum.  In this
>        mode the exact order of lines may not be preserved, just different
>        Running *.exp chunks should be in correct order.
>    make[3]: Leaving directory '/tmp/build/gdb/testsuite'
>    make[2]: Leaving directory '/tmp/build/gdb/testsuite'
>    make[1]: Leaving directory '/tmp/build/gdb'
>
> Rather than DejaGnu failing, we now get a nice 'No matching tests
> found' message, followed by some other noise.  This other noise is
> first `find` failing, followed by the dg-extract-results.py script
> failing.
>
> What happens here is that, in the check-parallel rule, the outputs
> directory is deleted before DejaGnu is invoked.  Then we try to run
> all the tests, and finally we use find and dg-extract-results.py to
> combine all the separate .sun
.sum*
> and .log files together.  However, if
> there are no tests run then the outputs/ directory is never created,
> so the find command and consequently the dg-extract-results.py script,
> fail.
>
> This commit aims to fix the following issues:
>
>   (1) For check-single and check-parallel rules, don't run any of the
>   post-processing steps if DejaGnu failed to run.  This will avoid all
>   the noise after the initial failure of DejaGnu,
>
>   (2) For check-single ensure that we don't accidentally report
>   previous results, this is related to the above, but is worth calling
>   out as a separate point, and
>
>   (3) For check-single, print the 'No matching tests found' message
>   just like we do for a parallel test run.  This makes the parallel and
>   non-parallel testing behaviour more similar, and I think is clearer
>   than the current 'Illegal Argument' error message.
>
> Points (1) and (2) will be handled by moving the post processing steps
> inside an if block within the recipe.  For check-single I propose
> deleting the gdb.sum and gdb.log files before running DejaGnu, this is
> similar (I think) to how we delete the outputs/ directory in the
> check-parallel rule.
>
> For point (3) I plan to split the check-single rule in two, the
> existing check-single will be renamed do-check-single, then a new
> check-single rule will be added.  The new check-single rule can either
> depend on the new do-check-single, or will ensure the 'No matching
> tests found' message is printed when appropriate.
> ---
>   gdb/testsuite/Makefile.in | 40 +++++++++++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
> index 87ba522c9e0..49b8a60a35c 100644
> --- a/gdb/testsuite/Makefile.in
> +++ b/gdb/testsuite/Makefile.in
> @@ -203,18 +203,36 @@ expanded_tests := $(patsubst $(srcdir)/%,%,$(wildcard $(addprefix $(srcdir)/,$(T
>   expanded_tests_or_none := $(or $(expanded_tests),no-matching-tests-found)
>   endif
>   
> +# With check-single, if TESTS was expanded to "no-matching-tests-found" then
> +# this will be passed to DejaGnu, resuling in an error.  With check-parallel
> +# in the same situation, we avoid invoking DejaGnu, and instead just call
> +# the check/no-matching-tests-found rule (which prints a helpful message).
> +#
> +# To get the same behaviour for check-single we decide here, based on how
> +# TESTS expanded, whether check-single should redirect to do-check-single or
> +# to check/no-matching-tests-found.
> +ifeq ($(expanded_tests_or_none),no-matching-tests-found)
> +CHECK_SINGLE_DEP=check/no-matching-tests-found
> +else
> +CHECK_SINGLE_DEP=do-check-single
> +endif
> +
>   # Shorthand for running all the tests in a single directory.
>   check-gdb.%:
>   	$(MAKE) check TESTS="gdb.$*/*.exp"
>   
> -check-single:
> -	-rm -f *core*
> +do-check-single:
> +	-rm -f *core* gdb.sum gdb.log
>   	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
>   	result=$$?; \
> -	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
> -	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
> +	if test -e gdb.sum; then \
> +	  $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
> +	  sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
> +	fi; \
>   	exit $$result
>   
> +check-single: $(CHECK_SINGLE_DEP)
> +
>   check-single-racy:
>   	-rm -rf cache racy_outputs temp
>   	mkdir -p racy_outputs; \
> @@ -240,12 +258,14 @@ check-parallel:
>   	-rm -rf cache outputs temp
>   	$(MAKE) -k do-check-parallel; \
>   	result=$$?; \
> -	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \
> -	  `find outputs -name gdb.sum -print` > gdb.sum; \
> -	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
> -	  `find outputs -name gdb.log -print` > gdb.log; \
> -	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
> -	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
> +	if test -d outputs; then \
> +	  $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \
> +	    `find outputs -name gdb.sum -print` > gdb.sum; \
> +	  $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
> +	    `find outputs -name gdb.log -print` > gdb.log; \
> +	  $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
> +	  sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
> +	fi; \
>   	exit $$result
>   
>   check-parallel-racy: