diff mbox series

[DejaGNU,1/1] Support per-test execution timeout factor

Message ID alpine.DEB.2.20.2312111745200.5892@tpp.orcam.me.uk
State New
Headers show
Series [DejaGNU,1/1] Support per-test execution timeout factor | expand

Commit Message

Maciej W. Rozycki Dec. 12, 2023, 2:04 p.m. UTC 
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'.
---
 baseboards/qemu.exp |    4 ++++
 config/gdb-comm.exp |    4 ++++
 config/gdb_stub.exp |    4 ++++
 config/sim.exp      |    4 ++++
 config/unix.exp     |    4 ++++
 doc/dejagnu.texi    |   10 +++++++++-
 6 files changed, 29 insertions(+), 1 deletion(-)

dejagnu-test-timeout-factor.diff

Comments

Jeff Law Dec. 12, 2023, 11:02 p.m. UTC | #1 
On 12/12/23 07:04, 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'.
OK
jeff
Jacob Bachmeyer Dec. 13, 2023, 3:48 a.m. UTC | #2 
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
diff mbox series

Patch

Index: dejagnu/baseboards/qemu.exp
===================================================================
--- dejagnu.orig/baseboards/qemu.exp
+++ dejagnu/baseboards/qemu.exp
@@ -200,11 +200,15 @@  proc qemu_load { dest prog args } {
     global qemu
     global timeout
     global test_timeout
+    global test_timeout_factor
 
     set wait_timeout $timeout
     if {[info exists test_timeout]} {
 	set wait_timeout $test_timeout
     }
+    if {[info exists test_timeout_factor]} {
+	set wait_timeout [expr $wait_timeout * $test_timeout_factor]
+    }
 
     verbose -log "Executing on $dest: $prog (timeout = $wait_timeout)" 2
 
Index: dejagnu/config/gdb-comm.exp
===================================================================
--- dejagnu.orig/config/gdb-comm.exp
+++ dejagnu/config/gdb-comm.exp
@@ -254,6 +254,7 @@  proc gdb_comm_load { dest prog args } {
     global GDBFLAGS
     global gdb_prompt
     global test_timeout
+    global test_timeout_factor
     set argnames { "command-line arguments" "input file" "output file" }
 
     for { set x 0 } { $x < [llength $args] } { incr x } {
@@ -274,6 +275,9 @@  proc gdb_comm_load { dest prog args } {
     } else {
 	set testcase_timeout 300
     }
+    if {[info exists test_timeout_factor]} {
+	set testcase_timeout [expr $testcase_timeout * $test_timeout_factor]
+    }
 
     verbose -log "Executing on $dest: $prog (timeout = $testcase_timeout)" 2
 
Index: dejagnu/config/gdb_stub.exp
===================================================================
--- dejagnu.orig/config/gdb_stub.exp
+++ dejagnu/config/gdb_stub.exp
@@ -471,6 +471,7 @@  proc gdb_stub_wait { dest timeout } {
 }
 
 proc gdb_stub_load { dest prog args } {
+    global test_timeout_factor
     global test_timeout
     global gdb_prompt
     set argnames { "command-line arguments" "input file" "output file" }
@@ -485,6 +486,9 @@  proc gdb_stub_load { dest prog args } {
     if {[info exists test_timeout]} {
 	set wait_timeout $test_timeout
     }
+    if {[info exists test_timeout_factor]} {
+	set wait_timeout [expr $wait_timeout * $test_timeout_factor]
+    }
 
     verbose -log "Executing on $dest: $prog (timeout = $wait_timeout)" 2
 
Index: dejagnu/config/sim.exp
===================================================================
--- dejagnu.orig/config/sim.exp
+++ dejagnu/config/sim.exp
@@ -60,6 +60,7 @@  proc sim_wait { dest timeout } {
 }
 
 proc sim_load { dest prog args } {
+    global test_timeout_factor
     global test_timeout
 
     set inpfile ""
@@ -82,6 +83,9 @@  proc sim_load { dest prog args } {
     } else {
 	set sim_time_limit 240
     }
+    if {[info exists test_timeout_factor]} {
+	set sim_time_limit [expr $sim_time_limit * $test_timeout_factor]
+    }
 
     set output ""
 
Index: dejagnu/config/unix.exp
===================================================================
--- dejagnu.orig/config/unix.exp
+++ dejagnu/config/unix.exp
@@ -33,6 +33,7 @@  load_lib remote.exp
 
 
 proc unix_load { dest prog args } {
+    global test_timeout_factor
     global ld_library_path
     global test_timeout
     set output ""
@@ -42,6 +43,9 @@  proc unix_load { dest prog args } {
     if {[info exists test_timeout]} {
 	set wait_timeout $test_timeout
     }
+    if {[info exists test_timeout_factor]} {
+	set wait_timeout [expr $wait_timeout * $test_timeout_factor]
+    }
 
     if { [llength $args] > 0 } {
 	set parg [lindex $args 0]
Index: dejagnu/doc/dejagnu.texi
===================================================================
--- dejagnu.orig/doc/dejagnu.texi
+++ dejagnu/doc/dejagnu.texi
@@ -1363,11 +1363,19 @@  by DejaGnu itself for cross testing, but
 to manipulate these itself.
 
 @vindex test_timeout
+@vindex test_timeout_factor
 The local @file{site.exp} may also set Tcl variables such as
 @code{test_timeout} which can control the amount of time (in seconds)
 to wait for a remote test to complete.  If not specified,
 @code{test_timeout} defaults to 120 or 300 seconds, depending on the
-communication protocol.
+communication protocol.  Additionally if @code{test_timeout_factor}
+Tcl variable has been set, then the amount of time to wait will be
+further multiplied by the value of this variable.  This multiplier
+will typically be set on a test case by test case basis though rather
+than in a configuration file, so that particularly slow test cases
+can be given a chance to complete while preventing excessive wait
+time from affecting the duration of a testsuite run if other test
+cases do fail to complete.
 
 @node Board configuration file, Remote host testing, Local configuration file, Customizing DejaGnu
 @section Board configuration file