* [PING][PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
@ 2023-09-14 13:02 ` Guinevere Larsen
2023-10-13 15:26 ` [PINGv2][PATCH " Guinevere Larsen
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-09-14 13:02 UTC (permalink / raw)
To: Guinevere Larsen, Bruno Larsen via Gdb-patches
Ping!
--
Cheers,
Guinevere Larsen
She/Her/Hers
On 28/08/2023 13:30, Guinevere Larsen wrote:
> 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 | 31 +++++++++++++++---------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..56c0eee8e1d 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
> @@ -194,17 +190,21 @@ proc test_gdb_complete_cmd_none { line } {
> 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"
> + set res -1
> }
> }
> + return $res
> }
>
> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +263,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,7 +282,22 @@ 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
> + # 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.
> +
> + if { [test_gdb_complete_cmd_unique $input_line\
> + $expected_output_re] == -1 } {
> + return -1
> + }
> +
> + if { [readline_is_used] } {
> + if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
> + $append_char_re] == -1 } {
> + return -1
> + }
> + }
> return 1
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PINGv2][PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
2023-09-14 13:02 ` [PING][PATCH " Guinevere Larsen
@ 2023-10-13 15:26 ` Guinevere Larsen
2023-10-16 23:48 ` [PATCH " Thiago Jung Bauermann
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-10-13 15:26 UTC (permalink / raw)
To: Guinevere Larsen, Bruno Larsen via Gdb-patches
Ping!
--
Cheers,
Guinevere Larsen
She/Her/Hers
On 28/08/2023 13:30, Guinevere Larsen wrote:
> 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 | 31 +++++++++++++++---------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..56c0eee8e1d 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
> @@ -194,17 +190,21 @@ proc test_gdb_complete_cmd_none { line } {
> 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"
> + set res -1
> }
> }
> + return $res
> }
>
> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +263,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,7 +282,22 @@ 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
> + # 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.
> +
> + if { [test_gdb_complete_cmd_unique $input_line\
> + $expected_output_re] == -1 } {
> + return -1
> + }
> +
> + if { [readline_is_used] } {
> + if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
> + $append_char_re] == -1 } {
> + return -1
> + }
> + }
> return 1
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
2023-09-14 13:02 ` [PING][PATCH " Guinevere Larsen
2023-10-13 15:26 ` [PINGv2][PATCH " Guinevere Larsen
@ 2023-10-16 23:48 ` Thiago Jung Bauermann
2023-10-24 15:58 ` [Pingv3] " Guinevere Larsen
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Thiago Jung Bauermann @ 2023-10-16 23:48 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
Hello Guinevere,
guinevere Larsen via Gdb-patches <gdb-patches@sourceware.org> 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 | 31 +++++++++++++++---------
> 1 file changed, 20 insertions(+), 11 deletions(-)
Great! I was seeing 422 FAILs in gdb.linespec/cpcompletion.exp, and also
1 FAIL in gdb.cp/cpcompletion.exp when using check-read1. With this
patch all tests pass in both testcases, so:
Tested-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Also, the code looks good to me:
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
--
Thiago
^ permalink raw reply [flat|nested] 14+ messages in thread* [Pingv3] [PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
` (2 preceding siblings ...)
2023-10-16 23:48 ` [PATCH " Thiago Jung Bauermann
@ 2023-10-24 15:58 ` Guinevere Larsen
2023-11-07 13:02 ` [Ping v4] " Guinevere Larsen
2023-11-07 13:47 ` Andrew Burgess
2023-11-08 16:56 ` [PATCH v4] " Guinevere Larsen
5 siblings, 1 reply; 14+ messages in thread
From: Guinevere Larsen @ 2023-10-24 15:58 UTC (permalink / raw)
To: Gdb-patches, Guinevere Larsen
Ping!
--
Cheers,
Guinevere Larsen
She/Her/Hers
On 28/08/2023 13:30, Guinevere Larsen wrote:
> 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 | 31 +++++++++++++++---------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..56c0eee8e1d 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
> @@ -194,17 +190,21 @@ proc test_gdb_complete_cmd_none { line } {
> 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"
> + set res -1
> }
> }
> + return $res
> }
>
> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +263,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,7 +282,22 @@ 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
> + # 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.
> +
> + if { [test_gdb_complete_cmd_unique $input_line\
> + $expected_output_re] == -1 } {
> + return -1
> + }
> +
> + if { [readline_is_used] } {
> + if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
> + $append_char_re] == -1 } {
> + return -1
> + }
> + }
> return 1
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
` (3 preceding siblings ...)
2023-10-24 15:58 ` [Pingv3] " Guinevere Larsen
@ 2023-11-07 13:47 ` Andrew Burgess
2023-11-08 16:10 ` Guinevere Larsen
2023-11-08 16:56 ` [PATCH v4] " Guinevere Larsen
5 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2023-11-07 13:47 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
Guinevere Larsen via Gdb-patches <gdb-patches@sourceware.org> 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 | 31 +++++++++++++++---------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..56c0eee8e1d 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
> @@ -194,17 +190,21 @@ proc test_gdb_complete_cmd_none { line } {
> 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"
> + set res -1
> }
> }
> + return $res
> }
>
> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +263,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,7 +282,22 @@ 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
> + # 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.
> +
> + if { [test_gdb_complete_cmd_unique $input_line\
> + $expected_output_re] == -1 } {
> + return -1
> + }
This doesn't seem right. Or at least, it's not really inline with how
the return value from this proc is used.
Check out `test_complete_prefix_range_re`. In here you can see that a
return value of -1 is used to indicate that a test in
test_gdb_complete_unique_re timedout, while here you're using it to
indicate general failure.
I would suggest that `test_gdb_complete_cmd_unique` should simply return
true/false to indicate if the test passed or failed (for any reason).
Then in `test_gdb_complete_unique_re` you can just:
if { ![test_gdb_complete_cmd_unique $input_line\
$expected_output_re] } {
return 0
}
The users of `test_gdb_complete_unique_re` can then continue to see -1
as the timeout indicator.
Of course, you could have test_gdb_complete_cmd_unique return -1 on
timeout, but I suspect a timeout here is much less likely, so maybe the
extra complexity of catching this case isn't worth the effort? I don't
think I'd bother, but you might feel different.
Thanks,
Andrew
> +
> + if { [readline_is_used] } {
> + if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
> + $append_char_re] == -1 } {
> + return -1
> + }
> + }
> return 1
> }
>
> --
> 2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3] gdb/testsuite: fix completion tests when using READ1
2023-11-07 13:47 ` Andrew Burgess
@ 2023-11-08 16:10 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-11-08 16:10 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 07/11/2023 14:47, Andrew Burgess wrote:
> Guinevere Larsen via Gdb-patches <gdb-patches@sourceware.org> 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 | 31 +++++++++++++++---------
>> 1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
>> index fdc512838c3..56c0eee8e1d 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
>> @@ -194,17 +190,21 @@ proc test_gdb_complete_cmd_none { line } {
>> 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"
>> + set res -1
>> }
>> }
>> + return $res
>> }
>>
>> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
>> @@ -263,12 +263,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,7 +282,22 @@ 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
>> + # 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.
>> +
>> + if { [test_gdb_complete_cmd_unique $input_line\
>> + $expected_output_re] == -1 } {
>> + return -1
>> + }
> This doesn't seem right. Or at least, it's not really inline with how
> the return value from this proc is used.
>
> Check out `test_complete_prefix_range_re`. In here you can see that a
> return value of -1 is used to indicate that a test in
> test_gdb_complete_unique_re timedout, while here you're using it to
> indicate general failure.
That's fair, I should have looked around before deciding.
>
> I would suggest that `test_gdb_complete_cmd_unique` should simply return
> true/false to indicate if the test passed or failed (for any reason).
> Then in `test_gdb_complete_unique_re` you can just:
>
> if { ![test_gdb_complete_cmd_unique $input_line\
> $expected_output_re] } {
> return 0
> }
>
> The users of `test_gdb_complete_unique_re` can then continue to see -1
> as the timeout indicator.
>
> Of course, you could have test_gdb_complete_cmd_unique return -1 on
> timeout, but I suspect a timeout here is much less likely, so maybe the
> extra complexity of catching this case isn't worth the effort? I don't
> think I'd bother, but you might feel different.
I think its easy enough to implement the timeout warning, and this way
its there if anyone wants to use it. I'll do this on v4
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> Andrew
>
>
>
>
>
>
>> +
>> + if { [readline_is_used] } {
>> + if { [test_gdb_complete_tab_unique $input_line $complete_line_re \
>> + $append_char_re] == -1 } {
>> + return -1
>> + }
>> + }
>> return 1
>> }
>>
>> --
>> 2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] gdb/testsuite: fix completion tests when using READ1
2023-08-28 11:30 [PATCH v3] gdb/testsuite: fix completion tests when using READ1 Guinevere Larsen
` (4 preceding siblings ...)
2023-11-07 13:47 ` Andrew Burgess
@ 2023-11-08 16:56 ` Guinevere Larsen
2023-11-13 11:28 ` Andrew Burgess
2023-11-22 9:44 ` [PATCH v5] " Guinevere Larsen
5 siblings, 2 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-11-08 16:56 UTC (permalink / raw)
To: gdb-patches; +Cc: Guinevere Larsen
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 | 32 ++++++++++++++++--------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index fdc512838c3..cc34ef51e22 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,26 @@ 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 -1
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"
+ set res 0
}
}
+ return $res
}
# Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
@@ -263,12 +264,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,7 +283,22 @@ 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
+ # 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]
+ }
+ }
return 1
}
--
2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4] gdb/testsuite: fix completion tests when using READ1
2023-11-08 16:56 ` [PATCH v4] " Guinevere Larsen
@ 2023-11-13 11:28 ` Andrew Burgess
2023-11-14 10:40 ` Guinevere Larsen
2023-11-22 9:44 ` [PATCH v5] " Guinevere Larsen
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2023-11-13 11:28 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Guinevere Larsen
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 | 32 ++++++++++++++++--------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index fdc512838c3..cc34ef51e22 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,26 @@ 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 -1
> 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"
> + set res 0
> }
If you want to return -1 for timeout, then I would follow the pattern of
how test_gdb_complete_tab_unique does it. Inside the gdb_test_multiple,
add a new block:
timeout {
fail "$test (timeout)"
set res -1
}
Then you can initialise 'res' to 0 instead of -1, delete the 'set res 0'
for the existing fail case, and now, if any of the additional failure
cases that are added by calling gdb_test_multiple trigger, you'll get a
return value of 0. For the one passing case you'll get 1, and for the
one timeout case you'll get -1.
> }
> + return $res
> }
>
> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
> @@ -263,12 +264,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,7 +283,22 @@ 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
> + # 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]
> + }
This seems wrong. You probably mean:
if { [readline_is_used] } {
if { [test_gdb_complete_tab_unique $input_line \
$complete_line_re $append_char_re] == -1 } {
return -1
}
}
Otherwise, if test_gdb_complete_tab_unique times out -- the only timeout
we really care about -- you'll not return -1.
Thanks,
Andrew
> + }
> return 1
> }
>
> --
> 2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4] gdb/testsuite: fix completion tests when using READ1
2023-11-13 11:28 ` Andrew Burgess
@ 2023-11-14 10:40 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-11-14 10:40 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 13/11/2023 12:28, 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 | 32 ++++++++++++++++--------
>> 1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
>> index fdc512838c3..cc34ef51e22 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,26 @@ 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 -1
>> 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"
>> + set res 0
>> }
> If you want to return -1 for timeout, then I would follow the pattern of
> how test_gdb_complete_tab_unique does it. Inside the gdb_test_multiple,
> add a new block:
>
> timeout {
> fail "$test (timeout)"
> set res -1
> }
>
> Then you can initialise 'res' to 0 instead of -1, delete the 'set res 0'
> for the existing fail case, and now, if any of the additional failure
> cases that are added by calling gdb_test_multiple trigger, you'll get a
> return value of 0. For the one passing case you'll get 1, and for the
> one timeout case you'll get -1.
That's a good point. I always forget that there are more ways that
gdb_test_multiple can fail. I'll update this!
>
>> }
>> + return $res
>> }
>>
>> # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
>> @@ -263,12 +264,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,7 +283,22 @@ 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
>> + # 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]
>> + }
> This seems wrong. You probably mean:
>
> if { [readline_is_used] } {
> if { [test_gdb_complete_tab_unique $input_line \
> $complete_line_re $append_char_re] == -1 } {
> return -1
> }
> }
>
> Otherwise, if test_gdb_complete_tab_unique times out -- the only timeout
> we really care about -- you'll not return -1.
No we won't. if test_gdb_complete_cmd_unique returned -1, the previous
if condition will cause the function to exit early. This is expected to
always return 1, unless there is a difference in the results between tab
and cmd completion.
Also, since I'm removing the partial completion part, the only way for
that function to return 0 is if it hits one of the default failures from
gdb_test_multiple, so I would think that the rare cases where 0 is
returned should be captured too. Otherwise we might see an internal
error and still return that the test was successful.
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> Andrew
>
>> + }
>> return 1
>> }
>>
>> --
>> 2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5] gdb/testsuite: fix completion tests when using READ1
2023-11-08 16:56 ` [PATCH v4] " Guinevere Larsen
2023-11-13 11:28 ` Andrew Burgess
@ 2023-11-22 9:44 ` Guinevere Larsen
2023-11-29 15:25 ` Andrew Burgess
1 sibling, 1 reply; 14+ messages in thread
From: Guinevere Larsen @ 2023-11-22 9:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Guinevere Larsen
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]
+ }
+ }
+ return $test_result
}
# Like TEST_GDB_COMPLETE_UNIQUE_RE, but COMPLETE_LINE is a string, not
--
2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] gdb/testsuite: fix completion tests when using READ1
2023-11-22 9:44 ` [PATCH v5] " Guinevere Larsen
@ 2023-11-29 15:25 ` Andrew Burgess
2023-12-01 12:31 ` Guinevere Larsen
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2023-11-29 15:25 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Guinevere Larsen
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] gdb/testsuite: fix completion tests when using READ1
2023-11-29 15:25 ` Andrew Burgess
@ 2023-12-01 12:31 ` Guinevere Larsen
0 siblings, 0 replies; 14+ messages in thread
From: Guinevere Larsen @ 2023-12-01 12:31 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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!
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread