Re: [PATCH, testsuite] Fix some duplicate test names

[Pedro Alves <palves@redhat.com>]

Thanks!

Some comments as I quickly skimmed the patch.  Some of the issues
appear more than once, but I didn't point at all instances.

On 5/26/20 3:18 PM, Luis Machado via Gdb-patches wrote:
>  # Test that GDB manages caches correctly for tagged address.
>  # Read from P2,
> -gdb_test "x p2" "$hex:\[\t \]+0x000004d2"
> +gdb_test "x p2" "$hex:\[\t \]+0x000004d2" "x p2"
>  gdb_test_no_output "set variable i = 5678"
>  # Test that *P2 is updated.
> -gdb_test "x p2" "$hex:\[\t \]+0x0000162e"
> +gdb_test "x p2" "$hex:\[\t \]+0x0000162e" "x p2 (updated)"
>  

NAK here -- these should be considered the same.  See:

 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

If the detection machinery does not consider them duplicates, then it
should be fixed. 

>      gdb_test_multiple "break *test_ldr_literal_16" "break test_ldr_literal_16" {
>  	-re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" {
> -	    pass "break test_ldr_literal"
> +	    pass "break test_ldr_literal_16"
>  	}

Why not "break *break test_ldr_literal_16"?  I.e., the "*" is missing.

But consider $gdb_test_name, and dropping the explicit test name in
the gdb_test_multiple call too:

     gdb_test_multiple "break *test_ldr_literal_16" "" {
 	-re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" {
	    pass $gdb_test_name
 	}

>      gdb_test_multiple "break *test_zero_cbnz" "break test_zero_cbnz" {
>  	-re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" {
> -	    pass "break test_ldr_literal"
> +	    pass "break test_ldr_literal in test_zero_cbnz"
>  	}

This one shows why $gdb_test_name would be better -- here a fail caught
by gdb_test_multiple's internal patterns will issue a different message
compared to a PASS:

 FAIL: break test_zero_cbnz
 PASS: break test_ldr_literal in test_zero_cbnz

> -gdb_test "print x" "$decimal = 45"
> +gdb_test "print x" "$decimal = 45" "validate setting a globa, 2nd time"
>  

Typo: "globa".

> +++ b/gdb/testsuite/gdb.base/return2.exp
> @@ -50,7 +50,7 @@ proc return_1 { type } {
>      gdb_test "print ${type}_resultval == testval.${type}_testval" ".* = 1" \
>  	    "${type} value returned successfully"
>      gdb_test "print ${type}_resultval != ${type}_returnval" ".* = 1" \
> -	    "validate result value not equal to program return value"
> +	    "validate result value not equal to program return value (${type})"
>  }

That issue with tail parens again.

> +gdb_test "p foo1_3 (a)"  "Cannot resolve.*" \
> +	 "pointer to pointer of wrong type (a)"
> +gdb_test "p foo1_3 (bp)" "Cannot resolve.*" \
> +	 "pointer to pointer of wrong type (bp)"
>  gdb_test "p foo1_4 (bp)" "= 14"             "pointer to ancestor pointer"

Ditto.

> +with_test_prefix "Strict type checking on" {
> +    gdb_test "p foo1_type_check (123)" [format $error_str "foo1_type_check"]
> +    gdb_test "p foo2_type_check (0, 1)" [format $error_str "foo2_type_check"]

Lowercase "Strict".

> @@ -401,25 +401,25 @@ void do_frozen_tests ()
>    v1.nested.k = 9;
>    /*:
>      set_frozen V1 1
> -    mi_varobj_update * {} "update varobjs: nothing changed"
> -    mi_check_varobj_value V1.i 1 "check V1.i: 1"
> -    mi_check_varobj_value V1.nested.j 2 "check V1.nested.j: 2"
> -    mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3"    
> +    mi_varobj_update * {} "update varobjs: nothing changed, 2nd"
> +    mi_check_varobj_value V1.i 1 "check V1.i: 1, 2nd"
> +    mi_check_varobj_value V1.nested.j 2 "check V1.nested.j: 2, 2nd"
> +    mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3, 2nd"
>      # Check that explicit update for elements of structures
>      # works.
>      # Update v1.j
>      mi_varobj_update V1.nested.j {V1.nested.j} "update V1.nested.j"
> -    mi_check_varobj_value V1.i 1 "check V1.i: 1"
> -    mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8"
> -    mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3"    
> +    mi_check_varobj_value V1.i 1 "check V1.i: 1, 3rd"
> +    mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8, 1st"
> +    mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3, 3rd"
>      # Update v1.nested, check that children is updated.
>      mi_varobj_update V1.nested {V1.nested.k} "update V1.nested"
>      mi_check_varobj_value V1.i 1 "check V1.i: 1"
> -    mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8"
> +    mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8, 2nd"
>      mi_check_varobj_value V1.nested.k 9 "check V1.nested.k: 9"    
>      # Update v1.i
>      mi_varobj_update V1.i {V1.i} "update V1.i"
> -    mi_check_varobj_value V1.i 7 "check V1.i: 7"
> +    mi_check_varobj_value V1.i 7 "check V1.i: 7, 1st"

Here, instead of the counting, it would seem to me better to use
with_test_prefix, like

with_test_prefix "update *" {
...
}

with_test_prefix "update v1.j" {
...
}

with_test_prefix "v1.nested" {
...
}

Thanks,
Pedro Alves