bug#41918: [PATCH] Propagate error value of auto-loaded command

[Tom de Vries]

On 6/19/20 2:00 AM, Jacob Bachmeyer wrote:
> Tom de Vries wrote:
>> On 6/18/20 1:19 AM, Jacob Bachmeyer wrote:
>>  
>>> Tom de Vries wrote:
>>>    
>>>> Hi,
>>>>
>>>> I think I found a bug in proc unknown in lib/framework.exp.
>>>>
>>>> Patch describing the problem and fixing it attached below.
>>>>         
>>> I found a similar issue while patching bug #41824; please check whether
>>> that patch addresses this issue adequately.
>>>     
>>
>> AFAICT, it does not.  Dejagnu test-case attached.
>>   
> 
> That test is incorrect:  it does not specify the --keep_going option,
> but expects runtest to continue after a test script aborts with a Tcl
> error.

Well, yes, it's respecting the difference in handling of proc-not-found
vs other tcl errors that is present in current dejagnu.

Since:
- your implementation of --keep_going did not modify this, and
- your description of keep-going in NEWS explictly respected that
  difference,
I had no reason to deviate from that.

Now that you've decided to change the handling of proc-not-found vs
other tcl errors in patch 0005, obviously the test-case needs to be
updated to accomodate for that.

> I have attached a fixed patch (0004) that avoids modifying the source
> tree by setting up autoloading in the build tree.
> 

Ack, thanks.

> I have also attached another patch (0005) that fixes an inconsistency in
> DejaGnu's handling of Tcl errors in test scripts.  Previously, DejaGnu
> would abort the test run if an undefined procedure is called, but would
> only issue an ERROR message and continue with the next test script for
> all other Tcl errors.  This patch solves that by ensuring that DejaGnu
> will abort on any uncaught Tcl error in a test script unless the
> --keep_going option is given on the command line.  An UNRESOLVED result
> appears in the log in any case if a test script aborts due to an error.
> 

I couldn't apply this cleanly, so instead I'm now looking at branch PR41918.

I see that for abort-undef.exp we have:
...
ERROR: (DejaGnu) proc "bogus_command 1 2 3 4" does not exist.
The error code is TCL LOOKUP COMMAND bogus_command
The info on the error is:
invalid command name "bogus_command"
    while executing
"::tcl_unknown bogus_command 1 2 3 4"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::tcl_unknown $args"
ERROR: tcl error sourcing
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp.
ERROR: tcl error code TCL LOOKUP COMMAND bogus_command
ERROR: invalid command name "bogus_command"
    while executing
"bogus_command 1 2 3 4"
    (file
"/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
line 23)
    invoked from within
"source
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
...

Should we error twice on this? FWIW, just removing ::unknown seems to be
the easiest way to fix that.

Anyway, I have two patches attached:
- one that rewords the NEWS entries into something more intuitive for me
- one that adds a test-case abort-dbz.exp, a copy of
  abort-al-dbz.exp, but without the auto-load bit.

Thanks,
- Tom



Reword NEWS entries

---
 NEWS | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 619f0ff..abae60b 100644
--- a/NEWS
+++ b/NEWS
@@ -7,11 +7,10 @@ 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 aborts if a test script fails with any Tcl error.  Previously,
+   only calling an undefined procedure would cause the test run to abort.
 X. runtest now accepts a --keep_going option to continue with other test
-   scripts after a test script invokes an undefined command.
-X. Unless the --keep_going option is used, runtest now aborts if a test
-   script fails with any Tcl error.  Previously, only calling an undefined
-   procedure would cause the test run to abort.
+   scripts after a test script fails with a Tcl error.
 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

Add abort.test/abort-dbz.exp

---
 testsuite/runtest.main/abort.exp                   | 11 ++++++++
 .../abort/testsuite/abort.test/abort-dbz.exp       | 29 ++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
index b352b56..59a934c 100644
--- a/testsuite/runtest.main/abort.exp
+++ b/testsuite/runtest.main/abort.exp
@@ -50,6 +50,17 @@ set tests {
        "PASS: running abort-undef.exp.*\
        *UNRESOLVED: .* aborted.*\
        *expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "stop at divide-by-zero without --keep_going"
+       "abort-dbz.exp simple.exp"
+       "PASS: running abort-dbz.exp.*\
+       *UNRESOLVED: .* aborted.*\
+       *expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "continue after divide-by-zero with --keep_going"
+       "--keep_going abort-dbz.exp simple.exp"
+       "PASS: running abort-dbz.exp.*\
+       *UNRESOLVED: .* aborted.*\
+       *PASS: simple test.*\
+       *expected passes\[ \t\]+2\n" }
     { "stop at auto-loaded divide-by-zero without --keep_going"
        "abort-al-dbz.exp simple.exp"
        "PASS: running abort-al-dbz.exp.*\
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp 
b/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp
new file mode 100644
index 0000000..711347d
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp
@@ -0,0 +1,29 @@
+# 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.
+
+# Cause a divide-by-zero error.
+
+pass "running abort-dbz.exp"
+
+proc throw_arith_error_div_by_zero { } {
+    expr { 1 / 0 }
+}
+
+throw_arith_error_div_by_zero
+
+fail "script did not abort"