tests: fix overeager scrubbing of environment variables

PATCH Superseded Jens Lehmann 4 participants 10 messages Jun 15, 2016

Share & tools

Jens Lehmann

author

2016-06-15 22:50 UTC #0 lore raw

PATCH 1/1 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_SKIP_TESTS

- GIT_TEST*

- GIT_PROVE_OPTS

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

Signed-off-by Jens Lehmann <[email protected]>

---

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(-)

t/test-lib.sh +1 -1

index 7cc9a52..4a8c443 100644
@@ -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);
 ')
 [email protected]

Signature · 1 line

1.7.4.2.407.gac78c

Jonathan Nieder

Acked-by on v1 1/1

2016-06-15 22:50 UTC #1 lore raw

Quoted 7 lines · Jens Lehmann

- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_PROVE_OPTS
Let's fix that by adding them to the exception list.
Signed-off-by Jens Lehmann <[email protected]>

Acked-by Jonathan Nieder <[email protected]>

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.

Quoted 5 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

2016-06-15 22:50 UTC #2 lore raw

PATCH 1/1 squash! tests: fix overeager scrubbing of environment variables

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

Quoted 10 lines · Jonathan Nieder

Quoted 7 lines · Jens Lehmann
- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_PROVE_OPTS
Let's fix that by adding them to the exception list.
Signed-off-by Jens Lehmann <[email protected]>

Quoted 8 lines

Acked-by Jonathan Nieder <[email protected]>
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 <[email protected]>

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

t/test-lib.sh +9 -1

index 4a8c443..d28e647 100644
@@ -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);
 ')
 [email protected]

Signature · 1 line

1.7.4.2

Jeff King

2016-06-15 22:50 UTC #3 lore raw

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

Quoted 16 lines · Jonathan Nieder

How about something like this on top? Would it still be safe on platforms with ancient perl?
Signed-off-by Jonathan Nieder <[email protected]>
---
 t/test-lib.sh |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
t/test-lib.sh +0 -0
index 4a8c443..d28e647 100644
@@ -46,7 +46,15 @@ unset VISUAL
 unset EMAIL
 unset $(perl -e '
 	my @env = keys %ENV;

t/test-lib.sh unset VISUAL · line 57

Quoted 10 lines

t/test-lib.sh +9 -1

-	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

author

2016-06-15 22:50 UTC #4 lore raw

PATCH v2 1/1 tests: fix overeager scrubbing of environment variables

In reply to Jonathan Nieder

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 <[email protected]>

Acked-by Jonathan Nieder <[email protected]>

---

Am 27.03.2011 23:37, schrieb Jonathan Nieder:

Quoted 11 lines

Quoted 7 lines · Jens Lehmann
- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_PROVE_OPTS
Let's fix that by adding them to the exception list.
Signed-off-by Jens Lehmann <[email protected]>
Acked-by Jonathan Nieder <[email protected]>

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!)

Quoted 6 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?).

Quoted 2 lines

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 ... ;-)

Quoted 7 lines

Quoted 5 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(-)

t/test-lib.sh +1 -1

index 7cc9a52..f1f6d94 100644
@@ -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);
 ')
 [email protected]

Signature · 1 line

1.7.4.2.407.gac78c

Jonathan Nieder

2016-06-15 22:50 UTC #5 lore raw

[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 <[email protected]>

---

Quoted 3 lines · Jens Lehmann

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.

t/test-lib.sh unset VISUAL · line 49

Quoted 8 lines

t/test-lib.sh +1 -1

@@ -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(-)

t/t0081-line-buffer.sh +1 -1

index 1dbe1c9..099abe0 100755
@@ -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 &&

t/t3302-notes-index-expensive.sh +1 -1

index e35d781..62820d0 100755
@@ -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 () {

t/t3419-rebase-patch-id.sh +1 -1

index bd8efaf..0419e81 100755
@@ -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()

Signature · 1 line

1.7.4.2

Junio C Hamano

2016-06-15 22:50 UTC #6 lore raw

In reply to Jonathan Nieder

Quoted 14 lines · Jonathan Nieder

Quoted 7 lines · Jens Lehmann
- GIT_SKIP_TESTS
- GIT_TEST*
- GIT_PROVE_OPTS
Let's fix that by adding them to the exception list.
Signed-off-by Jens Lehmann <[email protected]>
Acked-by Jonathan Nieder <[email protected]>
It misses a few, though:

Quoted 7 lines

- 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

author

2016-06-15 22:50 UTC #7 lore raw

PATCH v3 1/1 tests: fix overeager scrubbing of environment variables

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 <[email protected]>

Acked-by Jonathan Nieder <[email protected]>

Thanks-to: Jeff King <[email protected]>
---

Am 28.03.2011 19:00, schrieb Junio C Hamano:

Quoted 4 lines

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(-)

t/test-lib.sh +10 -1

index 7cc9a52..7a56f11 100644
@@ -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);
 ')
 [email protected]

Signature · 1 line

1.7.4.2.407.g50b80.dirty

Jonathan Nieder

2016-06-15 22:50 UTC #8 lore raw

Quoted 1 line · Jens Lehmann

Acked-by Jonathan Nieder <[email protected]>

Yes, still. :)

Quoted 2 lines

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

2016-06-15 22:50 UTC #9 lore raw

Quoted 6 lines · Jonathan Nieder

Quoted 1 line · Jens Lehmann
Acked-by Jonathan Nieder <[email protected]>
Yes, still. :)

Quoted 7 lines

Quoted 2 lines
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.