* [PATCH] test-lint-duplicates: Only check for numbered test cases @ 2013-04-03 5:54 Torsten Bögershausen 2013-04-03 14:28 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Torsten Bögershausen @ 2013-04-03 5:54 UTC (permalink / raw) To: git; +Cc: tboegi Running make inside contrib/remote-helpers fails in "test-lint-duplicates" This was because the regexp checking for duplicate numbers strips everything after the first "-" in the filename, including the prefix. As a result, 2 pathnames like "xxxx/contrib/remote-helpers/test-bzr.sh" and "xxxx/contrib/remote-helpers/test-hg-bidi.sh" are both converted into "xxxx/contrib/remote", and reported as duplicate. Improve the regexp: Remove everything after tNNNN- (where X stand for a digit) Signed-off-by: Torsten Bögershausen <tboegi@web.de> --- t/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/Makefile b/t/Makefile index 1923cc1..f123d02 100644 --- a/t/Makefile +++ b/t/Makefile @@ -48,7 +48,7 @@ clean: clean-except-prove-cache test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax test-lint-duplicates: - @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \ + @dups=`echo $(T) | tr ' ' '\n' | sed -ne 's|\(.*/\)*t\([0-9][0-9][0-9][0-9]\)-.*|\2|p' | sort | uniq -d` && \ test -z "$$dups" || { \ echo >&2 "duplicate test numbers:" $$dups; exit 1; } -- 1.8.2.411.g65a544e ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] test-lint-duplicates: Only check for numbered test cases 2013-04-03 5:54 [PATCH] test-lint-duplicates: Only check for numbered test cases Torsten Bögershausen @ 2013-04-03 14:28 ` Jeff King 2013-04-10 14:14 ` Torsten Bögershausen 0 siblings, 1 reply; 4+ messages in thread From: Jeff King @ 2013-04-03 14:28 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: git On Wed, Apr 03, 2013 at 07:54:02AM +0200, Torsten Bögershausen wrote: > Running make inside contrib/remote-helpers fails in "test-lint-duplicates" > > This was because the regexp checking for duplicate numbers strips everything > after the first "-" in the filename, including the prefix. > > As a result, 2 pathnames like > "xxxx/contrib/remote-helpers/test-bzr.sh" and > "xxxx/contrib/remote-helpers/test-hg-bidi.sh" > > are both converted into > "xxxx/contrib/remote", and reported as duplicate. > > Improve the regexp: > Remove everything after tNNNN- (where X stand for a digit) I think the approach to just make test-lint-duplicates a no-op on non-numbered tests is reasonable, but this is side-stepping half of the issue. The problems are: 1. We do not have numbers in our test names. 2. We _do_ have full paths in the test names, which might have elements which look like test script names. Your patch tightens the match so that a hyphen in the path name does not confuse our script. But it trades it for being confused by tNNNN in the pathname. Which is admittedly less likely, but is not addressing the fundamental issues that we should only be processing basenames. So something like "sed 's,.*/,,'" would fix that. But that still leaves us with a bunch of tests called "test-foo", "test-bar", etc, which will appear as duplicates. So we would still want to tighten the number parsing. Like: diff --git a/t/Makefile b/t/Makefile index 5c6de81..e5afa4c 100644 --- a/t/Makefile +++ b/t/Makefile @@ -47,7 +47,9 @@ test-lint-duplicates: test-lint: test-lint-duplicates test-lint-executable test-lint-duplicates: - @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \ + @dups=`echo $(T) | tr ' ' '\n' | \ + sed -e 's,.*/,,' -e 's/\(t[0-9][0-9][0-9][0-9]\)-.*/\1/' | \ + sort | uniq -d` && \ test -z "$$dups" || { \ echo >&2 "duplicate test numbers:" $$dups; exit 1; } -Peff ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] test-lint-duplicates: Only check for numbered test cases 2013-04-03 14:28 ` Jeff King @ 2013-04-10 14:14 ` Torsten Bögershausen 2013-04-10 16:00 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Torsten Bögershausen @ 2013-04-10 14:14 UTC (permalink / raw) To: Jeff King; +Cc: Torsten Bögershausen, git On 03.04.13 16:28, Jeff King wrote: > On Wed, Apr 03, 2013 at 07:54:02AM +0200, Torsten Bögershausen wrote: > >> Running make inside contrib/remote-helpers fails in "test-lint-duplicates" >> >> This was because the regexp checking for duplicate numbers strips everything >> after the first "-" in the filename, including the prefix. >> >> As a result, 2 pathnames like >> "xxxx/contrib/remote-helpers/test-bzr.sh" and >> "xxxx/contrib/remote-helpers/test-hg-bidi.sh" >> >> are both converted into >> "xxxx/contrib/remote", and reported as duplicate. >> >> Improve the regexp: >> Remove everything after tNNNN- (where X stand for a digit) > > I think the approach to just make test-lint-duplicates a no-op on > non-numbered tests is reasonable, but this is side-stepping half of the > issue. The problems are: > > 1. We do not have numbers in our test names. > > 2. We _do_ have full paths in the test names, which might have > elements which look like test script names. > > Your patch tightens the match so that a hyphen in the path name does not > confuse our script. But it trades it for being confused by tNNNN in the > pathname. Which is admittedly less likely, but is not addressing the > fundamental issues that we should only be processing basenames. > > So something like "sed 's,.*/,,'" would fix that. But that still leaves > us with a bunch of tests called "test-foo", "test-bar", etc, which will > appear as duplicates. So we would still want to tighten the number > parsing. > > Like: > > diff --git a/t/Makefile b/t/Makefile > index 5c6de81..e5afa4c 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -47,7 +47,9 @@ test-lint-duplicates: > test-lint: test-lint-duplicates test-lint-executable > > test-lint-duplicates: > - @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \ > + @dups=`echo $(T) | tr ' ' '\n' | \ > + sed -e 's,.*/,,' -e 's/\(t[0-9][0-9][0-9][0-9]\)-.*/\1/' | \ > + sort | uniq -d` && \ > test -z "$$dups" || { \ > echo >&2 "duplicate test numbers:" $$dups; exit 1; } > > > -Peff I thinkg we need both the striping of the path and the "grepping" for numbered test cases only. I'll send a patch in a minute ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] test-lint-duplicates: Only check for numbered test cases 2013-04-10 14:14 ` Torsten Bögershausen @ 2013-04-10 16:00 ` Jeff King 0 siblings, 0 replies; 4+ messages in thread From: Jeff King @ 2013-04-10 16:00 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: git On Wed, Apr 10, 2013 at 04:14:34PM +0200, Torsten Bögershausen wrote: > > test-lint-duplicates: > > - @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \ > > + @dups=`echo $(T) | tr ' ' '\n' | \ > > + sed -e 's,.*/,,' -e 's/\(t[0-9][0-9][0-9][0-9]\)-.*/\1/' | \ > > + sort | uniq -d` && \ > > test -z "$$dups" || { \ > > echo >&2 "duplicate test numbers:" $$dups; exit 1; } > > > > > > -Peff > I thinkg we need both the striping of the path and the "grepping" for > numbered test cases only. > I'll send a patch in a minute I think it is fine either way. My command above turns: /path/to/test-one.sh /path/to/test-two.sh into test-one.sh test-two.sh which is fine for running through uniq. If you use "sed -n", you simply end up with an empty list, which is also fine. -Peff ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-04-10 16:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-03 5:54 [PATCH] test-lint-duplicates: Only check for numbered test cases Torsten Bögershausen 2013-04-03 14:28 ` Jeff King 2013-04-10 14:14 ` Torsten Bögershausen 2013-04-10 16:00 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).