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

[Ben Peart <peartben@gmail.com>]

On 9/20/2018 2:43 PM, Junio C Hamano wrote:
> 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.
> 

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@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"
>