bug#41824: Dejagnu's unknown proc aborts testsuite run when triggered in test-case

[Jacob Bachmeyer]

Tom de Vries wrote:
> On 6/17/20 5:26 AM, Jacob Bachmeyer wrote:
>   
> > Tom de Vries wrote:
> >     
> > > On 6/16/20 6:05 AM, Jacob Bachmeyer wrote:
> > >  
> > >       
> > > > Tom de Vries wrote:
> > > >         
> > > > > A possible solution to this problem could be to make the exiting on
> > > > > error (which is done in dejagnu's version of proc unknown in
> > > > > framework.exp) optional.
> > > > >         
> > > > >           
> > > > This is obviously a good solution, but leads to the obvious question of
> > > > how (command-line option?  (Make normally aborts on error, but has a
> > > > --keep-going option.)  configuration variable?) that should be
> > > > determined, or if exit-on-unknown-procedure should ever be a default.
> > > > Could simply reporting an UNRESOLVED test (indicating that the testcase
> > > > script aborted due to calling an undefined command) be a better
> > > > option? It would show up in the summary report at the end.
> > > >
> > > > At first, I was planning to schedule this for the 1.7 series, but I
> > > > checked and runtest.exp:runtest already catches Tcl errors, so fixing
> > > > this does not require a significant change to the normal control
> > > > flow. Would simply reporting an "UNRESOLVED:  testcase $file aborted on
> > > > unknown command '$what'" result, then propagating the Tcl error in the
> > > > ::unknown proc be a workable solution?
> > > >     
> > > >         
> > > I think so, yes.  I've tried that out in attached gdb patch, which also
> > > solves the reliance on ::tcl_unknown.
> > >   
> > >       
> > That patch is subtly wrong in other ways.  First, it assumes that
> > ::unknown is a procedure and not a command; there is no guarantee of
> > this in the Tcl manual.  Second, it will fail if run inside a safe
> > interpreter where the default "unknown" does not exist.  Third, it
> > breaks Tcl's default autoloading because it adds an UNRESOLVED result
> > regardless of the result of Tcl's autoload search.
> >     
>
> Your last remark here got me thinking about dejagnu's unknown proc, and
> I've filed two bugs:
> - bug#41914: Propagate return value of auto-loaded command
> - bug#41918: Propagate error value of auto-loaded command
>   

I believe both of these were fixed while working on this issue in the 
attached patch.  Apologies for the delay; I wanted tests for the new 
feature.

> > The best solution is to just revert the hacks and wait for this to be
> > solved upstream, where it is expected to land for the 1.6.3 release.
> >
> >     
> > > [...]
> > > Note btw that in both cases I didn't put $file in the unresolved
> > > message, because that's already present in pf_prefix, but that might not
> > > always be true.
> > >   
> > >       
> > The use of pf_prefix appears to be a feature of the GDB testsuite, so
> > the upstream implementation of this feature will not be able to rely on it.
> >
> >
> > I am hesitant to alter default behavior for 1.6.3, so this will probably
> > be added as a "--keep-going" option to runtest.>  I should be able to
> > land this for 1.6.3; it is simple enough to do and we are already
> > getting some other new command-line options in 1.6.3 anyway that were
> > needed to fix DejaGnu's own testsuite.  As of yet, there is no
> > (supported) way planned to set the --keep-going option from an init
> > file, only on the actual command-line.  This may change in the future;
> > if it does, there will be some general API or just a special parse of
> > ~/.dejagnurc looking for command-line options.  These are planned for
> > 1.7 at the earliest.
> >     
>
> I guess it would be convenient if we could switch this on somehow from
> lib/${tool}.exp.

The problem here is that this detects an invalidity in the testsuite.  
Allowing testsuites to select --keep_going is asking (Murphy's Law 
again) for people to turn it on and then ignore the errors from broken 
test scripts "because it works" -- never mind that it is obviously 
broken, but an extra UNRESOLVED or three can still be easily ignored.  
The entire test run stopping cold unless a special command-line option 
is given that is specifically documented to suppress abort-on-error is 
much harder to explain away or even try to justify as acceptable.


