[v3,5/7] perf lint: check test-lint-shell-syntax in perf tests

Commit Message

Nipunn Koorapati Oct. 19, 2020, 10:47 p.m. UTC

From: Nipunn Koorapati <[e-mail]>

Perf tests have some seq instead of test_seq. This
runs the existing tests on the perf tests as well.

Signed-off-by: Nipunn Koorapati <[e-mail]>
---
 t/Makefile             | 3 ++-
 t/perf/p3400-rebase.sh | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Taylor Blau Oct. 20, 2020, 2:38 a.m. UTC | #1

On Mon, Oct 19, 2020 at 10:47:35PM +0000, Nipunn Koorapati via GitGitGadget wrote:
>  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)

I really appreciate your initiative to modify t/Makefile to start
linting t/perf/p????-*.sh files, too. Could I bother you to elaborate a
little bit on why you chose to modify a recipe in t/Makefile instead of
t/perf/Makefile?

I'm not necessarily opposed, but having this in t/perf/Makefile would
allow me to just run 'make' in 't/perf' and still have the scripts
linted there without having to involve a 'make' in 't'.

For what it's worth, I suspect that this is because 't/Makefile' already
has a 'test-lint-shell-syntax' target, and 't/perf/Makefile' does not. I
think it would be OK to add it there, too, and move this change into
t/perf.

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

Makes sense. I wouldn't be opposed to breaking this out into an earlier
change (e.g., "it's about to become not OK to use seq in t/perf, so
prepare for that by replacing any invocations with test_seq()"), but I
think it's probably not worth it, since this patch is small as it is.

Thanks,
Taylor

Junio C Hamano Oct. 20, 2020, 3:10 a.m. UTC | #2

Taylor Blau <[e-mail]> writes:

>>  		echo change$i >unrelated-file$i &&
>> -		seq 1000 | tac >>unrelated-file$i &&
>> +		test_seq 1000 | tac >>unrelated-file$i &&
>
> Makes sense. I wouldn't be opposed to breaking this out into an earlier
> change (e.g., "it's about to become not OK to use seq in t/perf, so
> prepare for that by replacing any invocations with test_seq()"), but I
> think it's probably not worth it, since this patch is small as it is.

test_seq is fine, but I do not think tac is portable (only saved by
the fact that not many people, especially on exotic platforms, run
perf scripts).

Taylor Blau Oct. 20, 2020, 3:15 a.m. UTC | #3

On Mon, Oct 19, 2020 at 08:10:56PM -0700, Junio C Hamano wrote:
> Taylor Blau <[e-mail]> writes:
>
> >>  		echo change$i >unrelated-file$i &&
> >> -		seq 1000 | tac >>unrelated-file$i &&
> >> +		test_seq 1000 | tac >>unrelated-file$i &&
> >
> > Makes sense. I wouldn't be opposed to breaking this out into an earlier
> > change (e.g., "it's about to become not OK to use seq in t/perf, so
> > prepare for that by replacing any invocations with test_seq()"), but I
> > think it's probably not worth it, since this patch is small as it is.
>
> test_seq is fine, but I do not think tac is portable (only saved by
> the fact that not many people, especially on exotic platforms, run
> perf scripts).

Serves me right for reading while I'm tired! I glazed right over 'tac'.
If you need a truly unrelated file, you could write random data into it
(there are some examples in t/test-lib-functions.sh), but I'd just write
'test_seq 1001'.

Thanks,
Taylor

Nipunn Koorapati Oct. 20, 2020, 10:09 a.m. UTC | #4

On Tue, Oct 20, 2020 at 3:39 AM Taylor Blau <[e-mail]> wrote:
>
> I'm not necessarily opposed, but having this in t/perf/Makefile would
> allow me to just run 'make' in 't/perf' and still have the scripts
> linted there without having to involve a 'make' in 't'.
>
> For what it's worth, I suspect that this is because 't/Makefile' already
> has a 'test-lint-shell-syntax' target, and 't/perf/Makefile' does not. I
> think it would be OK to add it there, too, and move this change into
> t/perf.

Looked at doing this and noticed that there are several targets in test-lint
in t/Makefile. This would involve duplicating them into t/perf/Makefile which
seems like it would be poor form, especially given their complexity.
Perhaps t/perf/Makefile could have a target which calls t/Makefile's test-lint
target instead. Will play around with it.

>
> Makes sense. I wouldn't be opposed to breaking this out into an earlier
> change (e.g., "it's about to become not OK to use seq in t/perf, so
> prepare for that by replacing any invocations with test_seq()"), but I
> think it's probably not worth it, since this patch is small as it is.
>

Yeah - I see the point, but I agree that since the patch is small,
it's ok this way.
If the patch grows significantly, I can make it into two patches

--Nipunn

Nipunn Koorapati Oct. 20, 2020, 10:16 a.m. UTC | #5

> > test_seq is fine, but I do not think tac is portable (only saved by
> > the fact that not many people, especially on exotic platforms, run
> > perf scripts).
>
> Serves me right for reading while I'm tired! I glazed right over 'tac'.
> If you need a truly unrelated file, you could write random data into it
> (there are some examples in t/test-lib-functions.sh), but I'd just write
> 'test_seq 1001'.

Seems like there might be some value to adding `tac` to the perl script
check-non-portable-shell.pl - though I'm not sure what we'd use as an
alternative. I'll leave this here for now for someone else to handle in
a follow up patch series

Patch

diff --git a/t/Makefile b/t/Makefile
index c83fd18861..74b53af2bd 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
 
@@ -91,7 +92,7 @@  test-lint-executable:
 		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/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 ||