Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2020, 17:23 UTC
Message-ID: <xmqqwo78wdq9.fsf@gitster.c.googlers.com>
In-Reply-To: <0767c8b77c820cfc03bbc617da4dc9f20ba4a46a.1585114881.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> The `test_expect_*` functions use `test_eval_` and so does
> `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
> `test_eval_` turns on tracing while evaluating the code block, and turns
> it off directly after it.
>
> This is unwanted for nested invocations.

Nice finding.

Show 5 quoted lines

> As we will introduce just such a scenario with the GPG, GPGSM and
> RFC1991 prereqs, let's fix that by introducing a variable that keeps
> track of the current trace level: nested `test_eval_` calls will
> increment and then decrement the level, and only when it reaches 0, the
> tracing will _actually_ be turned off.

Doesn't this explanation urge us to reorder these patches? It sounds to me that it argues to have this step before 3/5.

Other than that, both the explanation and the code look correctly done. It looks to me that the surrounding code favors trailing "_" instead of leading one for private names, so we might want to rename the variable to $trace_level_ but that is minor.

Thanks.

Show 60 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t0000-basic.sh | 13 +++++++++++++
>  t/test-lib.sh    |  6 ++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 3e440c078d5..b8597216200 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -833,6 +833,19 @@ then
>  	exit 1
>  fi
>  
> +test_expect_success 'lazy prereqs do not turn off tracing' "
> +	run_sub_test_lib_test lazy-prereq-and-tracing \
> +		'lazy prereqs and -x' -v -x <<-\\EOF &&
> +	test_lazy_prereq LAZY true
> +
> +	test_expect_success lazy 'test_have_prereq LAZY && echo trace'
> +
> +	test_done
> +	EOF
> +
> +	grep 'echo trace' lazy-prereq-and-tracing/err
> +"
> +
>  test_expect_success 'tests clean up even on failures' "
>  	run_sub_test_lib_test_err \
>  		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0ea1e5a05ed..dbf25348106 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -882,6 +882,7 @@ maybe_setup_valgrind () {
>  	fi
>  }
>  
> +_trace_level=0
>  want_trace () {
>  	test "$trace" = t && {
>  		test "$verbose" = t || test "$verbose_log" = t
> @@ -895,7 +896,7 @@ want_trace () {
>  test_eval_inner_ () {
>  	# Do not add anything extra (including LF) after '$*'
>  	eval "
> -		want_trace && set -x
> +		want_trace && _trace_level=$(($_trace_level+1)) && set -x
>  		$*"
>  }
>  
> @@ -926,7 +927,8 @@ test_eval_ () {
>  		test_eval_ret_=$?
>  		if want_trace
>  		then
> -			set +x
> +			test 1 = $_trace_level && set +x
> +			_trace_level=$(($_trace_level-1))
>  		fi
>  	} 2>/dev/null 4>&2