-- Jacob

From c5b21f1f1cfaabf1431010c314aadcc0b7b708f0 Mon Sep 17 00:00:00 2001
From: Jacob Bachmeyer <jcb62281+dev@gmail.com>
Date: Wed, 17 Jun 2020 18:08:57 -0500
Subject: [PATCH] Allow testing to continue after an undefined command is called

---
 ChangeLog                                          |   26 +++++++
 Makefile.am                                        |    2 +-
 Makefile.in                                        |    2 +-
 NEWS                                               |    2 +
 doc/dejagnu.texi                                   |    5 +
 doc/runtest.1                                      |    3 +
 lib/framework.exp                                  |   24 +++++-
 runtest.exp                                        |   12 +++
 testsuite/runtest.main/abort.exp                   |   78 ++++++++++++++++++++
 .../abort/testsuite/abort.test/abort-undef.exp     |   25 ++++++
 .../abort/testsuite/abort.test/simple.exp          |   21 +++++
 11 files changed, 195 insertions(+), 5 deletions(-)
 create mode 100644 testsuite/runtest.main/abort.exp
 create mode 100644 
testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp
 create mode 100644 testsuite/runtest.main/abort/testsuite/abort.test/simple.exp

diff --git a/ChangeLog b/ChangeLog
index 69a80ef..2351101 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,29 @@
+2020-06-17  Jacob Bachmeyer  <jcb62281+dev@gmail.com>
+
+       PR 41824
+
+       * NEWS: Add item for --keep_going option.
+
+       * Makefile.am (CLEANFILES): Add abort-init.exp to list.
+
+       * doc/dejagnu.texi (Invoking runtest): Document new --keep_going
+       command line option.
+       * doc/runtest.1: Likewise.
+
+       * lib/framework.exp (unknown): Report an UNRESOLVED result if an
+       unknown command is invoked.  Avoid exiting and propagate the error
+       from Tcl's "unknown" procedure if --keep_going was
+       specified. Brace procedure argument list.
+       * runtest.exp (dejagnu::opt): New namespace.
+       Add option --keep_going to continue running tests after a test
+       script aborts due to calling an undefined command.
+
+       * testsuite/runtest.main/abort.exp: New file.
+       * testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp:
+       New file.
+       * testsuite/runtest.main/abort/testsuite/abort.test/simple.exp:
+       New file.
+
 2020-06-02  Jacob Bachmeyer  <jcb62281+dev@gmail.com>
 
        PR 41647
diff --git a/Makefile.am b/Makefile.am
index 6c48c20..9a2ba81 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -26,7 +26,7 @@ EXTRA_DIST = ChangeLog-1992 MAINTAINERS dejagnu runtest \
        $(commands_DATA) $(TESTSUITE_FILES) $(TEXINFO_TEX)\
        $(CONTRIB)
 
-CLEANFILES = options-init.exp stats-init.exp
+CLEANFILES = abort-init.exp options-init.exp stats-init.exp
 
 clean-local: clean-local-check
 .PHONY: clean-local-check
diff --git a/Makefile.in b/Makefile.in
index 1cd211c..bc9d1e5 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -381,7 +381,7 @@ EXTRA_DIST = ChangeLog-1992 MAINTAINERS dejagnu runtest \
        $(commands_DATA) $(TESTSUITE_FILES) $(TEXINFO_TEX)\
        $(CONTRIB)
 
-CLEANFILES = options-init.exp stats-init.exp
+CLEANFILES = abort-init.exp options-init.exp stats-init.exp
 bin_SCRIPTS = dejagnu runtest
 include_HEADERS = dejagnu.h
 pkgdata_DATA = \
diff --git a/NEWS b/NEWS
index 209e9c4..4354422 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,8 @@ Changes since 1.6.2:
    should use this proc. The 'is_remote' proc is deprecated.
 2. runtest now accepts --local_init and --global_init options to override
    the default of reading "site.exp".  See the manual for details.
