[patch] print-threads.exp: Extend timeout for slower tests.

[patch] print-threads.exp: Extend timeout for slower tests.

[Doug Evans]

Hi.

This test can errantly fail due to timeouts,
so this patch extends the timeout for the slower tests.

I will commit this in a few days if there are no objections.

2011-11-02  Doug Evans  <dje@google.com>

	* testsuite/gdb.threads/print-threads.exp: Extend timeout for slower
	tests.

Index: print-threads.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/print-threads.exp,v
retrieving revision 1.19
diff -u -p -r1.19 print-threads.exp
--- print-threads.exp	7 Mar 2011 16:03:03 -0000	1.19
+++ print-threads.exp	2 Nov 2011 22:09:35 -0000
@@ -98,6 +98,9 @@ proc test_all_threads { name kill } {
     }
 }
 
+# Record the old timeout, we need to extend it for slower tests.
+set oldtimeout $timeout
+
 runto_main
 gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\."
 gdb_test_no_output "set var slow = 0"
@@ -106,12 +109,18 @@ test_all_threads "fast" 0
 runto_main
 gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (2)"
 gdb_test_no_output "set var slow = 1"
+# Extend the timeout for slower tests.
+set timeout [expr $oldtimeout + 120]
 test_all_threads "slow" 0
+set $timeout $oldtimeout
 
 runto_main
 gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (3)"
 gdb_test_no_output "set var slow = 1" "set var slow = 1 (2)"
 gdb_breakpoint "kill"
+# Extend the timeout for slower tests.
+set timeout [expr $oldtimeout + 120]
 test_all_threads "slow with kill breakpoint" 1
+set $timeout $oldtimeout
 
 return 0

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Jan Kratochvil]

On Wed, 02 Nov 2011 23:11:54 +0100, Doug Evans wrote:
> +# Record the old timeout, we need to extend it for slower tests.
> +set oldtimeout $timeout

You can use global $gdb_test_timeout nowadays.


Thanks,
Jan

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Doug Evans]

On Wed, Nov 2, 2011 at 4:21 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 02 Nov 2011 23:11:54 +0100, Doug Evans wrote:
>> +# Record the old timeout, we need to extend it for slower tests.
>> +set oldtimeout $timeout
>
> You can use global $gdb_test_timeout nowadays.

Thanks, I didn't know about gdb_test_timeout.
Still, I like the patch as is, one less global the code has to care about.

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Joel Brobecker]

> >> +# Record the old timeout, we need to extend it for slower tests.
> >> +set oldtimeout $timeout
> >
> > You can use global $gdb_test_timeout nowadays.
> 
> Thanks, I didn't know about gdb_test_timeout.
> Still, I like the patch as is, one less global the code has to care about.

Same here.  I don't think that this variable is meant to be changed
by testcases when you just want to change it for a few tests.
Otherwise, you run the risk of affecting all the other testcases,
which set the timeout to $gdb_test_timeout at the start of every
testcase (this was done to prevent a change of timeout duration
from affecting subsequent testcases). See gdb_init:

    # Reset the timeout value to the default.  This way, any testcase
    # that changes the timeout value without resetting it cannot affect
    # the timeout used in subsequent testcases.
    global gdb_test_timeout
    global timeout
    set timeout $gdb_test_timeout

-- 
Joel

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Jan Kratochvil]

On Mon, 07 Nov 2011 17:22:02 +0100, Joel Brobecker wrote:
> > >> +# Record the old timeout, we need to extend it for slower tests.
> > >> +set oldtimeout $timeout
> > >
> > > You can use global $gdb_test_timeout nowadays.
> > 
> > Thanks, I didn't know about gdb_test_timeout.
> > Still, I like the patch as is, one less global the code has to care about.
> 
> Same here.

I did not agree with Doug.  I think there was just some misunderstanding.

I was proposing the attached patch.

The very last line is sure redundant but I think it should be there for the
possibility of future test additions.

Found in Doug's original patch:
	set $timeout $oldtimeout
should have been:
	set timeout $oldtimeout


Thanks,
Jan


gdb/testsuite/
2011-11-08  Doug Evans  <dje@google.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/print-threads.exp: Extend timeout for slower tests.

