| Message ID | 4adfcba4-0f2b-44f5-a312-97f00f979435@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03f72a4ed8b0d9f379db80749150a13b3ad1f2cd |
| Headers | show |
| Series | mark tests as leak-free | expand |
Rubén Justo <rjusto@gmail.com> writes: > This test is leak-free since it was added in e137fe3b29 (unit tests: add > TAP unit test framework, 2023-11-09) > > Let's mark it as leak-free to make sure it stays that way (and to reduce > noise when looking for other leak-free scripts after we fix some leaks). For other tests in this series, that rationale is a very sensible thing, but does it apply to this one? The point of the t-basic tests is to ensure the lightweight unit test framework that requires nothing from Git behaves (and keeps behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9] tests under leak sanitizer is to exercise production Git code to catch leaks in Git code. So it is not quite clear if we even want to run this t0080 under leak sanitizer to begin with. t0080 is a relatively tiny test, but do we even want to spend leak sanitizer cycles on it? I dunno. > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > t/t0080-unit-test-output.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh > index 961b54b06c..6657c114a3 100755 > --- a/t/t0080-unit-test-output.sh > +++ b/t/t0080-unit-test-output.sh > @@ -2,6 +2,7 @@ > > test_description='Test the output of the unit test framework' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'TAP output from unit tests' '
On 29-ene-2024 14:15:15, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > This test is leak-free since it was added in e137fe3b29 (unit tests: add > > TAP unit test framework, 2023-11-09) > > > > Let's mark it as leak-free to make sure it stays that way (and to reduce > > noise when looking for other leak-free scripts after we fix some leaks). > > For other tests in this series, that rationale is a very sensible > thing, but does it apply to this one? > > The point of the t-basic tests is to ensure the lightweight unit > test framework that requires nothing from Git behaves (and keeps > behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9] > tests under leak sanitizer is to exercise production Git code to > catch leaks in Git code. > > So it is not quite clear if we even want to run this t0080 under > leak sanitizer to begin with. t0080 is a relatively tiny test, but > do we even want to spend leak sanitizer cycles on it? I dunno. IIUC, that would imply building test-tool with a different set of flags than Git, new artifacts ... or running test-tool with some LSAN_OPTIONS options, to disable it ... or both ... or ... And that is assuming that with test-tool we won't catch a leak in Git that we're not seeing in the other tests ... Maybe this is tangential to this series but, while a decision is being made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check pass, which is the objective in this series. > > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > > --- > > t/t0080-unit-test-output.sh | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh > > index 961b54b06c..6657c114a3 100755 > > --- a/t/t0080-unit-test-output.sh > > +++ b/t/t0080-unit-test-output.sh > > @@ -2,6 +2,7 @@ > > > > test_description='Test the output of the unit test framework' > > > > +TEST_PASSES_SANITIZE_LEAK=true > > . ./test-lib.sh > > > > test_expect_success 'TAP output from unit tests' '
Rubén Justo <rjusto@gmail.com> writes: >> The point of the t-basic tests is to ensure the lightweight unit >> test framework that requires nothing from Git behaves (and keeps >> behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9] >> tests under leak sanitizer is to exercise production Git code to >> catch leaks in Git code. >> >> So it is not quite clear if we even want to run this t0080 under >> leak sanitizer to begin with. t0080 is a relatively tiny test, but >> do we even want to spend leak sanitizer cycles on it? I dunno. > > IIUC, that would imply building test-tool with a different set of flags > than Git, new artifacts ... or running test-tool with some LSAN_OPTIONS > options, to disable it ... or both ... or ... > > And that is assuming that with test-tool we won't catch a leak in Git > that we're not seeing in the other tests ... But t0080 does not even run test-tool, does it? The t-basic unit test is about testing the unit test framework and does not even trigger any of the half-libified Git code. So I am not sure why you are bringing up test-tool into the picture. > Maybe this is tangential to this series but, while a decision is being > made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check > pass, which is the objective in this series. One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to true is because that way the marked test will be run under the leak sanitizer in the CI. What do we expect to gain by running t0080, which is to run the t-basic unit test, under the leak sanitizer? Unlike other t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would we care about a new leak found in t-basic run from t0080 in the first place? Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself. Annotating the tests that we want to run under the sanitizer and see them passing with it is. And obviously these tests that exercise Git production code are very good candidates for us to do so. It is unclear if t0080 falls into the same category. That is why I asked what we expect to gain by running it. Thanks.
On Mon, Jan 29, 2024 at 02:15:15PM -0800, Junio C Hamano wrote: > > Let's mark it as leak-free to make sure it stays that way (and to reduce > > noise when looking for other leak-free scripts after we fix some leaks). > > For other tests in this series, that rationale is a very sensible > thing, but does it apply to this one? > > The point of the t-basic tests is to ensure the lightweight unit > test framework that requires nothing from Git behaves (and keeps > behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9] > tests under leak sanitizer is to exercise production Git code to > catch leaks in Git code. > > So it is not quite clear if we even want to run this t0080 under > leak sanitizer to begin with. t0080 is a relatively tiny test, but > do we even want to spend leak sanitizer cycles on it? I dunno. I think you are right that we do not particularly care about leaks in the t-basic code. That is also true of other test harness code (other unit-tests, but also stuff in t/helper). But IMHO it is less work to just keep that code leak-free than it is to try to distinguish between production and test code. Right now, it is not that hard to simply leave the PASSES_SANITIZE_LEAK flag off of t0080, and then it won't be run in the leak-checking CI job. But I think the end-game of all of this leak-checking stuff is that eventually _everything_ will be leak-free, and we will discard the whole PASSES_SANITIZE_LEAK mechanism entirely. And in that end-game, it is simpler for everything, including t-basic, to just be leak-free and checked under the same regime. Setting the flag now just makes sure we continue correctly on that path, rather than getting surprised near the end of the road that t-basic has some dumb leak. Plus it avoids the script popping up as a false positive when checking for scripts which can be marked. -Peff
On 29-ene-2024 15:51:33, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> The point of the t-basic tests is to ensure the lightweight unit > >> test framework that requires nothing from Git behaves (and keeps > >> behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9] > >> tests under leak sanitizer is to exercise production Git code to > >> catch leaks in Git code. > >> > >> So it is not quite clear if we even want to run this t0080 under > >> leak sanitizer to begin with. t0080 is a relatively tiny test, but > >> do we even want to spend leak sanitizer cycles on it? I dunno. > > > > IIUC, that would imply building test-tool with a different set of flags > > than Git, new artifacts ... or running test-tool with some LSAN_OPTIONS > > options, to disable it ... or both ... or ... > > > > And that is assuming that with test-tool we won't catch a leak in Git > > that we're not seeing in the other tests ... > > But t0080 does not even run test-tool, does it? The t-basic unit > test is about testing the unit test framework and does not even > trigger any of the half-libified Git code. So I am not sure why > you are bringing up test-tool into the picture. Of course, test-tool has nothing to do here. I think I got distracted because: $ ( cd t; ./t0080-unit-test-output.sh ) Bail out! You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory My reasoning was about t/unit-test/bin/t-basic (though also applies to test-tool), due to: $ make SANITIZE=leak -n t/unit-tests/bin/t-basic ... echo ' ' LINK t/unit-tests/bin/t-basic;cc -g -O2 -Wall -I. \ -DHAVE_SYSINFO -fsanitize=leak -fno-sanitize-recover=leak \ -fno-omit-frame-pointer -DSUPPRESS_ANNOTATED_LEAKS -O0 \ -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND \ -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES \ -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 \ -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" \ -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK \ -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME \ -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM \ '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES \ -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -o t/unit-tests/bin/t-basic \ t/unit-tests/t-basic.o t/unit-tests/test-lib.o common-main.o libgit.a \ xdiff/lib.a reftable/libreftable.a libgit.a xdiff/lib.a \ reftable/libreftable.a libgit.a -lz -lpthread -lrt Note that we inject this flags: -fsanitize=leak -fno-sanitize-recover=leak -fno-omit-frame-pointer \ -DSUPPRESS_ANNOTATED_LEAKS -O0 > > > Maybe this is tangential to this series but, while a decision is being > > made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check > > pass, which is the objective in this series. > > One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to > true is because that way the marked test will be run under the leak > sanitizer in the CI. > > What do we expect to gain by running t0080, which is to run the > t-basic unit test, under the leak sanitizer? Unlike other > t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would > we care about a new leak found in t-basic run from t0080 in the > first place? > > Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself. Indeed. It points to a horizon. > Annotating the tests that we want to run under the sanitizer and see > them passing with it is. Maybe this is also a horizon (not reachable by definition), and expecting "make test" to be leak-free (including t0080) a good path towards that horizon, IMHO. But you are right, those leak sanitizer cycles may not be worth it. > And obviously these tests that exercise > Git production code are very good candidates for us to do so. It is > unclear if t0080 falls into the same category. That is why I asked > what we expect to gain by running it. > > Thanks. Thank you for bringing up a good question. I see you queued this as 4/4. OK. I'll consider that if a re-roll for this series is needed.
Jeff King <peff@peff.net> writes: > Setting the flag now just makes sure we continue correctly on that path, > rather than getting surprised near the end of the road that t-basic has > some dumb leak. Plus it avoids the script popping up as a false positive > when checking for scripts which can be marked. Alright. Any such "dumb leak" in the "basic" would hopefully be caught by the real t-$other unit tests exercised under the leak sanitizer, I hope, but it is not worth our time wondering if it makes sense to special case t0080 specifically.
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh index 961b54b06c..6657c114a3 100755 --- a/t/t0080-unit-test-output.sh +++ b/t/t0080-unit-test-output.sh @@ -2,6 +2,7 @@ test_description='Test the output of the unit test framework' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'TAP output from unit tests' '
This test is leak-free since it was added in e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09) Let's mark it as leak-free to make sure it stays that way (and to reduce noise when looking for other leak-free scripts after we fix some leaks). Signed-off-by: Rubén Justo <rjusto@gmail.com> --- t/t0080-unit-test-output.sh | 1 + 1 file changed, 1 insertion(+)