threads / patch / 59072

patch, 3 partsfixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

Subject: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

## tl;dr

8 messages between Jan 11, 2023 and Jan 14, 2023. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Andrei Rybak· Jan 11, 2023, 23:32 UTC · lore
[ I apologize for some people getting this twice -- I messed up when
  invoking `git send-email` ]
On 2023-01-06T19:25, Eric Sunshine wrote:
Show 10 quoted lines
> Not related to your patch at all, but I notice in this test that the
> call to test_when_finished() is commented out:
> 
>     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> 
> which makes me wonder if it was commented out while the test was being
> debugged but then forgotten, and that the script is now potentially
> leaking a running daemon if something in the test fails after the
> daemon was started, or if the daemon does not shut down on its own as
> it's supposed to do. [cc:+Jeff Hostetler]
Here's a patch series that fixes some of the commented out test code.
I skipped changing the following:
1. a minute-long test_expect_failure is commented out in t0014-alias.sh .
   Technically, this could be uncommented and marked with `EXPENSIVE`
   prerequisite, but it doesn't seem worth it for a `test_expect_failure`.
   [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an
   alias testing suite, 2018-09-16) ]
2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
   b & add c VS rename b->c' is commented out with an explicit "# FIXME:
   rename/add conflicts are horribly broken right now;" above the commented out
   part.
   [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
   updates can be skipped in a merge, 2018-04-19) ]
3. I've experimented a bit with commented out test in file
   t9200-git-cvsexportcommit.sh.  Tests in this file rely on state from previous
   tests, which complicates more thorough investigation.  Unfortunately,  I
   didn't have bandwidth to investigate this further.
   [ cc Robin Rosenberg, who added it in fe142b3a45 (Rework cvsexportcommit to
   handle binary files for all cases., 2006-11-12) and commented out in
   e86ad71fe5 (Make cvsexportcommit work with filenames with spaces and
   non-ascii characters., 2006-12-11) ]
I found these places in tests with:
    git grep -P '^\s*[#]\s*test_' -- 't/t[0-9]*'
The rest of the results of this grep are documentation comments.
Andrei Rybak (3):
  t6003: uncomment test '--max-age=c3, --topo-order'
  t6422: drop commented out code
  t7527: uncomment test_when_finished step in a test
 t/t6003-rev-list-topo-order.sh       | 23 ++++++++++-------------
 t/t6422-merge-rename-corner-cases.sh |  2 --
 t/t7527-builtin-fsmonitor.sh         |  2 +-
 3 files changed, 11 insertions(+), 16 deletions(-)
-- 
2.39.0
Andrei Rybak· Jan 11, 2023, 23:32 UTC · re: Andrei Rybak · lore

[PATCH v1 1/3] t6003: uncomment test '--max-age=c3, --topo-order'

Test '--max-age=c3, --topo-order' in t6003-rev-list-topo-order.sh has been commented out as failing since its introduction in [1]. However, the test is successful at least since commit [2] -- bisecting further is harder because of incompatibility of such old Git code with modern header file <openssl/bn.h> [3].

Uncomment this test to gain test coverage.
[1] f573571a21 ([PATCH] Add t/t6003 with some --topo-order tests,
    2005-07-07)
[2] 765ac8ec46 (Rip out merge-order and make "git log <paths>..." work
    again., 2006-02-28)
[3] BIGNUM used in git's `epoch.c` which was removed in [2] changed
    significantly between OpenSSL 1.0.2 and OpenSSL 1.1.0
    See also https://stackoverflow.com/a/42295243/1083697 and
    https://lore.kernel.org/git/Y71qiCs+oAS2OegH@coredump.intra.peff.net/
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t6003-rev-list-topo-order.sh | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
Show changes to t/t6003-rev-list-topo-order.sh +10 −13
diff --git a/t/t6003-rev-list-topo-order.sh b/t/t6003-rev-list-topo-order.sh
index 1f7d7dd20c..5cf2cee74d 100755
--- a/t/t6003-rev-list-topo-order.sh
+++ b/t/t6003-rev-list-topo-order.sh
@@ -326,19 +326,16 @@ a2
 c3
 EOF
 
-#
-# this test fails on --topo-order - a fix is required
-#
-#test_output_expect_success '--max-age=c3, --topo-order' "git rev-list --topo-order --max-age=$(commit_date c3) l5" <<EOF
-#l5
-#l4
-#l3
-#a4
-#c3
-#b4
-#a3
-#a2
-#EOF
+test_output_expect_success '--max-age=c3, --topo-order' "git rev-list --topo-order --max-age=$(commit_date c3) l5" <<EOF
+l5
+l4
+l3
+a4
+c3
+b4
+a3
+a2
+EOF
 
 test_output_expect_success 'one specified head reachable from another a4, c3, --topo-order' "list_duplicates git rev-list --topo-order a4 c3" <<EOF
 EOF