--- a/gdb/testsuite/gdb.threads/print-threads.exp
+++ b/gdb/testsuite/gdb.threads/print-threads.exp
@@ -106,12 +106,18 @@ test_all_threads "fast" 0
 runto_main
 gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (2)"
 gdb_test_no_output "set var slow = 1"
+# Extend the timeout for slower tests.
+set timeout [expr $gdb_test_timeout + 120]
 test_all_threads "slow" 0
+set timeout $gdb_test_timeout
 
 runto_main
 gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (3)"
 gdb_test_no_output "set var slow = 1" "set var slow = 1 (2)"
 gdb_breakpoint "kill"
+# Extend the timeout for slower tests.
+set timeout [expr $gdb_test_timeout + 120]
 test_all_threads "slow with kill breakpoint" 1
+set timeout $gdb_test_timeout
 
 return 0

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Doug Evans]

On Mon, Nov 7, 2011 at 11:56 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 07 Nov 2011 17:22:02 +0100, Joel Brobecker wrote:
>> > >> +# Record the old timeout, we need to extend it for slower tests.
>> > >> +set oldtimeout $timeout
>> > >
>> > > You can use global $gdb_test_timeout nowadays.
>> >
>> > Thanks, I didn't know about gdb_test_timeout.
>> > Still, I like the patch as is, one less global the code has to care about.
>>
>> Same here.
>
> I did not agree with Doug.  I think there was just some misunderstanding.
>
> I was proposing the attached patch.
>
> The very last line is sure redundant but I think it should be there for the
> possibility of future test additions.
>
> Found in Doug's original patch:
>        set $timeout $oldtimeout
> should have been:
>        set timeout $oldtimeout

That's just a typo (obviously).

>
> Thanks,
> Jan
>
>
> gdb/testsuite/
> 2011-11-08  Doug Evans  <dje@google.com>
>            Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * gdb.threads/print-threads.exp: Extend timeout for slower tests.
>
> --- a/gdb/testsuite/gdb.threads/print-threads.exp
> +++ b/gdb/testsuite/gdb.threads/print-threads.exp
> @@ -106,12 +106,18 @@ test_all_threads "fast" 0
>  runto_main
>  gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (2)"
>  gdb_test_no_output "set var slow = 1"
> +# Extend the timeout for slower tests.
> +set timeout [expr $gdb_test_timeout + 120]
>  test_all_threads "slow" 0
> +set timeout $gdb_test_timeout
>
>  runto_main
>  gdb_test "break thread_function" "Breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*print-threads.c, line \[0-9\]*\\." "break thread_function (3)"
>  gdb_test_no_output "set var slow = 1" "set var slow = 1 (2)"
>  gdb_breakpoint "kill"
> +# Extend the timeout for slower tests.
> +set timeout [expr $gdb_test_timeout + 120]
>  test_all_threads "slow with kill breakpoint" 1
> +set timeout $gdb_test_timeout
>
>  return 0
>

I disagree.
There are no existing uses of gdb_test_timeout outside of lib/gdb.exp,
and regardless, I prefer my patch.  "timeout" is a standard global variable
.exp writers know about.  Adding gdb_test_timeout is one more global,
one more bit of context .exp writers are expected to know about.  I
think it's fine as a mechanism to ensure a stable and consistent value
at the start of the test, but that his hidden by lib/gdb.exp routines.
 Once I'm in a testsuite .exp file, I'd rather not have to involve it
in my hacking.

Re: [patch] print-threads.exp: Extend timeout for slower tests.

[Jan Kratochvil]

On Tue, 08 Nov 2011 09:11:31 +0100, Doug Evans wrote:
> There are no existing uses of gdb_test_timeout outside of lib/gdb.exp,
> and regardless, I prefer my patch.  "timeout" is a standard global variable
> .exp writers know about.  Adding gdb_test_timeout is one more global,
...
>  Once I'm in a testsuite .exp file, I'd rather not have to involve it
> in my hacking.

Every GDB .exp has to heavily depend on lib/gdb.exp.
And for new GDB contributors TCL is exactly as new as lib/gdb.exp.

I see it was not misunderstanding but rather a real disagreement.
I do not mind, this is just bikeshedding, other patches need more reviews.


Thanks,
Jan