+X. runtest now accepts a --keep_going option to continue with other test
+   scripts after a test script invokes an undefined command.
 3. A utility procedure relative_filename has been added.  This procedure
    computes a relative file name to a given destination from a given base.
 4. The utility procedure 'grep' now accepts a '-n' option that
diff --git a/doc/dejagnu.texi b/doc/dejagnu.texi
index bb386e8..cd8c4e9 100644
--- a/doc/dejagnu.texi
+++ b/doc/dejagnu.texi
@@ -644,6 +644,11 @@ The host board to use.
 @item @code{--ignore [tests(s)] }
 The name(s) of specific tests to ignore.
 
+@item @code{--keep_going}
+Continue testing when test scripts encounter recoverable fatal errors.
+Such errors always cause the offending test script to abort
+immediately, but DejaGnu may be able to continue with the next script.
+
 @item @code{--local_init [name]}
 Use @emph{name} as the testsuite local init file instead of
 @file{site.exp} in the current directory and in @emph{objdir}.  The
diff --git a/doc/runtest.1 b/doc/runtest.1
index d043ee7..e8913b1 100644
--- a/doc/runtest.1
+++ b/doc/runtest.1
@@ -45,6 +45,9 @@ The host board definition to use.
 .BI --ignore \ test1.exp\ test2.exp\ ...
 Do not run the specified tests.
 .TP
+.B --keep_going
+Do not abort test run if a test script encounters a fatal error.
+.TP
 .BI --local_init \ NAME
 The NAME to use for the testsuite local init file in both the current
 directory and objdir.
