[PATCH] tests: fix overeager scrubbing of environment variables

[PATCH] tests: fix overeager scrubbing of environment variables

[Jens Lehmann]

In commit 95a1d12e9b9f ("tests: scrub environment of GIT_* variables") all
environment variables starting with "GIT_" were unset for the tests using
a perl script rather than unsetting them one by one. Only three exceptions
were made to make them work as before: "GIT_TRACE*", "GIT_DEBUG*" and
"GIT_USE_LOOKUP".

Unfortunately some environment variables used by the test framework itself
were not added to the exceptions and thus stopped working when given
before the make command instead of after it. Those are:

- GIT_SKIP_TESTS

- GIT_TEST*

- GIT_PROVE_OPTS

Let's fix that by adding them to the exception list.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I noticed today that skipping a test the way I was used to suddenly failed:

GIT_SKIP_TESTS='t1234' GIT_TEST_OPTS='--root=/dev/shm' make -j10 test

This should work according to t/README, but didn't anymore.

 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7cc9a52..4a8c443 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -46,7 +46,7 @@ unset VISUAL
 unset EMAIL
 unset $(perl -e '
 	my @env = keys %ENV;
-	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP)/, @env);
+	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP|SKIP_TESTS|TEST|PROVE_OPTS)/, @env);
 	print join("\n", @vars);
 ')
 GIT_AUTHOR_EMAIL=author@example.com
-- 
1.7.4.2.407.gac78c

Re: [PATCH] tests: fix overeager scrubbing of environment variables

[Jonathan Nieder]

Jens Lehmann wrote:

> - GIT_SKIP_TESTS
> - GIT_TEST*
> - GIT_PROVE_OPTS
>
> Let's fix that by adding them to the exception list.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

It misses a few, though:

 - GIT_REMOTE_SVN_TEST_BIG_FILES
 - GIT_NOTES_TIMING_TESTS
 - GIT_PATCHID_TIMING_TESTS
 - GIT_VALGRIND_OPTIONS

If we were starting over I suppose those would all be GIT_TEST* but
there's something to be said for muscle memory.

> I noticed today that skipping a test the way I was used to suddenly failed:
>
> GIT_SKIP_TESTS='t1234' GIT_TEST_OPTS='--root=/dev/shm' make -j10 test
>
> This should work according to t/README, but didn't anymore.

I would have just written that in the change description.

Good catch, thanks.

[PATCH] squash! tests: fix overeager scrubbing of environment variables

[Jonathan Nieder]

(+cc: Jeff and Ævar for perl advice)
Jonathan Nieder wrote:
> Jens Lehmann wrote:

>> - GIT_SKIP_TESTS
>> - GIT_TEST*
>> - GIT_PROVE_OPTS
>>
>> Let's fix that by adding them to the exception list.
>>
>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> It misses a few, though:
>
>  - GIT_REMOTE_SVN_TEST_BIG_FILES
>  - GIT_NOTES_TIMING_TESTS
>  - GIT_PATCHID_TIMING_TESTS
>  - GIT_VALGRIND_OPTIONS

How about something like this on top?  Would it still be safe on
platforms with ancient perl?

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a8c443..d28e647 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -46,7 +46,15 @@ unset VISUAL
 unset EMAIL
 unset $(perl -e '
 	my @env = keys %ENV;
-	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP|SKIP_TESTS|TEST|PROVE_OPTS)/, @env);
+	my @vars = grep(/^GIT_/ && !m{^GIT_(
+			TRACE |
+			DEBUG |
+			USE_LOOKUP |
+			TEST |
+			.*_TEST |
+			PROVE |
+			VALGRIND
+		)}x, @env);
 	print join("\n", @vars);
 ')
 GIT_AUTHOR_EMAIL=author@example.com
-- 
1.7.4.2

[PATCH v2] tests: fix overeager scrubbing of environment variables

[Jens Lehmann]

In commit 95a1d12e9b9f ("tests: scrub environment of GIT_* variables") all
environment variables starting with "GIT_" were unset for the tests using
a perl script rather than unsetting them one by one. Only three exceptions
were made to make them work as before: "GIT_TRACE*", "GIT_DEBUG*" and
"GIT_USE_LOOKUP".

Unfortunately some environment variables used by the test framework itself
were not added to the exceptions and thus stopped working when given
before the make command instead of after it. Those are:

- GIT_NOTES_TIMING_TESTS
- GIT_PATCHID_TIMING_TESTS
- GIT_PROVE_OPTS
- GIT_REMOTE_SVN_TEST_BIG_FILES
- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_VALGRIND_OPTIONS

I noticed that when skipping a test the way I was used to suddenly failed:

GIT_SKIP_TESTS='t1234' GIT_TEST_OPTS='--root=/dev/shm' make -j10 test

This should work according to t/README, but didn't anymore, so let's fix
that by adding them to the exception list.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
---

Am 27.03.2011 23:37, schrieb Jonathan Nieder:
> Jens Lehmann wrote:
> 
>> - GIT_SKIP_TESTS
>> - GIT_TEST*
>> - GIT_PROVE_OPTS
>>
>> Let's fix that by adding them to the exception list.
>>
>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> 
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for your review! (I took the liberty to add your Ack to this updated
version, please speak up if you are unhappy with anything in this v2!)

