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

[Luis Machado <luis.machado@linaro.org>]

On 5/26/20 12:35 PM, Pedro Alves wrote:
> 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:
> 

Heh, I seem to have forgotten that particular rule, and it is tempting 
to use it. Fixed now.

>   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.
It doesn't, but maybe it is due to the space before the parentheses?

> 
>>       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
> 

Makes sense. I've fixed these and patched them up to use $gdb_test_name now.

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

Fixed now.

>> +++ 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.

Fixed. Though I did not try to fix the testcase itself, which allows the 
command to be output as the test name, and those contain parentheses. 
I'd need to go over it and find meaningful names for the tests.

> 
>> +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.
> 

Fixed.

>> +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".
> 

Fixed.

>> @@ -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" {
> ...
> }

I've switched to using with_test_prefix.

I'll send a v2 shortly.

Thanks!