[v5] gdb/testsuite: fix completion tests when using READ1

  The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
regressions when testing using the READ1 mechanism. The reason for that
is the new failure path in proc test_gdb_complete_tab_unique, which
looks for GDB suggesting more than what the test inputted, but not the
correct answer, followed by a white space. Consider the following case:

int foo(int bar, int baz);

Sending the command "break foo<tab>" to GDB will return

break foo(int, int)

which easily fits the buffer in normal testing, so everything works, but
when reading one character at a time, the test will find the partial
"break foo(int, " and assume that there was a mistake, so we get a
spurious FAIL.

That change was added because we wanted to avoid forcing a completion
failure to fail through timeout, which it had to do because there is no
way to verify that the output is done, mostly because when I was trying
to solve a different problem I kept getting reading errors and testing
completion was frustrating.

This commit implements a better way to avoid that frustration, by first
testing gdb's complete command and only if that passes we will test tab
completion. The difference is that when testing with the complete
command, we can tell when the output is over when we receive the GDB
prompt again, so we don't need to rely on timeouts. With this, the
change to test_gdb_complete_tab_unique has been removed as that test
will only be run and fail in the very unlikely scenario that tab
completion is different than command completion.
---
 gdb/testsuite/lib/completion-support.exp | 37 ++++++++++++++++--------
 1 file changed, 25 insertions(+), 12 deletions(-)
  

Guinevere Larsen <blarsen@redhat.com> writes:

> The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
> regressions when testing using the READ1 mechanism. The reason for that
> is the new failure path in proc test_gdb_complete_tab_unique, which
> looks for GDB suggesting more than what the test inputted, but not the
> correct answer, followed by a white space. Consider the following case:
>
> int foo(int bar, int baz);
>
> Sending the command "break foo<tab>" to GDB will return
>
> break foo(int, int)
>
> which easily fits the buffer in normal testing, so everything works, but
> when reading one character at a time, the test will find the partial
> "break foo(int, " and assume that there was a mistake, so we get a
> spurious FAIL.
>
> That change was added because we wanted to avoid forcing a completion
> failure to fail through timeout, which it had to do because there is no
> way to verify that the output is done, mostly because when I was trying
> to solve a different problem I kept getting reading errors and testing
> completion was frustrating.
>
> This commit implements a better way to avoid that frustration, by first
> testing gdb's complete command and only if that passes we will test tab
> completion. The difference is that when testing with the complete
> command, we can tell when the output is over when we receive the GDB
> prompt again, so we don't need to rely on timeouts. With this, the
> change to test_gdb_complete_tab_unique has been removed as that test
> will only be run and fail in the very unlikely scenario that tab
> completion is different than command completion.
> ---
>  gdb/testsuite/lib/completion-support.exp | 37 ++++++++++++++++--------
>  1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..16598aa5a6c 100644
> --- a/gdb/testsuite/lib/completion-support.exp
> +++ b/gdb/testsuite/lib/completion-support.exp
> @@ -111,15 +111,11 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
>  
>      set test "tab complete \"$input_line\""
>      send_gdb "$input_line\t"
> -    set partial_complete [string_to_regexp $input_line]
>      set res 1
>      gdb_test_multiple "" "$test" {
>  	-re "^$complete_line_re$append_char_re$" {
>  	    pass "$test"
>  	}
> -	-re "$partial_complete\[^ \]+ $" {
> -	    fail "$test"
> -	}
>  	timeout {
>  	    fail "$test (timeout)"
>  	    set res -1
> @@ -190,21 +186,29 @@ proc test_gdb_complete_cmd_none { line } {
>  
>  # Test that completing LINE with the complete command completes to
>  # COMPLETE_LINE_RE.
> +# Returns 1 if the test passed, 0 if it failed, -1 if it timed out.
>  
>  proc test_gdb_complete_cmd_unique { input_line complete_line_re } {
>      global gdb_prompt
>  
> +    set res 0
>      set cmd "complete $input_line"
>      set cmd_re [string_to_regexp $cmd]
>      set test "cmd complete \"$input_line\""
>      gdb_test_multiple $cmd $test {
>  	-re "^$cmd_re\r\n$complete_line_re\r\n$gdb_prompt $" {
>  	    pass $test
> +	    set res 1
>  	}
>  	-re "$gdb_prompt $" {
>  	    fail "$test"
>  	}
> +	timeout {
> +	    fail "$test (timeout)"
> +	    set res -1
> +	}
>      }
> +    return $res
>  }
>  
>  # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +267,6 @@ proc test_gdb_complete_none { input_line } {
>  
>  proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "} {max_completions 0}} {
>      set append_char_re [string_to_regexp $append_char]
> -    if { [readline_is_used] } {
> -	if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
> -		  $append_char_re] == -1 } {
> -	    return -1
> -	}
> -    }
>  
>      # Trim COMPLETE LINE, for the case we're completing a command with leading
>      # whitespace.  Leading command whitespace is discarded by GDB.
> @@ -288,8 +286,23 @@ proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "}
>  	    "\r\n$input_line_re $max_completion_reached_msg_re"
>      }
>  
> -    test_gdb_complete_cmd_unique $input_line $expected_output_re
> -    return 1
> +    # First test completion with the command, then with tab.
> +    # It is done in this order because cmd_complete knows when the output
> +    # from GDB is over, so it can fail without requiring a timeout, which
> +    # speeds up testing if necessary.
> +
> +    set test_result [test_gdb_complete_cmd_unique $input_line\
> +		$expected_output_re]
> +    if { $test_result != 1 } {
> +	return $test_result
> +    }
> +
> +    if { [readline_is_used] } {
> +	set test_result [test_gdb_complete_tab_unique $input_line \
> +		$complete_line_re $append_char_re]
> +	}
> +    }