> It misses a few, though:
> 
>  - GIT_REMOTE_SVN_TEST_BIG_FILES
>  - GIT_NOTES_TIMING_TESTS
>  - GIT_PATCHID_TIMING_TESTS
>  - GIT_VALGRIND_OPTIONS

Right you are, this version includes those too (but - except once for
GIT_VALGRIND_OPTIONS - none of them are mentioned in t/README, that's why
I managed to miss them ... maybe they should be documented there?).

> If we were starting over I suppose those would all be GIT_TEST* but
> there's something to be said for muscle memory.

Yeah, I thought the same about GIT_SKIP_TESTS when writing the first version
of this patch ... ;-)

>> I noticed today that skipping a test the way I was used to suddenly failed:
>>
>> GIT_SKIP_TESTS='t1234' GIT_TEST_OPTS='--root=/dev/shm' make -j10 test
>>
>> This should work according to t/README, but didn't anymore.
> 
> I would have just written that in the change description.

Yup, did that!

 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7cc9a52..f1f6d94 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -46,7 +46,7 @@ unset VISUAL
 unset EMAIL
 unset $(perl -e '
 	my @env = keys %ENV;
-	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP)/, @env);
+	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP|NOTES_TIMING_TESTS|PATCHID_TIMING_TESTS|PROVE_OPTS|REMOTE_SVN_TEST_BIG_FILES|SKIP_TESTS|TEST|VALGRIND_OPTIONS)/, @env);
 	print join("\n", @vars);
 ')
 GIT_AUTHOR_EMAIL=author@example.com
-- 
1.7.4.2.407.gac78c

[PATCH/RFC] tests: use the --long-tests facility to toggle some slow tests

[Jonathan Nieder]

The GIT_TEST_LONG facility (v1.6.0-rc0~246, 2008-06-17) was added to
support tests for a gitweb caching engine that was never merged.
So --long-tests has been a no-op for years.

Since then, some other expensive tests have sprouted up that fit its
description pretty well, namely:

 - GIT_NOTES_TIMING_TESTS
 - GIT_PATCHID_TIMING_TESTS
 - GIT_REMOTE_SVN_TEST_BIG_FILES

All of these are not part of the default test run because they are
expensive.  Let's replace their existing, undocumented triggers with
GIT_TEST_LONG, so now you can do

 sh t3302-notes-index-expensive.sh -l -v

to get a sanity check for notes lookup performance scalability.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jens Lehmann wrote:

> Right you are, this version includes those too (but - except once for
> GIT_VALGRIND_OPTIONS - none of them are mentioned in t/README, that's why
> I managed to miss them ... maybe they should be documented there?).

Good idea.  See below for a possible sneaky way to document three of
them.

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -46,7 +46,7 @@ unset VISUAL
>  unset EMAIL
>  unset $(perl -e '
>  	my @env = keys %ENV;
> -	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP)/, @env);
> +	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP|NOTES_TIMING_TESTS|PATCHID_TIMING_TESTS|PROVE_OPTS|REMOTE_SVN_TEST_BIG_FILES|SKIP_TESTS|TEST|VALGRIND_OPTIONS)/, @env);

My poor terminal. :)  I sent a possible more invasive change that
breaks this into multiple lines but it looks like our mails crossed.
Anyway, I have no serious complaint; it might be that this one is the
best way to go.  

 t/t0081-line-buffer.sh           |    2 +-
 t/t3302-notes-index-expensive.sh |    2 +-
 t/t3419-rebase-patch-id.sh       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 1dbe1c9..099abe0 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -12,7 +12,7 @@ correctly.
 "
 . ./test-lib.sh
 
-test -n "$GIT_REMOTE_SVN_TEST_BIG_FILES" && test_set_prereq EXPENSIVE
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 generate_tens_of_lines () {
 	tens=$1 &&
diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index e35d781..62820d0 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -8,7 +8,7 @@ test_description='Test commit notes index (expensive!)'
 . ./test-lib.sh
 
 test_set_prereq NOT_EXPENSIVE
-test -n "$GIT_NOTES_TIMING_TESTS" && test_set_prereq EXPENSIVE
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 create_repo () {
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index bd8efaf..0419e81 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,7 +5,7 @@ test_description='git rebase - test patch id computation'
 . ./test-lib.sh
 
 test_set_prereq NOT_EXPENSIVE
-test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 count()
-- 
1.7.4.2

Re: [PATCH] squash! tests: fix overeager scrubbing of environment variables

[Jeff King]

On Sun, Mar 27, 2011 at 05:17:53PM -0500, Jonathan Nieder wrote:

> How about something like this on top?  Would it still be safe on
> platforms with ancient perl?
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  t/test-lib.sh |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4a8c443..d28e647 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -46,7 +46,15 @@ unset VISUAL
>  unset EMAIL
>  unset $(perl -e '
>  	my @env = keys %ENV;
> -	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP|SKIP_TESTS|TEST|PROVE_OPTS)/, @env);
> +	my @vars = grep(/^GIT_/ && !m{^GIT_(
> +			TRACE |
> +			DEBUG |
> +			USE_LOOKUP |
> +			TEST |
> +			.*_TEST |
> +			PROVE |
> +			VALGRIND
> +		)}x, @env);

I think that should be fine. The most exotic thing you use is /x. I
don't know when it was introduced, but perl5004delta refers to it as
something that was already around before then.

I think it might be a little more readable to get rid of the "|" on each
line like this:

    my $ok = join("|", qw(
      TRACE
      DEBUG
      USE_LOOKUP
      TEST
      .*_TEST
      PROVE
      VALGRIND
    ));
    my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o) @env;

