git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rubén Justo" <rjusto@gmail.com>
To: Git List <git@vger.kernel.org>
Cc: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default
Date: Wed, 10 Jul 2024 11:30:13 +0900	[thread overview]
Message-ID: <d5c307e3-79c5-4795-838d-4a425b012ec0@gmail.com> (raw)
In-Reply-To: <598149bf-6541-4c9e-8c94-a108e3ee7fd7@gmail.com>

As we currently describe in t/README, it can happen that:

     Some tests run "git" (or "test-tool" etc.) without properly checking
     the exit code, or git will invoke itself and fail to ferry the
     abort() exit code to the original caller.

Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
capture all memory leaks triggered by our tests.

It seems unnecessary to force users to remember this option, as
forgetting it could lead to missed memory leaks.

We could solve the problem by setting GIT_TEST_SANITIZE_LEAK_LOG to
"true" by default, but that might suggest we think "false" makes sense,
which isn't the case.

Therefore, the best approach is to remove the option entirely while
maintaining the capability to detect memory leaks in blind spots of our
tests.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

@Peff: I thought your preference was to keep the option.  I agree to
remove it completely.  This, v2, address that.

I'm traveling, and while I think the change doesn't break anything, I'd
appreciate a double check ;-)

By the way, I used, more or less, the text for the message that Junio 
suggested.

  ci/lib.sh     |  1 -
  t/README      | 24 ------------------------
  t/test-lib.sh | 41 +++++++++++++++++++----------------------
  3 files changed, 19 insertions(+), 47 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 814578ffc6..51f8f59a29 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -370,7 +370,6 @@ linux-musl)
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
-	export GIT_TEST_SANITIZE_LEAK_LOG=true
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined
diff --git a/t/README b/t/README
index d9e0e07506..c2a732d59e 100644
--- a/t/README
+++ b/t/README
@@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" 
and those tests that
  pass under "SANITIZE=leak". This is especially useful when testing a
  series that fixes various memory leaks with "git rebase -x".

-GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
-"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
-"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
-make logs +machine-readable.
-
-With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
-before exiting and exit on failure if the logs showed that we had a
-memory leak, even if the test itself would have otherwise passed. This
-allows us to catch e.g. missing &&-chaining. This is especially useful
-when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
-
  GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
  will run to completion faster, and result in the same failing
  tests. The only practical reason to run
-GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
-combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
-first failing test case our leak logs won't show subsequent leaks we
-might have run into.
-
-GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
-leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
-run "git" (or "test-tool" etc.) without properly checking the exit
-code, or git will invoke itself and fail to ferry the abort() exit
-code to the original caller. When the two modes are combined we'll
-look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
-the test run to see if had memory leaks which the test itself didn't
-catch.

  GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
  default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 79d3e0e7d9..942828c55d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1270,8 +1270,8 @@ check_test_results_san_file_ () {
  		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
-		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory 
leak, exit non-zero!" &&
-		invert_exit_code=t
+		say "Our logs revealed a leak!" &&
+		test "$test_failure" != 0 || invert_exit_code=t
  	fi
  }

@@ -1555,28 +1555,28 @@ then
  		passes_sanitize_leak=t
  	fi

-	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
+	if test -z "$passes_sanitize_leak" &&
+	   ! test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" &&
+	   test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
  	then
-		sanitize_leak_check=t
-		if test -n "$invert_exit_code"
+		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+		test_done
+	else
+		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
  		then
-			BAIL_OUT "cannot use --invert-exit-code under 
GIT_TEST_PASSING_SANITIZE_LEAK=check"
-		fi
+			sanitize_leak_check=t
+			if test -n "$invert_exit_code"
+			then
+				BAIL_OUT "cannot use --invert-exit-code under 
GIT_TEST_PASSING_SANITIZE_LEAK=check"
+			fi

-		if test -z "$passes_sanitize_leak"
-		then
-			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting 
--invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
-			invert_exit_code=t
+			if test -z "$passes_sanitize_leak"
+			then
+				say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting 
--invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
+				invert_exit_code=t
+			fi
  		fi
-	elif test -z "$passes_sanitize_leak" &&
-	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
-	then
-		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
-		test_done
-	fi

-	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
-	then
  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
  		then
  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
@@ -1599,9 +1599,6 @@ elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = 
"check" ||
       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
  then
  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
-elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
-then
-	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
  fi

  if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&
-- 
2.45.1

  parent reply	other threads:[~2024-07-10  2:30 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-10  0:51 [PATCH] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default Rubén Justo
2024-07-10  1:12 ` Jeff King
2024-07-22  7:52   ` Patrick Steinhardt
2024-07-10  2:30 ` Rubén Justo [this message]
2024-07-10  3:58   ` [PATCH v2] " Junio C Hamano
2024-07-10  4:46     ` Rubén Justo
2024-07-10  7:16   ` Jeff King
2024-07-11 14:03     ` Rubén Justo
2024-07-11 14:10   ` [PATCH v3] " Rubén Justo
2024-07-17  7:02     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d5c307e3-79c5-4795-838d-4a425b012ec0@gmail.com \
    --to=rjusto@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).