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

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2020-04-29 11:04:40 -0400]:

> On 2020-04-29 5:02 a.m., Andrew Burgess wrote:
> > * Keith Seitz <keiths@redhat.com> [2020-04-28 12:08:25 -0700]:
> > 
> >> On 4/27/20 3:01 PM, Andrew Burgess wrote:
> >>> Changes since v1:
> >>>
> >>>   1. Original patch #1 is now merged.
> >>>
> >>>   2. Functionality is now placed inside a namespace.
> >>>   
> >>>   3. Better counting for multi-variant test runs, this is inline with
> >>>      how Dejagnu's muti-variant result counting works.
> >>>
> >>>   4. Use 'string first' instead of 'regexp'.
> >>>
> >>>   5. Reworded commit message on what is now patch #2.
> >>>
> >>> Further feedback welcome.
> >>
> >> Wow, that's almost a complete rewrite! While not what I was really
> >> suggesting, I have to admit, a /big/ smile came to my face while
> >> reading this. It is so nicely done!
> >>
> >> Thank you!
> >>
> >> Keith
> >>
> >> PS. As you know, IANAM, but you are, so I encourage you to approve
> >> your patch. ;-)
> > 
> > Thanks for your feedback, especially your TCL suggestions.
> > 
> > I'll let the patch sit for a while to see if anyone else has any
> > input.
> > 
> > Thanks,
> > Andrew
> > 
> 
> Hi Andrew,
> 
> Thanks for doing this, it's nice.  And by testing this patchset, I've found an offender
> and sent a patch for it :)
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-April/168052.html
> 
> 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.

Ask and you shall receive!

Below is a patch that applies on top of the existing series just to
try.  If you like this I'll fold this back into the relevant patches.

We now print "PATH: ...." or "DUPLICATE: ...." when we see an
offending test.

On printing DUPLICATE, things are a little .... odd.  I currently
count the number of unique duplicates, so:

  PASS: foo
  PASS: foo
  PASS: foo
  PASS: bar
  PASS: bar
  PASS: bar

Will give a duplicate count of '2', that is 'foo' and 'bar' are
duplicated.

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?

Thoughts welcome.

Thanks,
Andrew

---

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
+    }
+
+    # 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]
+	    }
+	}
+
+	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"
 	}
     }