diff --git a/lib/framework.exp b/lib/framework.exp
index e6ce197..53333ad 100644
--- a/lib/framework.exp
+++ b/lib/framework.exp
@@ -257,21 +257,39 @@ proc isnative { } {
 # This allows Tcl package autoloading to work in the modern age.
 
 rename ::unknown ::tcl_unknown
-proc unknown args {
-    if {[catch {uplevel 1 ::tcl_unknown $args} msg]} {
+proc unknown { args } {
+    set code [catch {uplevel 1 ::tcl_unknown $args} msg]
+    if { $code != 0 } {
        global errorCode
        global errorInfo
        global exit_status
 
+       set ret_cmd [list return -code $code]
+
        clone_output "ERROR: (DejaGnu) proc \"$args\" does not exist."
        if {[info exists errorCode]} {
+           lappend ret_cmd -errorcode $errorCode
            send_error "The error code is $errorCode\n"
        }
        if {[info exists errorInfo]} {
+           # omitting errorInfo from the propagated error makes this code
+           # invisible with the backtrace pointing directly to the problem
            send_error "The info on the error is:\n$errorInfo\n"
        }
        set exit_status 2
-       log_and_exit
+
+       set unresolved_msg "testcase '[uplevel info script]' aborted"
+       append unresolved_msg " at call to unknown command '$args'"
+       unresolved $unresolved_msg
+
+       lappend ret_cmd $msg
+       if { $::dejagnu::opt::keep_going } {
+           eval $ret_cmd
+       } else {
+           log_and_exit
+       }
+    } else {
+       return $msg
     }
 }
 
diff --git a/runtest.exp b/runtest.exp
index ba49e73..028ad5b 100644
--- a/runtest.exp
+++ b/runtest.exp
@@ -97,6 +97,13 @@ set global_init_file site.exp        ;# global init file name
 set testsuitedir       "testsuite"     ;# top-level testsuite source directory
 set testbuilddir       "testsuite"     ;# top-level testsuite object directory
 
+#
+# These are used for internal command-line flags.
+#
+namespace eval ::dejagnu::opt {
+    variable keep_going        0       ;# continue after a fatal error in 
testcase?
+}
+
 # Various ccache versions provide incorrect debug info such as ignoring
 # different current directory, breaking GDB testsuite.
 set env(CCACHE_DISABLE) 1
@@ -372,6 +379,7 @@ proc usage { } {
     send_user "\t--host \[triplet\]\tThe canonical triplet of the host 
machine\n"
     send_user "\t--host_board \[name\]\tThe host board to use\n"
     send_user "\t--ignore \[name(s)\]\tThe names of specific tests to ignore\n"
+    send_user "\t--keep_going\t\tContinue testing even if a script aborts\n"
     send_user "\t--local_init \[name\]\tThe file to load for local 
configuration\n"
     send_user "\t--log_dialog\t\t\Emit Expect output on stdout\n"
     send_user "\t--mail \[name(s)\]\tWhom to mail the results to\n"
@@ -1201,6 +1209,10 @@ for { set i 0 } { $i < $argc } { incr i } {
            continue
        }
 
+       "--k*" {                        # (--keep_going) reduce fatal errors
+           set ::dejagnu::opt::keep_going 1
+       }
+
        "--m*" {                        # (--mail) mail the output
            set mailing_list $optarg
            set mail_logs 1
diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
new file mode 100644
index 0000000..c5f7014
--- /dev/null
+++ b/testsuite/runtest.main/abort.exp
@@ -0,0 +1,78 @@
+# Copyright (C) 1995-2016, 2018, 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu 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.
+#
+# DejaGnu 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 DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# This file tests handling of fatal errors in testcases.
+# The way we do this is to recursively invoke ourselves on a small testsuite
+# and analyze the results.
+
+load_lib util-defs.exp
+
+if {![info exists tmpdir]} {
+    set tmpdir [testsuite file -object -top tmpdir]
+}
+
+set fd [open abort-init.exp w]
+puts $fd "set srcdir [testsuite file -source -test abort]"
+puts $fd "set objdir [testsuite file -object -test abort]"
+puts $fd "set tmpdir $tmpdir"
+close $fd
+
+if {![file isdirectory $tmpdir]} {
+    catch "file mkdir $tmpdir"
+}
+
+if {![file isdirectory [testsuite file -object -test abort]]} {
+    catch {file mkdir [testsuite file -object -test abort]}
+}
+
+set tests {
+    { "run only simple test"
+       "simple.exp"
+       "PASS: simple test.*\
+       *expected passes\[ \t\]+1\n" }
+    { "abort on undefined command"
+       "abort-undef.exp"
+       "PASS: running abort-undef.exp.*\
+       *UNRESOLVED: .* aborted at call to unknown command.*\
+       *expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "stop at abort without --keep_going"
+       "abort-undef.exp simple.exp"
+       "PASS: running abort-undef.exp.*\
+       *UNRESOLVED: .* aborted at call to unknown command.*\
+       *expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "continue after abort with --keep_going"
+       "--keep_going abort-undef.exp simple.exp"
+       "PASS: running abort-undef.exp.*\
+       *UNRESOLVED: .* aborted at call to unknown command.*\
+       *PASS: simple test.*\
+       *expected passes\[ \t\]+2\n.*unresolved testcases\[ \t\]+1\n" }
+}
+
+foreach t $tests {
+    if [util_test $RUNTEST \
+           "--local_init abort-init.exp\
+               --outdir $tmpdir -a [lindex $t 1]" \
+           "" \
+           [lindex $t 2]] {
+       fail [lindex $t 0]
+    } else {
+       pass [lindex $t 0]
+    }
+}
+
+file delete -force $tmpdir
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp 
b/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp
new file mode 100644
index 0000000..e5f4803
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp
@@ -0,0 +1,25 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu 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.
+#
+# DejaGnu 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 DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# Invoke an undefined command, causing a fatal error.
+
+pass "running abort-undef.exp"
+
+bogus_command 1 2 3 4
+
+fail "script did not abort"
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/simple.exp 
b/testsuite/runtest.main/abort/testsuite/abort.test/simple.exp
new file mode 100644
index 0000000..93a03e7
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/simple.exp
@@ -0,0 +1,21 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu 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.
+#
+# DejaGnu 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 DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# Return a passing result
+
+pass "simple test"
-- 
1.7.4.1