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

[Jacob Bachmeyer]

Tom de Vries wrote:
> On 6/16/20 6:05 AM, Jacob Bachmeyer wrote:
>   
> > Tom de Vries wrote:
> >     
> > > If I add a call to foobar at the end of a test-case in that testsuite,
> > > and I do a run of the testsuite, then the test-case aborts, and
> > > subsequently the testsuite run aborts, that is, no further tests are run.
> > >
> > > While it is as expected that the test-case aborts, it's not desirable
> > > that the testsuite run aborts.
> > >   
> > >       
> > The main problem I have with this is that calling an unknown procedure
> > is presumably a very serious error.  Aborting the entire run is a good
> > way to ensure that the problem is noticed, but perhaps with a testsuite
> > as large (and long-running) as the GDB testsuite has become, continuing
> > may also be reasonable to salvage at least some results.
> >
> > Where in the GDB testsuite is this a problem?  When is it appropriate to
> > continue after such a serious error?
> >     
>
> It's not a problem in a specific test-case, but a generic problem. If
> somebody makes a typo in a test-case and checks it in, there's no need
> to abort the test run.
>   

So the root cause problem is people committing changes to the GDB 
testsuite without actually testing their own changes first, at all?

> > How to ensure that the error is
> > not buried in the logs amidst the normal output from other tests and is
> > actually noticed?
> >     
>
> I monitor ERROR and WARNING in my test runs, so I didn't think of this,
> but indeed, those are not tracked in the summary, so that could be a
> problem, agreed.
>   

Blowing up the entire test run is a good way to ensure that problems 
(like typos in testcases) are noticed, but admittedly, that does not 
help when people fail to even run the tests before checking in their 
typos and then other developers' test runs blow up.  Murphy's Law says 
the offenders then carry on to check in more typos in other code without 
testing their own work there either...

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

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.



-- Jacob