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

[Tom de Vries]

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

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

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

Thanks,
- Tom