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

[Jacob Bachmeyer]

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