You've added two '}' here instead of one.  The first pairs with:

  if { [readline_is_used] } {

while the second actually closes the function.  As a result...

> +    return $test_result

... this is interpreted at file scope.

I tested this patch with:

  make check-gdb TESTS="gdb.*/*complete*.exp"

and you'll notice a few tests failing as a result.

However, I think the rest of this patch is fine.  If you fix this issue
and run at least the set of tests I gave above with no problems, then I
think you can go ahead and check this in without reposting.

Approved-By: Andrew Burgess <aburgess@redhat.com>

FYI: When using read1 I still see some timeouts with the test
gdb.ada/complete.exp, but these exist even without this patch.  I have a
fix for this, which I'll post soon.

Thanks,
Andrew

>  }
>  
>  # Like TEST_GDB_COMPLETE_UNIQUE_RE, but COMPLETE_LINE is a string, not
> -- 
> 2.41.0
  

On 29/11/2023 16:25, Andrew Burgess wrote:
> Guinevere Larsen <blarsen@redhat.com> writes:
>
>> The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
>> regressions when testing using the READ1 mechanism. The reason for that
>> is the new failure path in proc test_gdb_complete_tab_unique, which
>> looks for GDB suggesting more than what the test inputted, but not the
>> correct answer, followed by a white space. Consider the following case:
>>
>> int foo(int bar, int baz);
>>
>> Sending the command "break foo<tab>" to GDB will return
>>
>> break foo(int, int)
>>
>> which easily fits the buffer in normal testing, so everything works, but
>> when reading one character at a time, the test will find the partial
>> "break foo(int, " and assume that there was a mistake, so we get a
>> spurious FAIL.
>>
>> That change was added because we wanted to avoid forcing a completion
>> failure to fail through timeout, which it had to do because there is no
>> way to verify that the output is done, mostly because when I was trying
>> to solve a different problem I kept getting reading errors and testing
>> completion was frustrating.
>>
>> This commit implements a better way to avoid that frustration, by first
>> testing gdb's complete command and only if that passes we will test tab
>> completion. The difference is that when testing with the complete
>> command, we can tell when the output is over when we receive the GDB
>> prompt again, so we don't need to rely on timeouts. With this, the
>> change to test_gdb_complete_tab_unique has been removed as that test
>> will only be run and fail in the very unlikely scenario that tab
>> completion is different than command completion.
>> ---
>>   gdb/testsuite/lib/completion-support.exp | 37 ++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 12 deletions(-)
>>
>> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
>> index fdc512838c3..16598aa5a6c 100644
>> --- a/gdb/testsuite/lib/completion-support.exp
>> +++ b/gdb/testsuite/lib/completion-support.exp
>> @@ -111,15 +111,11 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
>>   
>>       set test "tab complete \"$input_line\""
>>       send_gdb "$input_line\t"
>> -    set partial_complete [string_to_regexp $input_line]
>>       set res 1
>>       gdb_test_multiple "" "$test" {
>>   	-re "^$complete_line_re$append_char_re$" {
>>   	    pass "$test"
>>   	}
>> -	-re "$partial_complete\[^ \]+ $" {
>> -	    fail "$test"
>> -	}
>>   	timeout {
>>   	    fail "$test (timeout)"
>>   	    set res -1
>> @@ -190,21 +186,29 @@ proc test_gdb_complete_cmd_none { line } {
>>   
>>   # Test that completing LINE with the complete command completes to
>>   # COMPLETE_LINE_RE.
>> +# Returns 1 if the test passed, 0 if it failed, -1 if it timed out.
>>   
>>   proc test_gdb_complete_cmd_unique { input_line complete_line_re } {
>>       global gdb_prompt
>>   
>> +    set res 0
>>       set cmd "complete $input_line"
>>       set cmd_re [string_to_regexp $cmd]
>>       set test "cmd complete \"$input_line\""
>>       gdb_test_multiple $cmd $test {
>>   	-re "^$cmd_re\r\n$complete_line_re\r\n$gdb_prompt $" {
>>   	    pass $test
>> +	    set res 1
>>   	}
>>   	-re "$gdb_prompt $" {
>>   	    fail "$test"
>>   	}
>> +	timeout {
>> +	    fail "$test (timeout)"
>> +	    set res -1
>> +	}
>>       }
>> +    return $res
>>   }
>>   
>>   # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
>> @@ -263,12 +267,6 @@ proc test_gdb_complete_none { input_line } {
>>   
>>   proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "} {max_completions 0}} {
>>       set append_char_re [string_to_regexp $append_char]
>> -    if { [readline_is_used] } {
>> -	if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
>> -		  $append_char_re] == -1 } {
>> -	    return -1
>> -	}
>> -    }
>>   
>>       # Trim COMPLETE LINE, for the case we're completing a command with leading
>>       # whitespace.  Leading command whitespace is discarded by GDB.
>> @@ -288,8 +286,23 @@ proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "}
>>   	    "\r\n$input_line_re $max_completion_reached_msg_re"
>>       }
>>   
>> -    test_gdb_complete_cmd_unique $input_line $expected_output_re
>> -    return 1
>> +    # First test completion with the command, then with tab.
>> +    # It is done in this order because cmd_complete knows when the output
>> +    # from GDB is over, so it can fail without requiring a timeout, which
>> +    # speeds up testing if necessary.
>> +
>> +    set test_result [test_gdb_complete_cmd_unique $input_line\
>> +		$expected_output_re]
>> +    if { $test_result != 1 } {
>> +	return $test_result
>> +    }
>> +
>> +    if { [readline_is_used] } {
>> +	set test_result [test_gdb_complete_tab_unique $input_line \
>> +		$complete_line_re $append_char_re]
>> +	}
>> +    }
> You've added two '}' here instead of one.  The first pairs with:
>
>    if { [readline_is_used] } {
>
> while the second actually closes the function.  As a result...
>
>> +    return $test_result
> ... this is interpreted at file scope.
>
> I tested this patch with:
>
>    make check-gdb TESTS="gdb.*/*complete*.exp"
>
> and you'll notice a few tests failing as a result.

oops, sorry about forgetting to rerun the tests. I fixed what you 
mentioned, and ran the test (though I used gdb.*/*complet*.exp to also 
get "completion" stuff) and I only get the ada failure that wasn't 
introduced by my patch.

Thanks for the review, I've pushed this patch!