diff mbox series

gcc.misc-tests/outputs.exp: assert unique test-names

Message ID 20210225051725.D642F203B3@pchp3.se.axis.com
State New
Headers show
Series gcc.misc-tests/outputs.exp: assert unique test-names | expand

Commit Message

Hans-Peter Nilsson Feb. 25, 2021, 5:17 a.m. UTC 
The gcc.misc-tests/outputs.exp tests can take some effort to
digest.

Navigating and debugging causes for failing tests here isn't
helped by the existence of tests with duplicate names.
Let's stop that from happening.  This requires that test-run
output is actually reviewed, as Tcl errors don't stop the
test-run, but then again there's no such dejagnu construct
that I know of.

Tested x86_64-pc-linux-gnu.

Ok to commit?  Or is a renaming patch appending a
number-suffix, like:

Comments

Jeff Law March 2, 2021, 10:39 p.m. UTC | #1 
On 2/24/21 10:17 PM, Hans-Peter Nilsson via Gcc-patches wrote:
> The gcc.misc-tests/outputs.exp tests can take some effort to
> digest.
>
> Navigating and debugging causes for failing tests here isn't
> helped by the existence of tests with duplicate names.
> Let's stop that from happening.  This requires that test-run
> output is actually reviewed, as Tcl errors don't stop the
> test-run, but then again there's no such dejagnu construct
> that I know of.
>
> Tested x86_64-pc-linux-gnu.
>
> Ok to commit?  Or is a renaming patch appending a
> number-suffix, like:
>
> --- outputs.exp.orig3	2021-02-25 06:13:28.304243791 +0100
> +++ outputs.exp	2021-02-25 06:13:51.575457825 +0100
> @@ -280,8 +280,8 @@ if { "$aout" != "" } then {
>  }
>  
>  # Driver-chosen outputs.
> -outest "$b asm default 1" $sing "-S" {} {{-0.s}}
> -outest "$b asm default 2" $mult "-S" {} {{-1.s -2.s}}
> +outest "$b-1 asm default 1" $sing "-S" {} {{-0.s}}
> +outest "$b-2 asm default 2" $mult "-S" {} {{-1.s -2.s}}
> ...
>
> ...better and ok to commit?  (IMHO: yes, much easier to follow)
>
> gcc/testsuite:
> 	* gcc.misc-tests/outputs.exp: Append discriminating
> 	suffixes to tests with duplicate names.
> 	(outest): Assert that each running test has a unique
> 	name.
OK.  And I think that in general changes which fix duplicated testnames
should be considered as not needing approval.  Just fix 'em and post the
patch for the historical record :-)

jeff
Hans-Peter Nilsson March 2, 2021, 10:58 p.m. UTC | #2 
> From: Jeff Law <jeffreyalaw@gmail.com>
> Date: Tue, 2 Mar 2021 23:39:27 +0100
> received-spf: None (smtp1.axis.com: no sender authenticity  information
>  available from domain of  postmaster@mail-il1-f172.google.com) identity=helo;
>   client-ip=209.85.166.172; receiver=smtp1.axis.com;
>   envelope-from="jeffreyalaw@gmail.com";
>   x-sender="postmaster@mail-il1-f172.google.com";
>   x-conformance=sidf_compatible
> Old-Content-Type: multipart/alternative;
> 	boundary="_000_08d773b4a6cd248fc44aa1877542afabgmailcom_"
> Content-Type: text/plain; charset=iso-8859-1
> 
> 
> 
> On 2/24/21 10:17 PM, Hans-Peter Nilsson via Gcc-patches wrote:
> > The gcc.misc-tests/outputs.exp tests can take some effort to
> > digest.
> >
> > Navigating and debugging causes for failing tests here isn't
> > helped by the existence of tests with duplicate names.
> > Let's stop that from happening.  This requires that test-run
> > output is actually reviewed, as Tcl errors don't stop the
> > test-run, but then again there's no such dejagnu construct
> > that I know of.
> >
> > Tested x86_64-pc-linux-gnu.
> >
> > Ok to commit?  Or is a renaming patch appending a
> > number-suffix, like:
> >
> > --- outputs.exp.orig3	2021-02-25 06:13:28.304243791 +0100
> > +++ outputs.exp	2021-02-25 06:13:51.575457825 +0100
> > @@ -280,8 +280,8 @@ if { "$aout" != "" } then {
> >  }
> >  
> >  # Driver-chosen outputs.
> > -outest "$b asm default 1" $sing "-S" {} {{-0.s}}
> > -outest "$b asm default 2" $mult "-S" {} {{-1.s -2.s}}
> > +outest "$b-1 asm default 1" $sing "-S" {} {{-0.s}}
> > +outest "$b-2 asm default 2" $mult "-S" {} {{-1.s -2.s}}
> > ...
> >
> > ...better and ok to commit?  (IMHO: yes, much easier to follow)
> >
> > gcc/testsuite:
> > 	* gcc.misc-tests/outputs.exp: Append discriminating
> > 	suffixes to tests with duplicate names.
> > 	(outest): Assert that each running test has a unique
> > 	name.
> OK.  And I think that in general changes which fix duplicated testnames
> should be considered as not needing approval.  Just fix 'em and post the
> patch for the historical record :-)

