Re: Re*: [PATCH v3 0/5] Cleanup pass on special test setups

> Ben Peart <benpeart@microsoft.com> 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.
> 

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   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"
>