[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dejagnu-bug
Subject:    bug#41918: [PATCH] Propagate error value of auto-loaded command
From:       Jacob Bachmeyer <jcb62281 () gmail ! com>
Date:       2020-06-20 0:32:12
Message-ID: 5EED590C.2090706 () gmail ! com
[Download RAW message or body]

Tom de Vries wrote:
> 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.
>
>   

That handling was actually very inconsistent:  an error in a regular 
testcase aborts the script, but testing (almost) silently continued, but 
an error if auto-loading had been used aborted the entire test run.  
That difference in handling proc-not-found and other Tcl errors was a 
long-standing bug in DejaGnu.  Any Tcl error means that the results are 
not really valid, so almost silently continuing is wrong and was wrong.

>> 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.
>   

Odd, but that is why I pushed that branch.

> 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.
>   

I have since realized that DejaGnu's ::unknown seems to be about useless 
other than producing that first error.  I am thinking about removing it 
entirely now that runtest aborts after catching a Tcl error from a test 
script.

> Anyway, I have two patches attached:
> - one that rewords the NEWS entries into something more intuitive for me
>   
Agreed; the NEWS entries were written separately and the later change 
did make the first one wrong.  Thanks.

> - one that adds a test-case abort-dbz.exp, a copy of
>   abort-al-dbz.exp, but without the auto-load bit.
>   
Agreed; that was a missing case in the testsuite.  Thanks.



-- Jacob



_______________________________________________
Bug-dejagnu mailing list
Bug-dejagnu@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-dejagnu

[prev in list] [next in list] [prev in thread] [next in thread]