Re: [PATCH] Add testcases for set&show backtrace and copying

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

Thanks for the patch!

> Add testcases to gdb.base/setshow.exp as follows:
>     *set&show backtrace limit
>     *set&show backtrace past-entry
>     *set&show backtrace past-main
>     *show copying

This change is missing a ChangeLog. Also, would you mind adding
a space after each '*' in your list?

> ---
>  gdb/testsuite/gdb.base/setshow.exp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/setshow.exp b/gdb/testsuite/gdb.base/setshow.exp
> index d8295e2..502d008 100644
> --- a/gdb/testsuite/gdb.base/setshow.exp
> +++ b/gdb/testsuite/gdb.base/setshow.exp
> @@ -227,6 +227,26 @@ gdb_test_no_output "set listsize 100" "set listsize 100"
>  #test show listsize 100
>  gdb_test "show listsize" "Number of source lines gdb will list by default is 100..*" "show listsize (100)" 
>  
> +#test set backtrace limit 3
> +gdb_test_no_output "set backtrace limit 3"

I think it's fairly obvious that you're testing "set backtrace limit 3",
so the comment is not really useful on its own. The same comment pretty
much applies to all the tests that you added.

What I would do is just remove all the added comments unless you think
there is something to explain about why you added this test, and/or
what's tricky about it, etc.

On Mon, Oct 26, 2015 at 12:16:31PM +0800, Fei Jie wrote:
> +#test show backtrace limit
> +gdb_test "show backtrace limit" \
> +"An upper bound on the number of backtrace levels is 3\."
> +#test set backtrace past-entry on
> +gdb_test_no_output "set backtrace past-entry on"
> +#test show backtrace past-entry
> +gdb_test "show backtrace past-entry" \
> +"Whether backtraces should continue past the entry point of a program is on\."

Usually, we indent continuation lines by 4 characters (in .exp files).

gdb_test "show backtrace past-entry"
    "Whether backtraces hould continue past the entry point.* is on\."

(I propose you relax a bit the expression, making shorter at the same
time, since it's really necessary to check the exact wording, only
that we have reasonable expectations that the command is working -
I might even suggest only checking "Whether.* is on\.", so that, if
we were to change the wording, we wouldn't have to dupliate it in
that test).

> +#test set backtrace past-main on
> +gdb_test_no_output "set backtrace past-main on"
> +#test show backtrace past-main
> +gdb_test "show backtrace past-main" \
> +"Whether backtraces should continue past \"main\" is on\."

Same remarks as above.

> +send_gdb "set backtrace limit 0\n"
> +send_gdb "set backtrace past-entry off\n"
> +send_gdb "set backtrace past-main off\n"

Please do not use send_gdb. In fact, let may say it again: the use
of send_gdb is absolutely and utterly forbidden in testcases unless
we are absolutely and 150% sure that there is no other way. I think
we document in the GDB testsuite cookbook (in the GDB wiki) why that is.

> +#test show copying
> +gdb_test "show copying" ".*GNU GENERAL PUBLIC LICENSE.*"

I think you can remove the leading ".*" (implicit)

>  if ![board_info target exists gdb_prompt] {
>      #test set prompt (FooBarBaz) 
>      set newprompt "\\(FooBarBaz\\)"
> -- 
> 1.8.3.1

Thanks!
-- 
Joel