Re: [PATCH][gdb/testsuite] Check for valid test name

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 9/13/21 4:26 PM, Andrew Burgess wrote:
> * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-09-13 09:19:11 +0200]:
> 
>> Hi,
>>
>> When running gdb.base/batch-exit-status.exp I noticed that the test name
>> contains a newline:
>> ...
>> PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M
>> : No such file or directory\.: [lindex $result 2] == 0
>> ...
>>
>> Check for this in ::CheckTestNames::check, such that we have a warning:
>> ...
>> PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M
>> : No such file or directory\.: [lindex $result 2] == 0
>> WARNING: Newline in test name
>> ...
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>>
>> Thanks,
>> - Tom
>>
>> [gdb/testsuite] Check for valid test name
>>
>> ---
>>  gdb/testsuite/lib/check-test-names.exp | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
>> index 1de7624d5c5..db76abab990 100644
>> --- a/gdb/testsuite/lib/check-test-names.exp
>> +++ b/gdb/testsuite/lib/check-test-names.exp
>> @@ -93,6 +93,13 @@ namespace eval ::CheckTestNames {
>>  	return $message
>>      }
>>  
>> +    # Check if MESSAGE is a well-formed test name.
>> +    proc _check_name { message } {
> 
> I think `_check_newline` would be a better name.
> 
>> +	if { [regexp \n $message]} {
>> +	    warning "Newline in test name"
>> +	}
> 

Hi Andrew,

thanks for the review.

What I tried to do here is pick a proc name that:
- describes what the current implementation is doing, but
- is still broad enough to not only describe what the implementation is
  doing, such that other checks could be added to it without having to
  change the name of the proc.
The name _check_newline fails to achieve the latter goal.

So, I've renamed to "_check_well_formed_name", perhaps that's more clear
than '_check_name'.

> If I run the entire testsuite I guess these warnings will likely get
> lost in the output, unlike the DUPLICATE/PATH warnings, which are
> listed along with the other PASS/FAIL/etc results.
> 

We can trigger the warning like this:
...
diff --git a/gdb/testsuite/gdb.base/batch-exit-status.exp
b/gdb/testsuite/gdb.base/batch-ex
it-status.exp
index 520083f7989..a55e92acf5f 100644
--- a/gdb/testsuite/gdb.base/batch-exit-status.exp
+++ b/gdb/testsuite/gdb.base/batch-exit-status.exp
@@ -94,3 +94,5 @@ set no_such_re ": $test\\."
 test_exit_status 1 "-batch \"\"" "1x: $test" ^[multi_line $no_such_re ""]$
 test_exit_status 1 "-batch \"\" \"\"" "2x: $test" \
     ^[multi_line $no_such_re $no_such_re ""]$
+
+pass "bla\nbla"
...

In the output I see:
...
Running batch-exit-status.exp ...
WARNING: Newline in test name
...

In the gdb.sum, I see:
...
PASS: gdb.base/batch-exit-status.exp: bla
bla
WARNING: Newline in test name
...

And in the gdb.log, I see:
...
PASS: gdb.base/batch-exit-status.exp: bla
bla
WARNING: Newline in test name
...

So AFAIU these warnings will not get lost in the output.

> Did you check to see if this warning occurs any other times in the
> testsuite right now?  I guess if we're clean right now then having
> this as a warning is fine, as we should spot this when adding new
> tests.
> 

Yes, AFAIK we're clean, I'll do a retest and verify that, and then commit.

Thanks,
- Tom

> I'm happy with this though (with the name change above).
> 
> Thanks,
> Andrew
> 
> 
>> +    }
>> +
>>      # Check if MESSAGE contains either the source path or the build path.
>>      # This will result in test names that can't easily be compared between
>>      # different runs of GDB.
>> @@ -110,6 +117,8 @@ namespace eval ::CheckTestNames {
>>  	if [ _check_duplicates $message ] {
>>  	    clone_output "DUPLICATE: $message"
>>  	}
>> +
>> +	_check_name $message
>>      }
>>  
>>      # If COUNT is greater than zero, disply PREFIX followed by COUNT.