diff mbox series

[v4,5/7] perf lint: add make test-lint to perf tests

Message ID b534cd137a833de802d6d95c1affb8d2d8f7de85.1603201265.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series use fsmonitor data in git diff eliminating O(num_files) calls to lstat | expand

Commit Message

Nipunn Koorapati Oct. 20, 2020, 1:41 p.m. UTC 
From: Nipunn Koorapati <nipunn@dropbox.com>

Perf tests have not been linted for some time.
They've grown some seq instead of test_seq. This
runs the existing lints on the perf tests as well.

Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
---
 t/Makefile             | 7 ++++---
 t/perf/Makefile        | 5 ++++-
 t/perf/p3400-rebase.sh | 6 +++---
 3 files changed, 11 insertions(+), 7 deletions(-)

Comments

Taylor Blau Oct. 20, 2020, 10:06 p.m. UTC | #1 
On Tue, Oct 20, 2020 at 01:41:02PM +0000, Nipunn Koorapati via GitGitGadget wrote:
> diff --git a/t/perf/Makefile b/t/perf/Makefile
> index 8c47155a7c..fcb0e8865e 100644
> --- a/t/perf/Makefile
> +++ b/t/perf/Makefile
> @@ -1,7 +1,7 @@
>  -include ../../config.mak
>  export GIT_TEST_OPTIONS
>
> -all: perf
> +all: test-lint perf
>
>  perf: pre-clean
>  	./run
> @@ -12,4 +12,7 @@ pre-clean:
>  clean:
>  	rm -rf build "trash directory".* test-results
>
> +test-lint:
> +	$(MAKE) -C .. test-lint
> +

Great; it sounds like adding a complete definition here was too much
effort to be worth it, but that adding a '$(MAKE) -C ..' is just right.
We can still run 'make test-lint' from within 't/perf', but there isn't
a bunch of clutter in this series to make that happen. Thanks.

>  .PHONY: all perf pre-clean clean
> diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
> index d202aaed06..7a0bb29448 100755
> --- a/t/perf/p3400-rebase.sh
> +++ b/t/perf/p3400-rebase.sh
> @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
>  	git checkout -f -B base &&
>  	git checkout -B to-rebase &&
>  	git checkout -B upstream &&
> -	for i in $(seq 100)
> +	for i in $(test_seq 100)
>  	do
>  		# simulate huge diffs
>  		echo change$i >unrelated-file$i &&
> -		seq 1000 >>unrelated-file$i &&
> +		test_seq 1000 >>unrelated-file$i &&
>  		git add unrelated-file$i &&
>  		test_tick &&
>  		git commit -m commit$i unrelated-file$i &&
>  		echo change$i >unrelated-file$i &&
> -		seq 1000 | tac >>unrelated-file$i &&
> +		test_seq 1000 | tac >>unrelated-file$i &&

The rest of this all looks good, but I think adding 'tac' here is still
wrong; this isn't available everywhere, so we would want to find an
alternative before going further. Is there a reason that you couldn't
use a different 'N' in 'test_seq N' here?

Thanks,
Taylor
Nipunn Koorapati Oct. 20, 2020, 10:17 p.m. UTC | #2 
> > --- a/t/perf/p3400-rebase.sh
> > +++ b/t/perf/p3400-rebase.sh
> > @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
> >       git checkout -f -B base &&
> >       git checkout -B to-rebase &&
> >       git checkout -B upstream &&
> > -     for i in $(seq 100)
> > +     for i in $(test_seq 100)
> >       do
> >               # simulate huge diffs
> >               echo change$i >unrelated-file$i &&
> > -             seq 1000 >>unrelated-file$i &&
> > +             test_seq 1000 >>unrelated-file$i &&
> >               git add unrelated-file$i &&
> >               test_tick &&
> >               git commit -m commit$i unrelated-file$i &&
> >               echo change$i >unrelated-file$i &&
> > -             seq 1000 | tac >>unrelated-file$i &&
> > +             test_seq 1000 | tac >>unrelated-file$i &&
>
> The rest of this all looks good, but I think adding 'tac' here is still
> wrong; this isn't available everywhere, so we would want to find an
> alternative before going further. Is there a reason that you couldn't
> use a different 'N' in 'test_seq N' here?

