[PATCH] testsuite: Add --status to runtest invocation

[PATCH] testsuite: Add --status to runtest invocation

[Simon Marchi]

As discussed in this thread:

  https://sourceware.org/ml/gdb-patches/2016-01/msg00243.html

By default, if a test driver (a test .exp) ends with an uncaught
error/exception, the runtest command will still have a return code of 0
(success).  I think that if a test (or the environment) is broken and
the test ends up with an exception, it should be considered as failed so
that we can notice it and fix it.

Passing the --status flag to runtest will make it return an error if one
of the test it runs ends up with an uncaught error.

gdb/testsuite/ChangeLog:

	* Makefile.in (check-single): Pass --status to runtest.
	(check/%.exp): Likewise.
---
 gdb/testsuite/Makefile.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index fb0d8e5..ae7fa7b 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -193,7 +193,7 @@ DO_RUNTEST = \
 @GMAKE_TRUE@	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none)
+	$(DO_RUNTEST) --status $(RUNTESTFLAGS) $(expanded_tests_or_none)
 
 check-parallel:
 	-rm -rf cache outputs temp
@@ -227,7 +227,7 @@ do-check-parallel: $(TEST_TARGETS)
 
 @GMAKE_TRUE@check/%.exp:
 @GMAKE_TRUE@	-mkdir -p outputs/$*
-@GMAKE_TRUE@	@$(DO_RUNTEST) GDB_PARALLEL=yes --outdir=outputs/$* $*.exp $(RUNTESTFLAGS)
+@GMAKE_TRUE@	@$(DO_RUNTEST) GDB_PARALLEL=yes --outdir=outputs/$* $*.exp --status $(RUNTESTFLAGS)
 
 check/no-matching-tests-found:
 	@echo ""
-- 
2.5.1

Re: [PATCH] testsuite: Add --status to runtest invocation

[Pedro Alves]

On 01/18/2016 10:52 PM, Simon Marchi wrote:
> As discussed in this thread:
> 
>   https://sourceware.org/ml/gdb-patches/2016-01/msg00243.html
> 
> By default, if a test driver (a test .exp) ends with an uncaught
> error/exception, the runtest command will still have a return code of 0
> (success).  I think that if a test (or the environment) is broken and
> the test ends up with an exception, it should be considered as failed so
> that we can notice it and fix it.
> 
> Passing the --status flag to runtest will make it return an error if one
> of the test it runs ends up with an uncaught error.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* Makefile.in (check-single): Pass --status to runtest.
> 	(check/%.exp): Likewise.

Hmm, the perf bits of the runtest invocation already pass --status:

@GMAKE_TRUE@	$(DO_RUNTEST) --status --outdir=gdb.perf/outputs/$* lib/build-piece.exp WORKER=$* GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=build-pieces
...
@GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf/workers GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=gen-workers
...
@GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=final

But there's one that doesn't:

check-perf: all $(abs_builddir)/site.exp
	@if test ! -d gdb.perf; then mkdir gdb.perf; fi
	$(DO_RUNTEST) --directory=gdb.perf --outdir gdb.perf GDB_PERFTEST_MODE=$(GDB_PERFTEST_MODE) $(RUNTESTFLAGS)

Seems like an oversight?

How about adding --status to DO_RUNTEST directly instead, so
that all invocations are always covered?

Thanks,
Pedro Alves

Re: [PATCH] testsuite: Add --status to runtest invocation

[Simon Marchi]

On 16-01-19 06:24 AM, Pedro Alves wrote:
> On 01/18/2016 10:52 PM, Simon Marchi wrote:
>> As discussed in this thread:
>>
>>   https://sourceware.org/ml/gdb-patches/2016-01/msg00243.html
>>
>> By default, if a test driver (a test .exp) ends with an uncaught
>> error/exception, the runtest command will still have a return code of 0
>> (success).  I think that if a test (or the environment) is broken and
>> the test ends up with an exception, it should be considered as failed so
>> that we can notice it and fix it.
>>
>> Passing the --status flag to runtest will make it return an error if one
>> of the test it runs ends up with an uncaught error.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* Makefile.in (check-single): Pass --status to runtest.
>> 	(check/%.exp): Likewise.
> 
> Hmm, the perf bits of the runtest invocation already pass --status:
> 
> @GMAKE_TRUE@	$(DO_RUNTEST) --status --outdir=gdb.perf/outputs/$* lib/build-piece.exp WORKER=$* GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=build-pieces
> ...
> @GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf/workers GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=gen-workers
> ...
> @GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=final
> 
> But there's one that doesn't:
> 
> check-perf: all $(abs_builddir)/site.exp
> 	@if test ! -d gdb.perf; then mkdir gdb.perf; fi
> 	$(DO_RUNTEST) --directory=gdb.perf --outdir gdb.perf GDB_PERFTEST_MODE=$(GDB_PERFTEST_MODE) $(RUNTESTFLAGS)
> 
> Seems like an oversight?
> 
> How about adding --status to DO_RUNTEST directly instead, so
> that all invocations are always covered?

