Re: [PATCH 35/40] Comprehensive C++ linespec/completer tests

[Keith Seitz <keiths@redhat.com>]

On 06/02/2017 05:22 AM, Pedro Alves wrote:
> Exercises all sorts of aspects fixed by the previous patches.

SUPER!

> Grows the gdb.linespec/ tests like this:
> 
>   -# of expected passes           573
>   +# of expected passes           4458

/me drools

> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.linespec/cpcompletion.exp: New file.
> 	* gdb.linespec/cpls-hyphen.cc: New file.
> 	* gdb.linespec/cpls.cc: New file.
> 	* gdb.linespec/cpls2.cc: New file.
> 	* gdb.linespec/explicit.exp: Load completion-support.exp.  Adjust
> 	test to use test_gdb_complete_unique.  Add label completion,
> 	keyword completion and explicit location completion tests.
> 	* lib/completion-support.exp: New file.

> diff --git a/gdb/testsuite/gdb.linespec/cpls-hyphen.cc b/gdb/testsuite/gdb.linespec/cpls-hyphen.cc
> new file mode 100644
> index 0000000..fdc063f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.linespec/cpls-hyphen.cc
> @@ -0,0 +1,14 @@
> +int
> +ns_hyphen_function (int i)
> +{
> +  if (i > 0)
> +    {
> +    label1:
> +      return i + 20;
> +    }
> +  else
> +    {
> +    label2:
> +      return i + 10;
> +    }
> +}

Does this file not require a copyright header?

> diff --git a/gdb/testsuite/gdb.linespec/explicit.exp b/gdb/testsuite/gdb.linespec/explicit.exp
> index 65d78ca..998b70a 100644
> --- a/gdb/testsuite/gdb.linespec/explicit.exp
> +++ b/gdb/testsuite/gdb.linespec/explicit.exp
> @@ -326,10 +329,202 @@ namespace eval $testfile {
[snip]
>  
> +	# Follows completion tests that require having no symbols
> +	# loaded.

"The following completion tests," perhaps?

> +	gdb_exit
> +	gdb_start
> +
> +	# The match list you get when you complete with no options
> +	# specified at all.
> +	set completion_list {
> +	    "-function"
> +	    "-label"
> +	    "-line"
> +	    "-probe"
> +	    "-probe-dtrace"
> +	    "-probe-stap"
> +	    "-qualified"
> +	    "-source"
> +	}
> +	with_test_prefix "complete with no arguments and no symbols" {
> +	    test_gdb_complete_multiple "b " "" "-" $completion_list
> +	    test_gdb_complete_multiple "b " "-" "" $completion_list
> +	}
>      }
>      # End of completion tests.
>  
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> new file mode 100644
> index 0000000..ef78269
> --- /dev/null
> +++ b/gdb/testsuite/lib/completion-support.exp
> @@ -0,0 +1,513 @@
[snip]
> +
> +# Test that completing INPUT_LINE with TAB completes to
> +# COMPLETE_LINE_RE.  APPEND_CHAR_RE is the character expected to be
> +# appended after EXPECTED_OUTPUT.  Normally that's a whitespace, but
> +# in some cases it's some other character, like a colon.
> +
> +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"
> +    gdb_test_multiple "" "$test" {
> +	-re "^$complete_line_re$append_char_re$" {
> +	    pass "$test"
> +	}
> +    }
> +
> +    clear_input_line $test
> +}
> +
> +# Test that completing INPUT_LINE with TAB completes to "INPUT_LINE +
> +# ADD_COMPLETED_LINE" and that is displays the completion matches in
                                  ^^

s/is/it/

> +# COMPLETION_LIST.
> +