Hey. I think there's some confusion. I didn't add `tac`. It was
already here. I didn't even notice it until Junio mentioned it.

--Nipunn
Taylor Blau Oct. 20, 2020, 10:19 p.m. UTC | #3 
On Tue, Oct 20, 2020 at 11:17:23PM +0100, Nipunn Koorapati wrote:
> > > --- a/t/perf/p3400-rebase.sh
> > > +++ b/t/perf/p3400-rebase.sh
> > > @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
> > >       git checkout -f -B base &&
> > >       git checkout -B to-rebase &&
> > >       git checkout -B upstream &&
> > > -     for i in $(seq 100)
> > > +     for i in $(test_seq 100)
> > >       do
> > >               # simulate huge diffs
> > >               echo change$i >unrelated-file$i &&
> > > -             seq 1000 >>unrelated-file$i &&
> > > +             test_seq 1000 >>unrelated-file$i &&
> > >               git add unrelated-file$i &&
> > >               test_tick &&
> > >               git commit -m commit$i unrelated-file$i &&
> > >               echo change$i >unrelated-file$i &&
> > > -             seq 1000 | tac >>unrelated-file$i &&
> > > +             test_seq 1000 | tac >>unrelated-file$i &&
> >
> > The rest of this all looks good, but I think adding 'tac' here is still
> > wrong; this isn't available everywhere, so we would want to find an
> > alternative before going further. Is there a reason that you couldn't
> > use a different 'N' in 'test_seq N' here?
>
> Hey. I think there's some confusion. I didn't add `tac`. It was
> already here. I didn't even notice it until Junio mentioned it.

You're right, sorry; I just saw a line beginning with '+' that contained
'tac' and thought that it was new in this patch. What you have is OK,
then, since it's not a new problem with your patch.

It couldn't hurt to have the linting phase catch that, but let's leave
that for another day, since I think what you have in this version looks
good to me.

Thanks for listening to all of my feedback :).

> --Nipunn

Thanks,
Taylor
diff mbox series

Patch

diff --git a/t/Makefile b/t/Makefile
index c83fd18861..882d26eee3 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -34,6 +34,7 @@  CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
+TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = sed -f chainlint.sed
 
@@ -81,17 +82,17 @@  test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
 
 test-lint-duplicates:
-	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
+	@dups=`echo $(T) $(TPERF) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
 		test -z "$$dups" || { \
 		echo >&2 "duplicate test numbers:" $$dups; exit 1; }
 
 test-lint-executable:
-	@bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
+	@bad=`for i in $(T) $(TPERF); do test -x "$$i" || echo $$i; done` && \
 		test -z "$$bad" || { \
 		echo >&2 "non-executable tests:" $$bad; exit 1; }
 
 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
 
 test-lint-filenames:
 	@# We do *not* pass a glob to ls-files but use grep instead, to catch
diff --git a/t/perf/Makefile b/t/perf/Makefile
index 8c47155a7c..fcb0e8865e 100644
--- a/t/perf/Makefile
+++ b/t/perf/Makefile
@@ -1,7 +1,7 @@ 
 -include ../../config.mak
 export GIT_TEST_OPTIONS
 
-all: perf
+all: test-lint perf
 
 perf: pre-clean
 	./run
@@ -12,4 +12,7 @@  pre-clean:
 clean:
 	rm -rf build "trash directory".* test-results
 
+test-lint:
+	$(MAKE) -C .. test-lint
+
 .PHONY: all perf pre-clean clean
diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
index d202aaed06..7a0bb29448 100755
--- a/t/perf/p3400-rebase.sh
+++ b/t/perf/p3400-rebase.sh
@@ -9,16 +9,16 @@  test_expect_success 'setup rebasing on top of a lot of changes' '
 	git checkout -f -B base &&
 	git checkout -B to-rebase &&
 	git checkout -B upstream &&
-	for i in $(seq 100)
+	for i in $(test_seq 100)
 	do
 		# simulate huge diffs
 		echo change$i >unrelated-file$i &&
-		seq 1000 >>unrelated-file$i &&
+		test_seq 1000 >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i unrelated-file$i &&
 		echo change$i >unrelated-file$i &&
-		seq 1000 | tac >>unrelated-file$i &&
+		test_seq 1000 | tac >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i-reverse unrelated-file$i ||