That's fine with me.

However, I did a wrong git command and ended up pushing this patch as
we as "testsuite: Make check-parallel return non-zero if a test failed"
without the ChangeLog.  I added their ChangeLog entries in a separate
commit.

So here's a patch that goes on top of all that that factors out --status.


From d64bd1d8ec33ec22ea6125e93bc670b3d19d5ec4 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Tue, 19 Jan 2016 10:53:20 -0500
Subject: [PATCH] testsuite: Factor out --status in DO_RUNTEST

gdb/testsuite/ChangeLog:

	* Makefile.in (DO_RUNTEST): Add --status and update usages.
---
 gdb/testsuite/Makefile.in | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 50edf8a..f59acc3 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -169,7 +169,7 @@ DO_RUNTEST = \
 	if [ -f $${rootme}/../../expect/expect ] ; then  \
 	  TCL_LIBRARY=$${srcdir}/../../tcl/library ; \
 	  export TCL_LIBRARY ; fi ; \
-	$(RUNTEST)
+	$(RUNTEST) --status

 # TESTS exists for the user to pass on the command line to easily
 # say "Only run these tests."  With check-single it's not necessary, but
@@ -193,7 +193,7 @@ DO_RUNTEST = \
 @GMAKE_TRUE@	$(MAKE) check TESTS="gdb.$*/*.exp"

 check-single:
-	$(DO_RUNTEST) --status $(RUNTESTFLAGS) $(expanded_tests_or_none)
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none)

 check-parallel:
 	-rm -rf cache outputs temp
@@ -229,7 +229,7 @@ do-check-parallel: $(TEST_TARGETS)

 @GMAKE_TRUE@check/%.exp:
 @GMAKE_TRUE@	-mkdir -p outputs/$*
-@GMAKE_TRUE@	@$(DO_RUNTEST) GDB_PARALLEL=yes --outdir=outputs/$* $*.exp --status $(RUNTESTFLAGS)
+@GMAKE_TRUE@	@$(DO_RUNTEST) GDB_PARALLEL=yes --outdir=outputs/$* $*.exp $(RUNTESTFLAGS)

 check/no-matching-tests-found:
 	@echo ""
@@ -239,7 +239,7 @@ check/no-matching-tests-found:
 # Utility rule invoked by step 2 of the build-perf rule.
 @GMAKE_TRUE@workers/%.worker:
 @GMAKE_TRUE@	mkdir -p gdb.perf/outputs/$*
-@GMAKE_TRUE@	$(DO_RUNTEST) --status --outdir=gdb.perf/outputs/$* lib/build-piece.exp WORKER=$* GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=build-pieces
+@GMAKE_TRUE@	$(DO_RUNTEST) --outdir=gdb.perf/outputs/$* lib/build-piece.exp WORKER=$* GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=build-pieces

 # Utility rule to build tests that support it in parallel.
 # The build is broken into 3 steps distinguished by GDB_PERFTEST_SUBMODE:
@@ -259,11 +259,11 @@ check/no-matching-tests-found:
 @GMAKE_TRUE@	rm -rf gdb.perf/workers
 @GMAKE_TRUE@	mkdir -p gdb.perf/workers
 @GMAKE_TRUE@	@: Step 1: Generate the build .worker files.
-@GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf/workers GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=gen-workers
+@GMAKE_TRUE@	$(DO_RUNTEST) --directory=gdb.perf --outdir gdb.perf/workers GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=gen-workers
 @GMAKE_TRUE@	@: Step 2: Compile the pieces.  Here is the build parallelism.
 @GMAKE_TRUE@	$(MAKE) $$(cd gdb.perf && echo workers/*/*.worker)
 @GMAKE_TRUE@	@: Step 3: Do the final link.
-@GMAKE_TRUE@	$(DO_RUNTEST) --status --directory=gdb.perf --outdir gdb.perf GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=final
+@GMAKE_TRUE@	$(DO_RUNTEST) --directory=gdb.perf --outdir gdb.perf GDB_PARALLEL=gdb.perf $(RUNTESTFLAGS) GDB_PERFTEST_MODE=compile GDB_PERFTEST_SUBMODE=final

 # The default is to both compile and run the tests.
 GDB_PERFTEST_MODE = both
-- 
2.5.1

Re: [PATCH] testsuite: Add --status to runtest invocation

[Pedro Alves]

On 01/19/2016 04:01 PM, Simon Marchi wrote:

> However, I did a wrong git command and ended up pushing this patch as
> we as "testsuite: Make check-parallel return non-zero if a test failed"
> without the ChangeLog.  I added their ChangeLog entries in a separate
> commit.
> 

Happens. :-)

> So here's a patch that goes on top of all that that factors out --status.

LGTM.

Thanks,
Pedro Alves

Re: [PATCH] testsuite: Add --status to runtest invocation

[Simon Marchi]

On 16-01-19 11:04 AM, Pedro Alves wrote:
> LGTM.

Thanks, pushed!