threads / patch / 26882

patchtests: fix overeager scrubbing of environment variables

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

## tl;dr

10 messages between Mar 27, 2011 and Mar 28, 2011. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Jens Lehmann· Mar 27, 2011, 21:22 UTC · lore

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(-)
Show changes to t/test-lib.sh +1 −1
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
Jonathan Nieder· Mar 27, 2011, 21:37 UTC · re: Jens Lehmann · lore

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

Jens Lehmann wrote:
Show 7 quoted lines
> - 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.

Show 5 quoted lines
> 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.
Jonathan Nieder· Mar 27, 2011, 22:17 UTC · re: Jonathan Nieder · lore

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

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

> Jens Lehmann wrote:
Show 16 quoted lines
>> - 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(-)
Show changes to t/test-lib.sh +9 −1
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
Jeff King· Mar 28, 2011, 13:58 UTC · re: Jonathan Nieder · lore

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

On Sun, Mar 27, 2011 at 05:17:53PM -0500, Jonathan Nieder wrote:
Show 26 quoted lines
> 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
Jens Lehmann· Mar 27, 2011, 22:26 UTC · re: Jonathan Nieder · lore

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

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:
Show 11 quoted lines
> 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!)

Show 6 quoted lines
> 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 ... ;-)

Show 7 quoted lines
>> 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(-)
Show changes to t/test-lib.sh +1 −1
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
Jonathan Nieder· Mar 27, 2011, 23:00 UTC · re: Jens Lehmann · lore

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

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.

Show 8 quoted lines
> --- 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(-)
Show changes to 3 files +3 −3

t/t0081-line-buffer.sh, t/t3302-notes-index-expensive.sh, t/t3419-rebase-patch-id.sh

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
Junio C Hamano· Mar 28, 2011, 17:00 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> 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.
Jens Lehmann· Mar 28, 2011, 19:16 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to t/test-lib.sh +10 −1
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
Jonathan Nieder· Mar 28, 2011, 19:37 UTC · re: Jens Lehmann · lore

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

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.

Junio C Hamano· Mar 28, 2011, 21:03 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> 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.

← back to recent threads