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

Keith Seitz
2020-04-29T16:03:42Z

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