which is a little simpler syntax, and makes patches nicer (you don't
have to add a trailing "|" on the line before). But that is a minor nit.

-Peff

Re: [PATCH] tests: fix overeager scrubbing of environment variables

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jens Lehmann wrote:
>
>> - GIT_SKIP_TESTS
>> - GIT_TEST*
>> - GIT_PROVE_OPTS
>>
>> Let's fix that by adding them to the exception list.
>>
>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> It misses a few, though:
>
>  - GIT_REMOTE_SVN_TEST_BIG_FILES
>  - GIT_NOTES_TIMING_TESTS
>  - GIT_PATCHID_TIMING_TESTS
>  - GIT_VALGRIND_OPTIONS
>
> If we were starting over I suppose those would all be GIT_TEST* but
> there's something to be said for muscle memory.

I don't think muscle memory matters as much in this case as the usual
end-user facing interfaces.  These are only for people who build and test
git, not for our end users.

I think it is Ok to do this (yes I am trying to avoid doing this myself,
as I am a bit behind and also am lazy) just as a single patch to rename
the environment variables, and filter them with a single GIT_TEST_*
pattern at the same time.

Thanks.

[PATCH v3] tests: fix overeager scrubbing of environment variables

[Jens Lehmann]

In commit 95a1d12e9b9f ("tests: scrub environment of GIT_* variables") all
environment variables starting with "GIT_" were unset for the tests using
a perl script rather than unsetting them one by one. Only three exceptions
were made to make them work as before: "GIT_TRACE*", "GIT_DEBUG*" and
"GIT_USE_LOOKUP".

Unfortunately some environment variables used by the test framework itself
were not added to the exceptions and thus stopped working when given
before the make command instead of after it. Those are:

- GIT_NOTES_TIMING_TESTS
- GIT_PATCHID_TIMING_TESTS
- GIT_PROVE_OPTS
- GIT_REMOTE_SVN_TEST_BIG_FILES
- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_VALGRIND_OPTIONS

I noticed that when skipping a test the way I was used to suddenly failed:

GIT_SKIP_TESTS='t1234' GIT_TEST_OPTS='--root=/dev/shm' make -j10 test

This should work according to t/README, but didn't anymore, so let's fix
that by adding them to the exception list. And to avoid having a long
regexp put the exceptions in a separate variable using nicer formatting.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Jeff King <peff@peff.net>
---


Am 28.03.2011 19:00, schrieb Junio C Hamano:
> I think it is Ok to do this (yes I am trying to avoid doing this myself,
> as I am a bit behind and also am lazy) just as a single patch to rename
> the environment variables, and filter them with a single GIT_TEST_*
> pattern at the same time.

Agreed, that would be the best solution. But I got sidetracked by this
issue while trying to fix something else, so I would rather want to
avoid doing the whole exercise myself. So here is v3 of my patch which
just makes these variables work again.


 t/test-lib.sh |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7cc9a52..7a56f11 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -46,7 +46,16 @@ unset VISUAL
 unset EMAIL
 unset $(perl -e '
 	my @env = keys %ENV;
-	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP)/, @env);
+	my $ok = join("|", qw(
+		TRACE
+		DEBUG
+		USE_LOOKUP
+		TEST
+		.*_TEST
+		PROVE
+		VALGRIND
+	));
+	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
 ')
 GIT_AUTHOR_EMAIL=author@example.com
-- 
1.7.4.2.407.g50b80.dirty

Re: [PATCH v3] tests: fix overeager scrubbing of environment variables

[Jonathan Nieder]

Jens Lehmann wrote:

> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Yes, still. :)

> So here is v3 of my patch which
> just makes these variables work again.

Thanks, all.  This looks good for an immediate fix, and I can think
separately about how to make downstreams projects suffer less if
changing the names of variables like GIT_SKIP_TESTS to follow a
GIT_TEST_ scheme.

Re: [PATCH v3] tests: fix overeager scrubbing of environment variables

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jens Lehmann wrote:
>
>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Yes, still. :)
>
>> So here is v3 of my patch which
>> just makes these variables work again.
>
> Thanks, all.  This looks good for an immediate fix, and I can think
> separately about how to make downstreams projects suffer less if
> changing the names of variables like GIT_SKIP_TESTS to follow a
> GIT_TEST_ scheme.

Thanks, all.