ci(*-leaks): skip the git-svn tests to save time

However, testing git-svn in the leak-checking jobs provides minimal
value: git-svn is implemented as a Perl script, and leak checking only handles C code. While git-svn does call into Git's built-in commands that are implemented in C, these are standard Git operations that are already thoroughly exercised elsewhere in the test suite. Therefore, running the git-svn tests in the leak-checking jobs only adds to the overall run time with little value in return.

From: Johannes Schindelin <[email protected]>

I noticed recently that the leak-checking jobs still take a lot of time, and upon analysis, the git-svn tests contribute significantly to this.

Analyzing a recent CI run, I saw that the Git test suite contains 1,017 tests, running for approximately 5¼ hours total. Of these, 65 git-svn-related tests (~6% of test count) took 42.24 minutes combined, accounting for ~13.% of the total runtime. This implies that the git-svn tests are roughly twice as expernsive compared to the other tests.

However, testing git-svn in the leak-checking jobs provides minimal
value: git-svn is implemented as a Perl script, and leak checking only handles C code. While git-svn does call into Git's built-in commands that are implemented in C, these are standard Git operations that are already thoroughly exercised elsewhere in the test suite. Therefore, running the git-svn tests in the leak-checking jobs only adds to the overall run time with little value in return.

Given that the leak-checking jobs are particularly time-intensive and these 42+ minutes of SVN tests per job provide no additional leak detection value, skip them in the *-leaks jobs to reduce CI runtime.

Assisted-by Claude Sonnet 4.5

Signed-off-by Johannes Schindelin <[email protected]>

---
ci(*-leaks): skip the git-svn tests to save time

I leaned heavily on AI to implement this patch, in particular when analyzing the logs. That's why I added that trailer talking about Claude Sonnet. If this is undesirable, please let me know.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2031%2Fdscho%2Fskip-svn-and-leak-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2031/dscho/skip-svn-and-leak-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2031

  ci/lib.sh | 1 +
  1 file changed, 1 insertion(+)

ci/lib.sh +1 -0

index f561884d40..a165c7f268 100755
@@ -356,6 +356,7 @@ linux-musl-meson)
  	;;
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
+	export NO_SVN_TESTS=LetsSaveSomeTime
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined

base-commit: 7264e61d87e58b9d0f5e6424c47c11e9657dfb75

Looking at the CI logs for this PR the p4 and cvs tests account for another 24 minutes of test time and I suspect they also offer little in the way of extra coverage. Unfortunately there is no equivalent of NO_SVN_TESTS to disable them - I wonder if building with NO_PYTHON and NO_PERL would make sense for the leak test job?

Either way I like the direction of this patch

Thanks

Phillip

Quoted 1 line

[snip]

Yup, I generally like this direction, and introducing NO_P4_TESTS and NO_CVS_TESTS would not be so bad. Here is how it looks on top of Dscho's patch.

--- >8 ---
Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too

Looking at the CI logs, the p4 and cvs tests account for another 24 minutes of test time and they offer minimal value for quite a similar reason as the previous step.

Let's introduce and use a mechanism to skip these tests to save some resources.

Suggested-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
[snip]

Quoted 15 lines · Junio C Hamano

Quoted 1 line

[snip]

Yup, I generally like this direction, and introducing NO_P4_TESTS and NO_CVS_TESTS would not be so bad. Here is how it looks on top of Dscho's patch.

--- >8 ---
Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too

Looking at the CI logs, the p4 and cvs tests account for another 24 minutes of test time and they offer minimal value for quite a similar reason as the previous step.

Let's introduce and use a mechanism to skip these tests to save some resources.

Quoted 1 line

Suggested-by Phillip Wood <[email protected]>

Nitpick: Using the commit ident

Phillip Wood <[email protected]>

might be slightly better?

Quoted 22 lines · Kristoffer Haugsbakk

Quoted 8 lines · Junio C Hamano

Quoted 1 line

[snip]

Yup, I generally like this direction, and introducing NO_P4_TESTS and NO_CVS_TESTS would not be so bad. Here is how it looks on top of Dscho's patch.

--- >8 ---
Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too

Quoted 8 lines

Looking at the CI logs, the p4 and cvs tests account for another 24 minutes of test time and they offer minimal value for quite a similar reason as the previous step.

Let's introduce and use a mechanism to skip these tests to save some resources.

Suggested-by Phillip Wood <[email protected]>

Nitpick: Using the commit ident

Phillip Wood <[email protected]>

Quoted 1 line

might be slightly better?

I didn't even realize there are multiple addresses in play, actually. I just took it from the e-mail header's Cc: field, which my MUA copied from From: field of the message I was responding to, which was the identity of the person who suggested the change after all ;-).

Yup, I generally like this direction, and introducing NO_P4_TESTS and NO_CVS_TESTS would not be so bad. Here is how it looks on top of Dscho's patch.

--- >8 ---
Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too

Looking at the CI logs, the p4 and cvs tests account for another 24 minutes of test time and they offer minimal value for quite a similar reason as the previous step.

Let's introduce and use a mechanism to skip these tests to save some resources.

