* [PATCH][gdb/testsuite] Check for valid test name @ 2021-09-13 7:19 Tom de Vries via Gdb-patches 2021-09-13 14:26 ` Andrew Burgess 0 siblings, 1 reply; 3+ messages in thread From: Tom de Vries via Gdb-patches @ 2021-09-13 7:19 UTC (permalink / raw) To: gdb-patches Hi, When running gdb.base/batch-exit-status.exp I noticed that the test name contains a newline: ... PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M : No such file or directory\.: [lindex $result 2] == 0 ... Check for this in ::CheckTestNames::check, such that we have a warning: ... PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M : No such file or directory\.: [lindex $result 2] == 0 WARNING: Newline in test name ... Tested on x86_64-linux. Any comments? Thanks, - Tom [gdb/testsuite] Check for valid test name --- gdb/testsuite/lib/check-test-names.exp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp index 1de7624d5c5..db76abab990 100644 --- a/gdb/testsuite/lib/check-test-names.exp +++ b/gdb/testsuite/lib/check-test-names.exp @@ -93,6 +93,13 @@ namespace eval ::CheckTestNames { return $message } + # Check if MESSAGE is a well-formed test name. + proc _check_name { message } { + if { [regexp \n $message]} { + warning "Newline in test name" + } + } + # 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. @@ -110,6 +117,8 @@ namespace eval ::CheckTestNames { if [ _check_duplicates $message ] { clone_output "DUPLICATE: $message" } + + _check_name $message } # If COUNT is greater than zero, disply PREFIX followed by COUNT. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][gdb/testsuite] Check for valid test name 2021-09-13 7:19 [PATCH][gdb/testsuite] Check for valid test name Tom de Vries via Gdb-patches @ 2021-09-13 14:26 ` Andrew Burgess 2021-09-13 18:37 ` Tom de Vries via Gdb-patches 0 siblings, 1 reply; 3+ messages in thread From: Andrew Burgess @ 2021-09-13 14:26 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-09-13 09:19:11 +0200]: > Hi, > > When running gdb.base/batch-exit-status.exp I noticed that the test name > contains a newline: > ... > PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M > : No such file or directory\.: [lindex $result 2] == 0 > ... > > Check for this in ::CheckTestNames::check, such that we have a warning: > ... > PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M > : No such file or directory\.: [lindex $result 2] == 0 > WARNING: Newline in test name > ... > > Tested on x86_64-linux. > > Any comments? > > Thanks, > - Tom > > [gdb/testsuite] Check for valid test name > > --- > gdb/testsuite/lib/check-test-names.exp | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp > index 1de7624d5c5..db76abab990 100644 > --- a/gdb/testsuite/lib/check-test-names.exp > +++ b/gdb/testsuite/lib/check-test-names.exp > @@ -93,6 +93,13 @@ namespace eval ::CheckTestNames { > return $message > } > > + # Check if MESSAGE is a well-formed test name. > + proc _check_name { message } { I think `_check_newline` would be a better name. > + if { [regexp \n $message]} { > + warning "Newline in test name" > + } If I run the entire testsuite I guess these warnings will likely get lost in the output, unlike the DUPLICATE/PATH warnings, which are listed along with the other PASS/FAIL/etc results. Did you check to see if this warning occurs any other times in the testsuite right now? I guess if we're clean right now then having this as a warning is fine, as we should spot this when adding new tests. I'm happy with this though (with the name change above). Thanks, Andrew > + } > + > # 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. > @@ -110,6 +117,8 @@ namespace eval ::CheckTestNames { > if [ _check_duplicates $message ] { > clone_output "DUPLICATE: $message" > } > + > + _check_name $message > } > > # If COUNT is greater than zero, disply PREFIX followed by COUNT. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][gdb/testsuite] Check for valid test name 2021-09-13 14:26 ` Andrew Burgess @ 2021-09-13 18:37 ` Tom de Vries via Gdb-patches 0 siblings, 0 replies; 3+ messages in thread From: Tom de Vries via Gdb-patches @ 2021-09-13 18:37 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 9/13/21 4:26 PM, Andrew Burgess wrote: > * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-09-13 09:19:11 +0200]: > >> Hi, >> >> When running gdb.base/batch-exit-status.exp I noticed that the test name >> contains a newline: >> ... >> PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M >> : No such file or directory\.: [lindex $result 2] == 0 >> ... >> >> Check for this in ::CheckTestNames::check, such that we have a warning: >> ... >> PASS: gdb.base/batch-exit-status.exp: : No such file or directory\.^M >> : No such file or directory\.: [lindex $result 2] == 0 >> WARNING: Newline in test name >> ... >> >> Tested on x86_64-linux. >> >> Any comments? >> >> Thanks, >> - Tom >> >> [gdb/testsuite] Check for valid test name >> >> --- >> gdb/testsuite/lib/check-test-names.exp | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp >> index 1de7624d5c5..db76abab990 100644 >> --- a/gdb/testsuite/lib/check-test-names.exp >> +++ b/gdb/testsuite/lib/check-test-names.exp >> @@ -93,6 +93,13 @@ namespace eval ::CheckTestNames { >> return $message >> } >> >> + # Check if MESSAGE is a well-formed test name. >> + proc _check_name { message } { > > I think `_check_newline` would be a better name. > >> + if { [regexp \n $message]} { >> + warning "Newline in test name" >> + } > Hi Andrew, thanks for the review. What I tried to do here is pick a proc name that: - describes what the current implementation is doing, but - is still broad enough to not only describe what the implementation is doing, such that other checks could be added to it without having to change the name of the proc. The name _check_newline fails to achieve the latter goal. So, I've renamed to "_check_well_formed_name", perhaps that's more clear than '_check_name'. > If I run the entire testsuite I guess these warnings will likely get > lost in the output, unlike the DUPLICATE/PATH warnings, which are > listed along with the other PASS/FAIL/etc results. > We can trigger the warning like this: ... diff --git a/gdb/testsuite/gdb.base/batch-exit-status.exp b/gdb/testsuite/gdb.base/batch-ex it-status.exp index 520083f7989..a55e92acf5f 100644 --- a/gdb/testsuite/gdb.base/batch-exit-status.exp +++ b/gdb/testsuite/gdb.base/batch-exit-status.exp @@ -94,3 +94,5 @@ set no_such_re ": $test\\." test_exit_status 1 "-batch \"\"" "1x: $test" ^[multi_line $no_such_re ""]$ test_exit_status 1 "-batch \"\" \"\"" "2x: $test" \ ^[multi_line $no_such_re $no_such_re ""]$ + +pass "bla\nbla" ... In the output I see: ... Running batch-exit-status.exp ... WARNING: Newline in test name ... In the gdb.sum, I see: ... PASS: gdb.base/batch-exit-status.exp: bla bla WARNING: Newline in test name ... And in the gdb.log, I see: ... PASS: gdb.base/batch-exit-status.exp: bla bla WARNING: Newline in test name ... So AFAIU these warnings will not get lost in the output. > Did you check to see if this warning occurs any other times in the > testsuite right now? I guess if we're clean right now then having > this as a warning is fine, as we should spot this when adding new > tests. > Yes, AFAIK we're clean, I'll do a retest and verify that, and then commit. Thanks, - Tom > I'm happy with this though (with the name change above). > > Thanks, > Andrew > > >> + } >> + >> # 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. >> @@ -110,6 +117,8 @@ namespace eval ::CheckTestNames { >> if [ _check_duplicates $message ] { >> clone_output "DUPLICATE: $message" >> } >> + >> + _check_name $message >> } >> >> # If COUNT is greater than zero, disply PREFIX followed by COUNT. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-13 18:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-13 7:19 [PATCH][gdb/testsuite] Check for valid test name Tom de Vries via Gdb-patches 2021-09-13 14:26 ` Andrew Burgess 2021-09-13 18:37 ` Tom de Vries via Gdb-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox