Rubén Justo <rjusto@xxxxxxxxx> writes:
> In the if-else's chain we have in "check_test_results_san_file_", we
> consider three variables: $passes_sanitize_leak, $sanitize_leak_check
> and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at
> that point).
Before this paragraph, we'd probably want to say what problem we are
fixing. Using the verb "fix" on the subject line without saying what
broken behaviour you see around GIT_TEST_SANITIZE_LEAK_LOG does not
help, either.
Your patch from September 2023 [*] did mention it upfront:
GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will
make the test return zero unintentionally.
With that inserted in front of the proposed log message, the
resulting explanation looks reasonable to me.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 79d3e0e7d9..7ed6d3fc47 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
> then
> say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
> invert_exit_code=t
> - else
> + elif test "$test_failure" = 0
> + then
> say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> invert_exit_code=t
> + else
> + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
> fi
> }
This is outside the scope of this patch simply because it is
inherited from the original, but does ", exit non-zero!" part of
the message really add any value? I am wondering if
else
- say "With GIT_TEST_SANITIZE_LEAK_LOG=true ..., exit non-zero!"
+ say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a leak."
+ test "$test_failure" != 0 || invert_exit_code=t
fi
might be what we eventually want to end up with, after the dust
settles from this fix.
Thanks.
