Re: [PATCH 3/4] gdb/testsuite: Detect and warn about duplicate test names

[Keith Seitz <keiths@redhat.com>]

On 4/23/20 10:53 AM, Andrew Burgess wrote:
> Spots when tests reuse the same test name and prints a new result type
> '# of duplicate test names' in the result summary.

The above reads awkwardly. Or I'm awkward. Take your pick. :-)

> Currently if the tests are run in parallel mode the new result type is
> not merged into the combined summary file so users will need to run in
> non-parallel mode to check this result.  A later commit will fix this.

I don't know how others approach testing, but when I add new/modify
tests, I always run them separately first without parallelization. This would
still catch any mistakes I might make.

So from my perspective, it's a still a win!

My only comment is a reiteration of the namespace wrapping I encouraged
in the last patch.

Keith

> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> index 6377ace3fc3..85520191f27 100644
> --- a/gdb/testsuite/lib/check-test-names.exp
> +++ b/gdb/testsuite/lib/check-test-names.exp
> @@ -22,6 +22,13 @@
>  # or the build path.
>  set paths_in_test_names 0
>  
> +# An associative array of all test names to the number of times each
> +# name is seen.  Used to detect duplicate test names.
> +array set all_test_names {}
> +
> +# A count of the number of duplicte test names seen.
> +set duplicate_test_names_seen 0
> +
>  # 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.
> @@ -31,6 +38,8 @@ set paths_in_test_names 0
>  proc check_test_names { message } {
>      global srcdir objdir
>      global paths_in_test_names
> +    global all_test_names
> +    global duplicate_test_names_seen
>  
>      foreach pattern [list $srcdir $objdir] {
>  	if { [ regexp $pattern $message ] } {
> @@ -39,6 +48,16 @@ proc check_test_names { message } {
>  	    return
>  	}
>      }
> +
> +    # Initialise a count, or increment the count for this test name.
> +    if {![info exists all_test_names($message)]} {
> +	set all_test_names($message) 0
> +    } else {
> +	if {$all_test_names($message) == 0} {
> +	    incr duplicate_test_names_seen
> +	}
> +	incr all_test_names($message)
> +    }
>  }
>  
>  # Arrange for CHECK_TEST_NAMES to be called.
> @@ -58,6 +77,7 @@ set local_record_procs(unsupported) "check_test_names"
>  rename log_summary original_log_summary
>  proc log_summary { args } {
>      global paths_in_test_names
> +    global duplicate_test_names_seen
>  
>      # If ARGS is the empty list then we don't want to pass a single
>      # empty string as a parameter here.
> @@ -66,6 +86,10 @@ proc log_summary { args } {
>      if { $paths_in_test_names > 0 } {
>  	clone_output "# of paths in test names\t${paths_in_test_names}"
>      }
> +
> +    if { $duplicate_test_names_seen > 0 } {
> +	clone_output "# of duplicate test names\t${duplicate_test_names_seen}"
> +    }
>  }
>  
>  # Wrapper around the global Dejagnu RESET_VARS procedure, resets our
> @@ -73,7 +97,12 @@ proc log_summary { args } {
>  rename reset_vars original_reset_vars
>  proc reset_vars {} {
>      global paths_in_test_names
> +    global all_test_names
> +    global duplicate_test_names_seen
>  
>      original_reset_vars
> +
>      set paths_in_test_names 0
> +    unset all_test_names
> +    set duplicate_test_names_seen 0
>  }
>