-- 
2.39.0
Andrei Rybak· Jan 11, 2023, 23:32 UTC · re: Andrei Rybak · lore

[PATCH v1 2/3] t6422: drop commented out code

In commit [1] tests in t6422-merge-rename-corner-cases.sh were refactored to not run setup steps separately. This included replacing all tests like

	test_expect_success "setup ..." '
		<code of setup>
	'
with corresponding Shell functions
	test_setup_... () {
		<code of setup>
	}

During this replacement first and last lines of one of such tests got left commented out in code. Drop these lines to avoid confusion.

[1] da1e295e00 (t604[236]: do not run setup in separate tests, 2019-10-22)
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t6422-merge-rename-corner-cases.sh | 2 --
 1 file changed, 2 deletions(-)
Show changes to t/t6422-merge-rename-corner-cases.sh +0 −2
diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
index 346253c7c8..076b6a74d5 100755
--- a/t/t6422-merge-rename-corner-cases.sh
+++ b/t/t6422-merge-rename-corner-cases.sh
@@ -1159,7 +1159,6 @@ test_conflicts_with_adds_and_renames() {
 	#   4) There should not be any three~* files in the working
 	#      tree
 	test_setup_collision_conflict () {
-	#test_expect_success "setup simple $sideL/$sideR conflict" '
 		git init simple_${sideL}_${sideR} &&
 		(
 			cd simple_${sideL}_${sideR} &&
@@ -1236,7 +1235,6 @@ test_conflicts_with_adds_and_renames() {
 			fi &&
 			test_tick && git commit -m R
 		)
-	#'
 	}
 
 	test_expect_success "check simple $sideL/$sideR conflict" '
-- 
2.39.0
Andrei Rybak· Jan 11, 2023, 23:32 UTC · re: Andrei Rybak · lore

[PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'

Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the helper function test_when_finished with stop_daemon_delete_repo. Function stop_daemon_delete_repo explicitly stops the daemon. Calling it via test_when_finished is needed for tests that don't check daemon's automatic shutdown logic [1] and it is needed to avoid daemons being left running in case of breakage of the logic of automatic shutdown of the daemon.

Unlike these tests, test 'case insensitive+preserving' added in [2] has a call to function test_when_finished commented out. It was commented out in all versions of the patch [2] during development [3]. This seems to not be intentional, because neither commit message in [2], nor the comment above the test mention this line being commented out. Compare it, for example, to "# unicode_debug=true" which is explicitly described by a documentation comment above it.

Uncomment test_when_finished for stop_daemon_delete_repo in test 'case insensitive+preserving' to ensure that daemons are not left running in cases when automatic shutdown logic of daemon itself is broken.

[1] See documentation in "fsmonitor--daemon.h" for details.
[2] caa9c37ec0 (t7527: test FSMonitor on case insensitive+preserving
    file system, 2022-05-26)
[3] See mailing list thread
    https://lore.kernel.org/git/41f8cbc2ae45cb86e299eb230ad3cb0319256c37.1653601644.git.gitgitgadget@gmail.com/T/#t
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t7527-builtin-fsmonitor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7527-builtin-fsmonitor.sh +1 −1
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 76d0220daa..2c271b4d7e 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -910,7 +910,7 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
 # the file/directory.
 #
 test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '
-#	test_when_finished "stop_daemon_delete_repo test_insensitive" &&
+	test_when_finished "stop_daemon_delete_repo test_insensitive" &&
 
 	git init test_insensitive &&
 
-- 
2.39.0
Eric Sunshine· Jan 14, 2023, 09:43 UTC · re: Andrei Rybak · lore

Re: [PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'

On Wed, Jan 11, 2023 at 7:00 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
Show 27 quoted lines
> Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the
> helper function test_when_finished with stop_daemon_delete_repo.
> Function stop_daemon_delete_repo explicitly stops the daemon.  Calling
> it via test_when_finished is needed for tests that don't check daemon's
> automatic shutdown logic [1] and it is needed to avoid daemons being
> left running in case of breakage of the logic of automatic shutdown of
> the daemon.
>
> Unlike these tests, test 'case insensitive+preserving' added in [2] has
> a call to function test_when_finished commented out.  It was commented
> out in all versions of the patch [2] during development [3].  This seems
> to not be intentional, because neither commit message in [2], nor the
> comment above the test mention this line being commented out.  Compare
> it, for example, to "# unicode_debug=true" which is explicitly described
> by a documentation comment above it.
>
> Uncomment test_when_finished for stop_daemon_delete_repo in test 'case
> insensitive+preserving' to ensure that daemons are not left running in
> cases when automatic shutdown logic of daemon itself is broken.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> @@ -910,7 +910,7 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
>  test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '
> -#      test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> +       test_when_finished "stop_daemon_delete_repo test_insensitive" &&

Nice. Thanks for working on this (and the series as a whole) in response to a very tangential comment of mine[1] when replying to an unrelated patch of yours.

[1]: https://lore.kernel.org/git/CAPig+cTgUPWxMox_nSka52dML6_GHUUoY4HCtcq7+7J0oEyeNw@mail.gmail.com/
Tim Schumacher· Jan 12, 2023, 00:45 UTC · re: Andrei Rybak · lore

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

On 12.01.23 00:32, Andrei Rybak wrote:
Show 11 quoted lines
> [...]
>
> Here's a patch series that fixes some of the commented out test code.
>
> I skipped changing the following:
>
> 1. a minute-long test_expect_failure is commented out in t0014-alias.sh .
>     Technically, this could be uncommented and marked with `EXPENSIVE`
>     prerequisite, but it doesn't seem worth it for a `test_expect_failure`.
>     [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an
>     alias testing suite, 2018-09-16) ]

The reason why this particular test is commented out (and why it mentions a run time of one minute) is because support for detecting external alias loops isn't yet implemented. This means that running that test would spin the test runner until the test times out due to an intentional infinite loop.

As soon as that is implemented properly, git would ideally detect the loop after a few iterations at latest, so the test wouldn't require to be marked as 'EXPENSIVE' in the first place.

For the context of your patches, skipping adjusting this test is most likely fine, as it references currently unimplemented behavior and it presumably would require more adjustments anyways before finally being enabled.

>
> [...]
>
Tim
Elijah Newren· Jan 12, 2023, 01:54 UTC · re: Andrei Rybak · lore

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
Show 17 quoted lines
>
> [ I apologize for some people getting this twice -- I messed up when
>   invoking `git send-email` ]
>
> On 2023-01-06T19:25, Eric Sunshine wrote:
> > Not related to your patch at all, but I notice in this test that the
> > call to test_when_finished() is commented out:
> >
> >     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> >
> > which makes me wonder if it was commented out while the test was being
> > debugged but then forgotten, and that the script is now potentially
> > leaking a running daemon if something in the test fails after the
> > daemon was started, or if the daemon does not shut down on its own as
> > it's supposed to do. [cc:+Jeff Hostetler]
>
> Here's a patch series that fixes some of the commented out test code.

Patch 2 is obviously correct. Patches 1 & 3 make sense to me, but it would be nice to have someone familiar with fsmonitor look at #3.

As to your notes about other related testcases...
> I skipped changing the following:
[...]
Show 6 quoted lines
> 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
>    b & add c VS rename b->c' is commented out with an explicit "# FIXME:
>    rename/add conflicts are horribly broken right now;" above the commented out
>    part.
>    [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
>    updates can be skipped in a merge, 2018-04-19) ]
[...]

You missed the cc...but I looked up this email since you did cc me for patch 2/3.

Yeah, the commented out code was never tested, because the only thing I could have tested at the time was incorrect results. So I just took a guess at what the improved testing would look like and apparently made 3 small errors in doing so. I have fixed it locally and can submit a patch.

Elijah Newren· Jan 13, 2023, 04:32 UTC · re: Elijah Newren · lore

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

On Wed, Jan 11, 2023 at 5:54 PM Elijah Newren <newren@gmail.com> wrote:
Show 43 quoted lines
>
> On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> >
> > [ I apologize for some people getting this twice -- I messed up when
> >   invoking `git send-email` ]
> >
> > On 2023-01-06T19:25, Eric Sunshine wrote:
> > > Not related to your patch at all, but I notice in this test that the
> > > call to test_when_finished() is commented out:
> > >
> > >     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> > >
> > > which makes me wonder if it was commented out while the test was being
> > > debugged but then forgotten, and that the script is now potentially
> > > leaking a running daemon if something in the test fails after the
> > > daemon was started, or if the daemon does not shut down on its own as
> > > it's supposed to do. [cc:+Jeff Hostetler]
> >
> > Here's a patch series that fixes some of the commented out test code.
>
> Patch 2 is obviously correct.  Patches 1 & 3 make sense to me, but it
> would be nice to have someone familiar with fsmonitor look at #3.
>
> As to your notes about other related testcases...
>
> > I skipped changing the following:
> [...]
> > 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
> >    b & add c VS rename b->c' is commented out with an explicit "# FIXME:
> >    rename/add conflicts are horribly broken right now;" above the commented out
> >    part.
> >    [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
> >    updates can be skipped in a merge, 2018-04-19) ]
> [...]
>
> You missed the cc...but I looked up this email since you did cc me for
> patch 2/3.
>
> Yeah, the commented out code was never tested, because the only thing
> I could have tested at the time was incorrect results.  So I just took
> a guess at what the improved testing would look like and apparently
> made 3 small errors in doing so.  I have fixed it locally and can
> submit a patch.

Submitted here: https://lore.kernel.org/git/pull.1462.git.1673584084761.gitgitgadget@gmail.com/

← back to recent threads