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

Keith Seitz
2020-04-23T20:28:15Z

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