Re: [PATCH DejaGNU 1/1] Support per-test execution timeout factor

Maciej W. Rozycki wrote:
> Add support for the `test_timeout_factor' global variable letting a test 
> case scale the wait timeout used for code execution.  This is useful for 
> particularly slow test cases for which increasing the wait timeout 
> globally would be excessive.
>
> 	* baseboards/qemu.exp (qemu_load): Handle `test_timeout_factor'.
> 	* config/gdb-comm.exp (gdb_comm_load): Likewise.
> 	* config/gdb_stub.exp (gdb_stub_load): Likewise.
> 	* config/sim.exp (sim_load): Likewise.
> 	* config/unix.exp (unix_load): Likewise.
> 	* doc/dejagnu.texi (Local configuration file): Document 
> 	`test_timeout_factor'.
> [...snip full diff...]

First, a minor technical issue:  brace your expr(n) expressions like this:

    set wait_timeout [expr { $wait_timeout * $test_timeout_factor }]

The Tcl expr(n) manpage recommends that style and explains a few 
situations where it is actually required for non-surprising results and 
that Tcl's optimizations work better if the expression to expr is 
braced.  All expr calls in new code in DejaGnu should have the braces.

Second, I need some more explanation how this fits together because I 
have some concerns about confusion between various timeouts.  In your 
introduction to this patch pair, you note that the test execution 
timeout and tool execution timeout are different.  My main concern is 
that "test_timeout_factor" (and for that matter, "test_timeout") may be 
badly named, or we need a more coherent model of testing with DejaGnu.  
(More precisely, we need better documentation...)

The anticipated confusion stems from the question of what exactly is the 
interval of a test?  In other words, what is the interval limited by 
"test_timeout"?  When does the clock start ticking and when does it stop 
before the alarm goes off?  (I have some suspicions that those answers 
are annoyingly counter-intuitive, which means I will have to write more 
documentation...)

Lastly, I note no objection to the dg-test-timeout-factor extension; as 
far as I can tell, dg.exp is designed to be extended in that way, so 
this is a supported extension point instead of an unsupportable monkeypatch.


-- Jacob