Suggested-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  ci/lib.sh       | 2 ++
  t/lib-cvs.sh    | 6 ++++++
  t/lib-git-p4.sh | 5 +++++
  3 files changed, 13 insertions(+)

ci/lib.sh +2 -0

index a165c7f268..3ecbf147db 100755
@@ -356,7 +356,9 @@ linux-musl-meson)
  	;;
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
+	export NO_CVS_TESTS=LetsSaveSomeTime
  	export NO_SVN_TESTS=LetsSaveSomeTime
+	export NO_P4_TESTS=LetsSaveSomeTime
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined

t/lib-cvs.sh +6 -0

index 57b9b2db9b..c8b4404888 100644
@@ -2,6 +2,12 @@
 
  . ./test-lib.sh
 
+if test -n "$NO_CVS_TESTS"
+then
+	skip_all='skipping git cvs tests, NO_CVS_TESTS defined'
+	test_done
+fi
+
  unset CVS_SERVER
 
  if ! type cvs >/dev/null 2>&1

t/lib-git-p4.sh +5 -0

index 2a5b8738ea..d22e9c684a 100644
@@ -16,6 +16,11 @@ P4D_TIMEOUT=300
 
  . ./test-lib.sh
 
+if test -n "$NO_P4_TESTS"
+then
+	skip_all='skipping git p4 tests, NO_P4_TESTS defined'
+	test_done
+fi
  if ! test_have_prereq PYTHON
  then
  	skip_all='skipping git p4 tests; python not available'

The patch looks good to me, it is very convenient that we can put the test in the library files rather than each test file. Should we drop these tests from the ASan job as well?

Thanks

Phillip

Quoted 55 lines

Suggested-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  ci/lib.sh       | 2 ++
  t/lib-cvs.sh    | 6 ++++++
  t/lib-git-p4.sh | 5 +++++
  3 files changed, 13 insertions(+)

ci/lib.sh +2 -0

index a165c7f268..3ecbf147db 100755
@@ -356,7 +356,9 @@ linux-musl-meson)
  	;;
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
+	export NO_CVS_TESTS=LetsSaveSomeTime
  	export NO_SVN_TESTS=LetsSaveSomeTime
+	export NO_P4_TESTS=LetsSaveSomeTime
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined

t/lib-cvs.sh +6 -0

index 57b9b2db9b..c8b4404888 100644
@@ -2,6 +2,12 @@
 
  . ./test-lib.sh
 
+if test -n "$NO_CVS_TESTS"
+then
+	skip_all='skipping git cvs tests, NO_CVS_TESTS defined'
+	test_done
+fi
+
  unset CVS_SERVER
 
  if ! type cvs >/dev/null 2>&1

t/lib-git-p4.sh +5 -0

index 2a5b8738ea..d22e9c684a 100644
@@ -16,6 +16,11 @@ P4D_TIMEOUT=300
 
  . ./test-lib.sh
 
+if test -n "$NO_P4_TESTS"
+then
+	skip_all='skipping git p4 tests, NO_P4_TESTS defined'
+	test_done
+fi
  if ! test_have_prereq PYTHON
  then
  	skip_all='skipping git p4 tests; python not available'

Quoted 3 lines · Phillip Wood

The patch looks good to me, it is very convenient that we can put the test in the library files rather than each test file. Should we drop these tests from the ASan job as well?

That's probably a good idea.

I also was wondering if we want a blanket NO_FOO_TESTS that we can use instead listing all. That FOO should not be SCM, though, as the reason why we exclude the tests is not because they are test about foreign SCM. We exclude them as low value because testing them exercises little code of ours that we may make mistakes these checks are trying to uncover and that we can fix when they do. NO_FOREIGN_CODE_TESTS? I dunno.

... If "git svn" was
implemented in C then we probably would want to check it for leaks even though it called a foreign program. That's a long winded way of saying I don't have any better suggestions!

Quoted 4 lines · Phillip Wood

... If "git svn" was
implemented in C then we probably would want to check it for leaks even though it called a foreign program. That's a long winded way of saying I don't have any better suggestions!

I am not sure if I agree. If Perl interpreter used to run the Perl version of "git svn" were found leaky, are we willing to go in and plug leaks there? Not likely, particularly since it is not what we ship and we do not have control over which version of Perl the users have on their systems. So we say "Perl is foreign and we are not equipped to plug leaks in various versions of it on users' systems, so it is not worth spending cycles to test for leaks in it".

If "git svn" were in C, linked with libsvn without using the perl binding, and libsvn were found leaky, the story is the same. We do not control the version of libsvn the users have on their systems, we are not equipped to plug leaks in there, so it is not our job to spend cycles to test for leaks in it.

I think that unless the libsvn that linked against was built with -fsanitize=leak we wouldn't find any leaks in it anyway. When I wrote my original mail I was imagining C implementation that forked "svn" but replaced the perl code with C that called the appropriate functions in libgit rather than forking git.

In that case I think there's an argument
for checking that our code does not leak. Anyway this is all rather hypothetical as we're not likely to rewrite these scripts in C.