Ok, thanks for the review!

Since there were two versions suggested and (to me) the "OK"
was ambiguous, I'll go ahead with the suggested (but
scripted) renaming of e.g. "$b blah", "$b foo", "$b blah
with futz", "$b frob" (etc) into "$b-1 blah", "$b-2 foo",
"$b-3 blah with futz", "$b-4 frob" (etc) ;-) But, I'm going
to wait 24h.

brgds, H-P
Alexandre Oliva March 3, 2021, 1:12 p.m. UTC | #3 
On Feb 25, 2021, Hans-Peter Nilsson <hp@axis.com> wrote:

> Navigating and debugging causes for failing tests here isn't
> helped by the existence of tests with duplicate names.

Aah, I guess I see what happened: some test sets were copied to cover
additional cases I hadn't covered (cool :-), but the test names were not
changed.

I wonder if we wouldn't be better off using say $bp instead of $b in
test name strings, so that bp could be set to "$b whatever" or "$b"
before a group of tests.

This would make it easier to copy blocks of tests, but it would make it
harder to search for a failing test.

I suppose the changes you proposed, along with code that rejects
duplicates, is preferrable:

> gcc/testsuite:
> 	* gcc.misc-tests/outputs.exp: Append discriminating
> 	suffixes to tests with duplicate names.
> 	(outest): Assert that each running test has a unique
> 	name.

LGTM, thanks!
diff mbox series

Patch

--- outputs.exp.orig3	2021-02-25 06:13:28.304243791 +0100
+++ outputs.exp	2021-02-25 06:13:51.575457825 +0100
@@ -280,8 +280,8 @@  if { "$aout" != "" } then {
 }
 
 # Driver-chosen outputs.
-outest "$b asm default 1" $sing "-S" {} {{-0.s}}
-outest "$b asm default 2" $mult "-S" {} {{-1.s -2.s}}
+outest "$b-1 asm default 1" $sing "-S" {} {{-0.s}}
+outest "$b-2 asm default 2" $mult "-S" {} {{-1.s -2.s}}
...

...better and ok to commit?  (IMHO: yes, much easier to follow)

gcc/testsuite:
	* gcc.misc-tests/outputs.exp: Append discriminating
	suffixes to tests with duplicate names.
	(outest): Assert that each running test has a unique
	name.
---
 gcc/testsuite/gcc.misc-tests/outputs.exp | 36 +++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/gcc/testsuite/gcc.misc-tests/outputs.exp b/gcc/testsuite/gcc.misc-tests/outputs.exp
