Re: [RFA_v3 6/8] Add a test for 'frame apply'

[Pedro Alves <palves@redhat.com>]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:
> Add a test for 'frame apply'

Very nice.  This looks mostly OK, though there's a couple details
that need sorting out.  See below.

> diff --git a/gdb/testsuite/gdb.base/frameapply.exp b/gdb/testsuite/gdb.base/frameapply.exp
> new file mode 100644
> index 0000000000..1911e15b72
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frameapply.exp
> @@ -0,0 +1,179 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +# Test 'frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND'
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile}] } {
> +    return -1
> +}
> +
> +clean_restart ${binfile}
> +
> +
> +if ![runto setup_done] then {
> +    gdb_suppress_tests
> +}

gdb_suppress_tests is a really old mechanism that we shouldn't
be adding new uses of.  Make that call fail and return immediately,
instead of continuing.

> +
> +set any "\[^\r\n\]*"
> +set ws "\[ \t\]\+"
> +set number "\[0-9]\+"
> +
> +
> +# check all | COUNT | -COUNT | id ID... with a simple command
> +foreach cmd {
> +    "frame apply all p /x 20"
> +    "frame apply id 0-5 p /x 20"
> +    "frame apply id 0-3 4-5 p /x 20"
> +    "frame apply id 0 1-2 3-5 p /x 20"
> +    "frame apply id 0 1 2 3 4 5 p /x 20"
> +    "frame apply id 0-0 1-1 2-2 3-3 4-4 5-5 p /x 20" } {
> +    gdb_test $cmd \
> +    [multi_line \
> +	 "#0${ws}setup_done ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#1${ws}${any} f1 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#2${ws}${any} f2 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#3${ws}${any} f3 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#4${ws}${any} f4 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#5${ws}${any} main ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	] \
> +	"Run a simple command on all frames"

lowercase "Run" -> "run".  Likewise throughout.

> +# check quietness
> +foreach cmd {
> +    "frame apply 2 p /x 20"
> +    "frame apply id 0-1 p /x 20"} {

I noticed that the testcase produces duplicated test names in gdb.sum.
Take a look here:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

You can fix this with e.g., foreach_with_prefix.  I'd suggest
wrapping the different stages of the testcase with
with_test_prefix as well, for visual guidance when browsing
gdb.sum results though that's less important.

Something like this:

# Check quietness.
with_test_prefix "quietness" {
  foreach_with_prefix frames {"2" "id 0-1"} {

    set cmd "frame apply $frames p /x 20"

    gdb_test $cmd \
        [multi_line \
	     "#0${ws}setup_done ${any}" \
	     "\\\$\[0-9]+ = 0x14${any}" \
	     "#1${ws}${any} f1 ${any}" \
	     "\\\$\[0-9]+ = 0x14${any}" \
	    ] \
	"run a command, printing location"
...

  }
}

(Note, "Check quietness." with uppercase and period.  Might as
well write complete sentences while at it.)

The same comments apply to patch #7, so I won't repeat them there.

Thanks,
Pedro Alves