Re: [PATCHv2 0/3] Automatic detection of test name problems

[Keith Seitz <keiths@redhat.com>]

On 4/29/20 8:38 AM, Andrew Burgess wrote:
> * Simon Marchi <simark@simark.ca> [2020-04-29 11:04:40 -0400]:
> 
>> When you detect an offender, do you think you could print something?  A bit like a "FAIL"
>> is printed when a test fails.  For example:
>>
>>   DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5
>>
>> That would make it easier to spot the problematic test(s).  But even without that, your
>> patchset looks good and useful to me.

That is a great idea, Simon. [Again -- why didn't I think of that?]

> However, I chose to print "DUPLICATE: ...." for _every_ duplicate, so
> you would get this:
> 
>   PASS: foo
>   PASS: foo
>   DUPLICATE: foo
>   PASS: foo
>   DUPLICATE: foo
>   PASS: bar
>   PASS: bar
>   DUPLICATE: bar
>   PASS: bar
>   DUPLICATE: bar
> 
> Obviously we don't get a DUPLICATE message for the first 'foo', or the
> first 'bar', we can't know by that point that these are going to be
> duplicated.
> 
> But, it might be confusing that this test will report 2 duplicates,
> but contain 4 DUPLICATE lines.  Would it in fact be better to report a
> count of 4 in this case I wonder?

I don't really care one way or the other. This is an exceptional case, and
is /far/ less offensive than, e.g., assertions aborting gdb. Seeing messages
just reinforces the message IMO. YMMV.

Just one or two Tcl comments. Of course. ;-)

> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> index 15c5544c8e4..8b525897ba3 100644
> --- a/gdb/testsuite/lib/check-test-names.exp
> +++ b/gdb/testsuite/lib/check-test-names.exp
> @@ -43,24 +43,27 @@ namespace eval ::CheckTestNames {
>  	incr counts($type,total)
>      }
>  
> -    # 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.
> -    #
> -    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> -    # incremented.
> -    proc check { message } {
> +    # Check if MESSAGE contains a build or source path, if it does increment
> +    # the relevant counter and return 1, otherwise, return 0.
> +    proc _check_paths { message } {
>  	global srcdir objdir
> -	variable all_test_names
>  
>  	foreach path [list $srcdir $objdir] {
>  	    if { [ string first $path $message ] >= 0 } {
>  		# Count each test just once.
>  		inc_count paths
> -		return
> +		return 1
>  	    }
>  	}
>  
> +	return 0
> +    }

Tcl supports booleans. This could return true/false.

> +
> +    # Check if MESSAGE is a duplicate, if it is then increment the
> +    # duplicates counter and return 1, otherwise, return 0.
> +    proc _check_duplicates { message } {
> +	variable all_test_names
> +
>  	# Initialise a count, or increment the count for this test name.
>  	if {![info exists all_test_names($message)]} {
>  	    set all_test_names($message) 0
> @@ -69,6 +72,43 @@ namespace eval ::CheckTestNames {
>  		inc_count duplicates
>  	    }
>  	    incr all_test_names($message)
> +	    return 1
> +	}
> +
> +	return 0
> +    }
> +
> +    # Remove the leading Dejagnu status marker from MESSAGE, and return the
> +    # remainder of MESSAGE.  A status marker is something like 'PASS: '.  If
> +    # no status marker is found, then just return MESSAGE unmodified.
> +    proc _strip_status { message } {
> +	foreach status {PASS FAIL XFAIL KFAIL XPASS KPASS UNRESOLVED \
> +			    UNTESTED UNSUPPORTED} {
> +	    set txt "${status}: "
> +	    if { [string compare -length [string len $txt] \
> +		      $txt $message] == 0 } {
> +		return  [string range $message [string len $txt] end]

Maybe it doesn't matter, but we know /all/ test result messages from DejaGNU are
going to start "STATUS: $result_message". Would it be worth simply looking for ": "
from the start of the string?

That would eliminate the loop here and greatly simplify this. It is, of course, less
precise than the above.

Keith

> +	    }
> +	}
> +
> +	return $message
> +    }
> +
> +    # 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.
> +    #
> +    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> +    # incremented.
> +    proc check { message } {
> +	set message [ _strip_status $message ]
> +
> +	if [ _check_paths $message ] {
> +	    clone_output "PATH: $message"
> +	}
> +
> +	if [ _check_duplicates $message ] {
> +	    clone_output "DUPLICATE: $message"
>  	}
>      }
>  
>