index ebd61448bfdd..0e5c1a55ce87 100644
--- a/gcc/testsuite/gcc.misc-tests/outputs.exp
+++ b/gcc/testsuite/gcc.misc-tests/outputs.exp
@@ -116,8 +116,16 @@  if [info exists env(MAKEFLAGS)] {
 # it weren't for
 # https://core.tcl-lang.org/tcl/tktview?name=5bbd044812), but .{i,s,o}
 # and .[iso] will pass even if only the .o is present.
+array unset outests *
 proc outest { test sources opts dirs outputs } {
     global b srcdir subdir
+    global outests
+
+    if { [info exists outests($test)] } {
+	error "multiple outputs.exp tests are named \"$test\", but for sanity, test-names must be unique"
+    }
+    set outests($test) 1
+
     set src {}
     foreach s $sources {
 	lappend src $srcdir/$subdir/$b$s
@@ -307,10 +315,10 @@  outest "$b exe savetmp named2" $mult "-o $b.exe -save-temps" {} {{--1.i --1.s --
 
 # Additional files are created when an @file is used
 if !$skip_atsave {
-outest "$b exe savetmp namedb" $sing "@/dev/null -o $b.exe -save-temps" {} {{--0.i --0.s --0.o .args.0 !!$gld .ld1_args !0 .exe}}
-outest "$b exe savetmp named2" $mult "@/dev/null -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o .args.0 !!$gld .ld1_args !0 .exe}}
-outest "$b exe savetmp named2" $mult "@/dev/null -I dummy -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o -args.0 -args.1 .args.2 !!$gld .ld1_args !0 .exe}}
-outest "$b exe savetmp named2" $mult "@/dev/null -I dummy -L dummy -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o -args.0 -args.1 .args.2 .args.3 !!$gld .ld1_args !0 .exe}}
+outest "$b exe savetmp namedb-2" $sing "@/dev/null -o $b.exe -save-temps" {} {{--0.i --0.s --0.o .args.0 !!$gld .ld1_args !0 .exe}}
+outest "$b exe savetmp named2-2" $mult "@/dev/null -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o .args.0 !!$gld .ld1_args !0 .exe}}
+outest "$b exe savetmp named2-3" $mult "@/dev/null -I dummy -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o -args.0 -args.1 .args.2 !!$gld .ld1_args !0 .exe}}
+outest "$b exe savetmp named2-4" $mult "@/dev/null -I dummy -L dummy -o $b.exe -save-temps" {} {{--1.i --1.s --1.o --2.i --2.s --2.o -args.0 -args.1 .args.2 .args.3 !!$gld .ld1_args !0 .exe}}
 }
 
 # Setting the main output to a dir selects it as the default aux&dump
@@ -400,9 +408,9 @@  outest "$b exe soddovr namedir2" $mult "-o o/$b.exe -save-temps=obj -dumpdir ./"
 outest "$b exe scddovr namedir0" $sing "-o o/$b-0.exe -save-temps=cwd -dumpdir o/" {o/} {{-0.i -0.s -0.o -0.exe} {}}
 outest "$b exe scddovr namedirb" $sing "-o o/$b.exe -save-temps=cwd -dumpdir o/" {o/} {{-0.i -0.s -0.o .exe} {}}
 outest "$b exe scddovr namedir2" $mult "-o o/$b.exe -save-temps=cwd -dumpdir o/" {o/} {{-1.i -1.s -1.o -2.i -2.s -2.o .exe} {}}
-outest "$b exe ddstovr namedir0" $sing "-o $b-0.exe -save-temps -dumpdir o/" {o/} {{-0.i -0.s -0.o} {-0.exe}}
-outest "$b exe ddstovr namedirb" $sing "-o $b.exe -save-temps -dumpdir o/" {o/} {{-0.i -0.s -0.o} {.exe}}
-outest "$b exe ddstovr namedir2" $mult "-o $b.exe -save-temps -dumpdir o/" {o/} {{-1.i -1.s -1.o -2.i -2.s -2.o} {.exe}}
+outest "$b exe ddstovr namedir0-2" $sing "-o $b-0.exe -save-temps -dumpdir o/" {o/} {{-0.i -0.s -0.o} {-0.exe}}
+outest "$b exe ddstovr namedirb-2" $sing "-o $b.exe -save-temps -dumpdir o/" {o/} {{-0.i -0.s -0.o} {.exe}}
+outest "$b exe ddstovr namedir2-2" $mult "-o $b.exe -save-temps -dumpdir o/" {o/} {{-1.i -1.s -1.o -2.i -2.s -2.o} {.exe}}
 
 
 # Compiler- and driver-generated aux and dump outputs.
@@ -661,14 +669,14 @@  outest "$b lto mult nameddir" $mult "-o dir/$b.exe -O2 -flto -flto-partition=one
 
 if $ltop {
 # -flto -fno-use-linker-plugin
-outest "$b lto sing unnamed" $sing "-O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage $oaout" {} {{a--0.c.???i.icf a--0.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su $aout}}
-outest "$b lto mult unnamed" $mult "-O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage $oaout" {} {{a--1.c.???i.icf a--1.c.???r.final a--2.c.???i.icf a--2.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su $aout}}
-outest "$b lto sing named" $sing "-o $b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {} {{--0.c.???i.icf --0.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe}}
-outest "$b lto mult named" $mult "-o $b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {} {{--1.c.???i.icf --1.c.???r.final --2.c.???i.icf --2.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe}}
-outest "$b lto sing nameddir" $sing "-o dir/$b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {dir/} {{--0.c.???i.icf --0.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe} {}}
-outest "$b lto mult nameddir" $mult "-o dir/$b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {dir/} {{--1.c.???i.icf --1.c.???r.final --2.c.???i.icf --2.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe} {}}
+outest "$b lto sing unnamed-2" $sing "-O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage $oaout" {} {{a--0.c.???i.icf a--0.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su $aout}}
+outest "$b lto mult unnamed-2" $mult "-O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage $oaout" {} {{a--1.c.???i.icf a--1.c.???r.final a--2.c.???i.icf a--2.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su $aout}}
+outest "$b lto sing named-2" $sing "-o $b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {} {{--0.c.???i.icf --0.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe}}
+outest "$b lto mult named-2" $mult "-o $b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {} {{--1.c.???i.icf --1.c.???r.final --2.c.???i.icf --2.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe}}
+outest "$b lto sing nameddir-2" $sing "-o dir/$b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {dir/} {{--0.c.???i.icf --0.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe} {}}
+outest "$b lto mult nameddir-2" $mult "-o dir/$b.exe -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage" {dir/} {{--1.c.???i.icf --1.c.???r.final --2.c.???i.icf --2.c.???r.final .wpa.???i.icf .ltrans0.ltrans.???r.final .ltrans0.ltrans.su .exe} {}}
 if !$skip_atsave {
-outest "$b lto sing unnamed" $sing "@/dev/null -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage -save-temps $oaout" {} {{a--0.c.???i.icf a--0.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su a--0.o a--0.s a--0.i a.ltrans0.o a.ltrans.out a.ltrans0.ltrans.o a.ltrans0.ltrans_args a.args.0 a.ltrans0.ltrans.s a.wpa.args.0 a.lto_args a.ld1_args a.ltrans_args a.ltrans0.ltrans.args.0 a.ld_args $aout}}
+outest "$b lto sing unnamed-3" $sing "@/dev/null -O2 -flto -fno-use-linker-plugin -flto-partition=one -fdump-ipa-icf-optimized -fdump-rtl-final -fstack-usage -save-temps $oaout" {} {{a--0.c.???i.icf a--0.c.???r.final a.wpa.???i.icf a.ltrans0.ltrans.???r.final a.ltrans0.ltrans.su a--0.o a--0.s a--0.i a.ltrans0.o a.ltrans.out a.ltrans0.ltrans.o a.ltrans0.ltrans_args a.args.0 a.ltrans0.ltrans.s a.wpa.args.0 a.lto_args a.ld1_args a.ltrans_args a.ltrans0.ltrans.args.0 a.ld_args $aout}}
 }
 }