On 9/20/2018 2:43 PM, Junio C Hamano wrote:
Ben Peart <benpeart@xxxxxxxxxxxxx> writes:
This round has one code change based on feedback. Other changes are just
rewording commit messages.
Thanks. I think the only remaining issue is what to do with the
interaction between extra/additional error message that comes from
the updates in 3/5 and the test framework selftest in t0000.
-- >8 --
Subject: t0000: do not get self-test disrupted by environment warnings
The test framework test-lib.sh itself would want to give warnings
and hints, e.g. when it sees a deprecated environment variable is in
use that we want to encourage users to migrate to another variable.
The self-test of test framework done in t0000 however do not expect
to see these warnings and hints, so depending on the settings of
environment variables, a running test may or may not produce these
messages to the standard error output, breaking the expectations of
self-test test framework does on itself. Here is what we see:
$ TEST_GIT_INDEX_VERSION=4 sh t0000-basic.sh -i -v
...
'err' is not empty, it contains:
warning: TEST_GIT_INDEX_VERSION is now GIT_TEST_INDEX_VERSION
hint: set GIT_TEST_INDEX_VERSION too during the transition period
not ok 5 - pretend we have a fully passing test suite
The following quick attempt to work it around does not work, because
some tests in t0000 do want to see expected errors from the test
framework itself.
t/t0000-basic.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 850f651e4e..88c6ed4696 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -88,7 +88,7 @@ _run_sub_test_lib_test_common () {
'
# Point to the t/test-lib.sh, which isn't in ../ as usual
- . "\$TEST_DIRECTORY"/test-lib.sh
+ . "\$TEST_DIRECTORY"/test-lib.sh >/dev/null 2>&1
EOF
cat >>"$name.sh" &&
chmod +x "$name.sh" &&
There are a few possible ways to work this around:
* We could strip the warning: and hint: unconditionally from the
error output before the error messages are checked in the
self-test (helper functions check_sub_test_lib_test_err and
check_sub_test_lib_test); the problem with this approach is that
it will make it impossible to write self-tests to ensure that
right warnings and hints are given.
* We could force a sane environment settings before the test helper
_run_sub_test_lib_test_common dot-sources test-lib.sh; the
problem with this approach is that _run_sub_test_lib_test_common
now needs to be aware of what pairs of environment variables are
checked in test-lib.sh using check_var_migration helper.
The final patch I came up with is probably the solution that is
least bad. Set a variable to tell test-lib.sh that we are running
a self-test, so that various pieces in test-lib.sh can react to keep
the output stable.
This looks like a reasonable compromise to me. It's nice to give the
migration hints to end users so they know they need to update their
environments to reflect the required changes. On the other hand, we
don't want or need them to be triggered when we are running the self-test.
It would be nice to enable that automatically without the need for
another environment variable but I couldn't think of a good way to
accomplish that so I agree - this seems like the "least bad" solution. :-)
Thanks Junio
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
t/t0000-basic.sh | 4 ++++
t/test-lib.sh | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 850f651e4e..52c02b7c7e 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -87,6 +87,10 @@ _run_sub_test_lib_test_common () {
passing metrics
'
+ # Tell the framework that we are self-testing to make sure
+ # it yields a stable result.
+ GIT_TEST_FRAMEWORK_SELFTEST=t &&
+
# Point to the t/test-lib.sh, which isn't in ../ as usual
. "\$TEST_DIRECTORY"/test-lib.sh
EOF
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8ef86e05a3..364a11ea25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -135,9 +135,17 @@ GIT_TRACE_BARE=1
export GIT_TRACE_BARE
check_var_migration () {
+ # the warnings and hints given from this helper depends
+ # on end-user settings, which will disrupt the self-test
+ # done on the test framework itself.
+ case "$GIT_TEST_FRAMEWORK_SELFTEST" in
+ t) return ;;
+ esac
+
old_name=$1 new_name=$2
eval "old_isset=\${${old_name}:+isset}"
eval "new_isset=\${${new_name}:+isset}"
+
case "$old_isset,$new_isset" in
isset,)
echo >&2 "warning: $old_name is now $new_name"
