Re: [PATCH] gdb.base/commands.exp: fix racy test (PR testsuite/12649)

[Jan Kratochvil <jan.kratochvil@redhat.com>]

Hi Marek,

On Wed, 13 Apr 2011 09:58:15 +0200, Marek Polacek wrote:
> The problem is with the ".*" part which just slurps everything,

in normal case, while with the following reproducer it does not slurp
everything which then falsely matches
proc gdb_test_multiple { command message user_code } {
        -re "\\((y or n|y or \\\[n\\\]|\\\[y\\\] or n)\\) " {
            fail "$message (got interactive prompt)"


> when we use preloaded read() which returns just one char at a time.

The problem reproducer in PR testsuite/12649 has no been mentioned here.


> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2011-04-13  Marek Polacek <mpolacek@redhat.com>
                           ^^^ two spaces here

The ChangeLog entry should be posted as plain text before the final commit, for
example now it already no longer applies to FSF GDB HEAD now.


> +	* gdb.base/commands.exp (redefine_backtrace_test): Fix race.

The description should be more detailed.  For example you do not mention you
have created a new testcase `expect response to define backtrace'.



> --- a/gdb/testsuite/gdb.base/commands.exp
> +++ b/gdb/testsuite/gdb.base/commands.exp
> @@ -704,18 +704,22 @@ proc redefine_backtrace_test {} {
>      global gdb_prompt
>  
>      gdb_test_multiple "define backtrace" "define backtrace" {
> -	-re "Really redefine built-in.*$" {
> +	-re "Really redefine built-in command \"backtrace\"\\? \\(y or n\\) $"  {
>  	    send_gdb "y\n"

Optional FYI: This send_gdb could be moved below into gdb_test_multiple:
    gdb_test_multiple "y" "expect response to define backtrace" {

But please do a PASS call here for better troubleshooting in the future:
	    pass "define backtrace"

> -	    exp_continue
>  	}
> +    }
>  
> -	-re "End with"  {
> +    # We send nothing this time.
> +    gdb_test_multiple "" "expect response to define backtrace" {
> +	-re "End with a line saying just \"end\".\r\n>$"  {

nitpick - `.' is a metacharacter here, it could be escaped:
   	-re "End with a line saying just \"end\"\\.\r\n>$"  {

>  	    pass "define backtrace in redefine_backtrace_test"

This test has name "expect response to define backtrace" which gets used
during various default FAIL cases by gdb_test_multiple.  The PASS name should
match the possible FAIL name, therefore rather some:

            pass "expect response to define backtrace"

>  	}
> -        default {
> +
> +	default {
>  	    fail "(timeout or eof) define backtrace in redefine_backtrace_test"
>  	}

This is redundant for gdb_test_multiple now but it is in fact unrelated
change.

I would prefer one re-post before the approval.


Thanks,
Jan