> +proc test_gdb_complete_tab_multiple { input_line add_completed_line \
> +					  completion_list } {
> +    global gdb_prompt
> +    global bell_re
[snip]
> +
> +proc test_gdb_complete_menu { line expected_output } {
> +

Is this used? Maybe in a subsequent patch?

> +    set test "menu complete $line"
> +#    send_gdb "$expr\033?"
> +#    send_gdb "$expr^\[?"
> +    send_gdb "$expr"
> +    send_gdb "\x1b"
> +    send_gdb "?"
> +    gdb_test_multiple "" "$test" {
> +	-re "$expected_output" {
> +	    pass "$test"
> +	}
> +    }
> +}
> +
[snip]
> +
> +proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "} {max_completions 0}} {

This procedure is only called from this file. If it is meant to be called by
test writers, it deserves a comment, even if trivial.

If it is only meant for use here, then it should be named differently to
differentiate it, or stick it into an appropriately namespace so that it is
more obvious to potential users of this code that it is not meant to be
"exported." There's a lot of (sometimes mindless) cut-n-paste going on in the
test suite.

> +    set append_char_re [string_to_regexp $append_char]
> +    test_gdb_complete_tab_unique $input_line $complete_line_re $append_char_re
> +
> +    # Trim INPUT_LINE and COMPLETE LINE, for the case we're completing
> +    # a command with leading whitespace.  Leading command whitespace
> +    # is discarded by GDB.
> +    set input_line [string trimleft $input_line]
> +    set expected_output_re [string trimleft $complete_line_re]
> +    if {$append_char_re != " "} {
> +	append expected_output_re $append_char_re
> +    }
> +    if {$max_completions} {
> +	set max_completion_reached_msg \
> +	    "*** List may be truncated, max-completions reached. ***"
> +	set input_line_re \
> +	    [string_to_regexp $input_line]
> +	set max_completion_reached_msg_re \
> +	    [string_to_regexp $max_completion_reached_msg]
> +
> +	append expected_output_re \
> +	    "\r\n$input_line_re $max_completion_reached_msg_re"
> +    }
> +
> +    test_gdb_complete_cmd_unique $input_line $expected_output_re
> +}
[snip]
> +
> +# Test completing all the substring prefixes of COMPLETION from
> +# [0..START) to [0..END) complete to COMPLETION.  If END is ommitted,
> +# default to the length of COMPLETION.
> +
> +proc test_complete_prefix_range {completion start {end -1}} {
> +    if {$end == -1} {
> +	set end [string length $completion]
> +    }
> +
> +    for {set i $start} {$i < $end} {incr i} {
> +	set line [string range $completion 0 $i]
> +	test_gdb_complete_unique "$line" "$completion"
> +    }
> +}
> +
> +proc test_complete_prefix_range_input {input completion_re start {end -1}} {

This procedure also isn't used. Maybe also a later patch? Ditto the "if it
is meant to be used, ..., or renamed/hidden."

> +    if {$end == -1} {
> +	set end [string length $input]
> +    }
> +
> +    for {set i $start} {$i < $end} {incr i} {
> +	set line [string range $input 0 $i]
> +	test_gdb_complete_unique_re "$line" $completion_re
> +    }
> +}
> +
[snip]
> +# Return true if lists A and B have the same elements.  Order of
> +# elements does not matter.
> +
> +proc gdb_leq {a b} {
> +    return [expr {[lsort $a] eq [lsort $b]}]
> +}
> +
> +# Check that creating the breakpoint at LINESPEC finds the same
> +# breakpoint locations as completing LINESPEC.  COMPLETION_LIST is
> +# expected completion match list.

You mention "LINESPEC" here, but this procedure actually takes a breakpoint
command and a linepsec (or location?), no?

> +
> +proc check_bp_locations_match_list {break_command completion_list} {
> +    global gdb_prompt
> +    global hex
> +
> +    with_test_prefix "compare \"$break_command\" completion list with bp location list" {
> +	set num_locations [create_bp $break_command]
> +
> +	set found_list ""
> +
> +	set any "\[^\r\n\]*"
> +
> +	gdb_test_multiple "info breakpoint \$bpnum" "info breakpoint" {
> +	    -re "in \(\[^\r\n\]*\) at " {
> +		# A function location.
> +		set found_location "$expect_out(1,string)"
> +		lappend found_list $found_location
> +		exp_continue
> +	    }
> +	    -re "breakpoint${any}keep${any}y${any}$hex\[ \t]*\(${any}\)\r\n" {
> +		# A label location.
> +		set found_location "$expect_out(1,string)"
> +		lappend found_list $found_location
> +		exp_continue
> +	    }
> +	    -re "$gdb_prompt $" {
> +	    }
> +	}
> +
> +	gdb_assert {[gdb_leq $found_list $completion_list]} "matches"
> +
> +	delete_breakpoints
> +    }
> +}
> +
[snip]

Keith