diff mbox series

[v2,09/13] git-p4: use `test_atexit` to kill the daemon

Message ID 3e2193a73de0b68d5a38f1792642c828f4aed1db.1539598316.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Offer to run CI/PR builds in Azure Pipelines | expand

Commit Message

Johannes Schindelin via GitGitGadget Oct. 15, 2018, 10:12 a.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

This should be more reliable than the current method, and prepares the
test suite for a consistent way to clean up before re-running the tests
with different options.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-git-p4.sh                            | 10 +---------
 t/t0000-basic.sh                           |  2 ++
 t/t9800-git-p4-basic.sh                    |  4 ----
 t/t9801-git-p4-branch.sh                   |  4 ----
 t/t9802-git-p4-filetype.sh                 |  4 ----
 t/t9803-git-p4-shell-metachars.sh          |  4 ----
 t/t9804-git-p4-label.sh                    |  4 ----
 t/t9805-git-p4-skip-submit-edit.sh         |  4 ----
 t/t9806-git-p4-options.sh                  |  5 -----
 t/t9807-git-p4-submit.sh                   |  4 ----
 t/t9808-git-p4-chdir.sh                    |  4 ----
 t/t9809-git-p4-client-view.sh              |  4 ----
 t/t9810-git-p4-rcs.sh                      |  4 ----
 t/t9811-git-p4-label-import.sh             |  5 -----
 t/t9812-git-p4-wildcards.sh                |  4 ----
 t/t9813-git-p4-preserve-users.sh           |  4 ----
 t/t9814-git-p4-rename.sh                   |  4 ----
 t/t9815-git-p4-submit-fail.sh              |  4 ----
 t/t9816-git-p4-locked.sh                   |  4 ----
 t/t9817-git-p4-exclude.sh                  |  4 ----
 t/t9818-git-p4-block.sh                    |  4 ----
 t/t9819-git-p4-case-folding.sh             |  4 ----
 t/t9820-git-p4-editor-handling.sh          |  4 ----
 t/t9821-git-p4-path-variations.sh          |  4 ----
 t/t9822-git-p4-path-encoding.sh            |  4 ----
 t/t9823-git-p4-mock-lfs.sh                 |  4 ----
 t/t9824-git-p4-git-lfs.sh                  |  4 ----
 t/t9825-git-p4-handle-utf16-without-bom.sh |  4 ----
 t/t9826-git-p4-keep-empty-commits.sh       |  4 ----
 t/t9827-git-p4-change-filetype.sh          |  4 ----
 t/t9828-git-p4-map-user.sh                 |  4 ----
 t/t9829-git-p4-jobs.sh                     |  4 ----
 t/t9830-git-p4-symlink-dir.sh              |  4 ----
 t/t9831-git-p4-triggers.sh                 |  4 ----
 t/t9832-unshelve.sh                        |  4 ----
 t/t9833-errors.sh                          |  5 -----
 36 files changed, 3 insertions(+), 148 deletions(-)

Comments

Luke Diamand Oct. 15, 2018, 11 a.m. UTC | #1
On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
>

I'm finding that it's leaving p4d processes lying around.

e.g.

$ ./t9820-git-p4-editor-handling.sh
<passes>
$ ./t9820-git-p4-editor-handling.sh
<fails>

And also

$ ./t9800-git-p4-basic.sh
<starts running tests, but I get bored easily>
Ctrl-C

$ ./t9800-git-p4-basic.sh
<fails>

$ ps | grep p4d
21392 pts/1    00:00:00 p4d  <<<<<


Luke
Johannes Schindelin Oct. 15, 2018, 3:02 p.m. UTC | #2
Hi Luke,

On Mon, 15 Oct 2018, Luke Diamand wrote:

> On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This should be more reliable than the current method, and prepares the
> > test suite for a consistent way to clean up before re-running the tests
> > with different options.
> >
> 
> I'm finding that it's leaving p4d processes lying around.

That's a bummer!

> e.g.
> 
> $ ./t9820-git-p4-editor-handling.sh
> <passes>
> $ ./t9820-git-p4-editor-handling.sh
> <fails>

Since I do not currently have a setup with p4d installed, can you run that
with `sh -x ...` and see whether this code path is hit?

 test_done () {
        GIT_EXIT_OK=t

+       test -n "$immediate" || test_atexit_handler
+
        if test -n "$write_junit_xml" && test -n "$junit_xml_path"
        then

> And also
> 
> $ ./t9800-git-p4-basic.sh
> <starts running tests, but I get bored easily>
> Ctrl-C

Oh, you're right. I think I need to do something in this line:

	trap 'exit $?' INT

in t/test-lib.sh, something like

	trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT

would you agree? (And: could you test?)

Thanks,
Dscho

> $ ./t9800-git-p4-basic.sh
> <fails>
> 
> $ ps | grep p4d
> 21392 pts/1    00:00:00 p4d  <<<<<
> 
> 
> Luke
>
Luke Diamand Oct. 15, 2018, 8:19 p.m. UTC | #3
On Mon, 15 Oct 2018 at 16:02, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Luke,
>
> On Mon, 15 Oct 2018, Luke Diamand wrote:
>
> > On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > This should be more reliable than the current method, and prepares the
> > > test suite for a consistent way to clean up before re-running the tests
> > > with different options.
> > >
> >
> > I'm finding that it's leaving p4d processes lying around.
>
> That's a bummer!
>
> > e.g.
> >
> > $ ./t9820-git-p4-editor-handling.sh
> > <passes>
> > $ ./t9820-git-p4-editor-handling.sh
> > <fails>
>
> Since I do not currently have a setup with p4d installed, can you run that
> with `sh -x ...` and see whether this code path is hit?

All you need to do is to put p4 and p4d in your PATH.

https://www.perforce.com/downloads/helix-core-p4d
https://www.perforce.com/downloads/helix-command-line-client-p4

The server is free to use for a small number of users, you don't need
to do anything to make it go.


>
>  test_done () {
>         GIT_EXIT_OK=t
>
> +       test -n "$immediate" || test_atexit_handler
> +

+ test -n
+ test_atexit_handler
./t9820-git-p4-editor-handling.sh: 764:
./t9820-git-p4-editor-handling.sh: test_atexit_handler: not found

Is that expected?




>         if test -n "$write_junit_xml" && test -n "$junit_xml_path"
>         then
>
> > And also
> >
> > $ ./t9800-git-p4-basic.sh
> > <starts running tests, but I get bored easily>
> > Ctrl-C
>
> Oh, you're right. I think I need to do something in this line:
>
>         trap 'exit $?' INT
>
> in t/test-lib.sh, something like
>
>         trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT
>
> would you agree? (And: could you test?)

Not sure.
Send me a patch and I can try it out.

Thanks,
Luke
Eric Sunshine Oct. 16, 2018, 3:34 a.m. UTC | #4
On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> @@ -134,6 +134,7 @@ check_sub_test_lib_test_err () {
> +cat >/dev/null <<\DDD
>  test_expect_success 'pretend we have a fully passing test suite' "
>         run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
>         for i in 1 2 3
> @@ -820,6 +821,7 @@ test_expect_success 'tests clean up even on failures' "
>         > 1..2
>         EOF
>  "
> +DDD

Is this "DDD" here-doc leftover debugging goop?
Johannes Schindelin Oct. 16, 2018, 8:51 a.m. UTC | #5
Hi Eric,

On Mon, 15 Oct 2018, Eric Sunshine wrote:

> On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > This should be more reliable than the current method, and prepares the
> > test suite for a consistent way to clean up before re-running the tests
> > with different options.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > @@ -134,6 +134,7 @@ check_sub_test_lib_test_err () {
> > +cat >/dev/null <<\DDD
> >  test_expect_success 'pretend we have a fully passing test suite' "
> >         run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
> >         for i in 1 2 3
> > @@ -820,6 +821,7 @@ test_expect_success 'tests clean up even on failures' "
> >         > 1..2
> >         EOF
> >  "
> > +DDD
> 
> Is this "DDD" here-doc leftover debugging goop?

Oy, oy, oy. This is definitely a left-over from debugging (as you can
imagine, it is pretty slow to run t0000-init.sh on Windows, and if I add a
test case, the development cycle is much faster with the trick you see
aboive). This left-over even made it into Git for Windows' `master`
branch! (Which is the reason I missed it before contributing v2).

Will fix,
Dscho
Johannes Schindelin Oct. 16, 2018, 9:39 a.m. UTC | #6
Hi Luke,

On Mon, 15 Oct 2018, Luke Diamand wrote:

> On Mon, 15 Oct 2018 at 16:02, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > Hi Luke,
> >
> > On Mon, 15 Oct 2018, Luke Diamand wrote:
> >
> > > On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > > >
> > > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > >
> > > > This should be more reliable than the current method, and prepares the
> > > > test suite for a consistent way to clean up before re-running the tests
> > > > with different options.
> > > >
> > >
> > > I'm finding that it's leaving p4d processes lying around.
> >
> > That's a bummer!
> >
> > > e.g.
> > >
> > > $ ./t9820-git-p4-editor-handling.sh
> > > <passes>
> > > $ ./t9820-git-p4-editor-handling.sh
> > > <fails>
> >
> > Since I do not currently have a setup with p4d installed, can you run that
> > with `sh -x ...` and see whether this code path is hit?
> 
> All you need to do is to put p4 and p4d in your PATH.
> 
> https://www.perforce.com/downloads/helix-core-p4d
> https://www.perforce.com/downloads/helix-command-line-client-p4

I did download p4d.exe and p4.exe to $HOME/custom/p4/ (similar to the way
ci/install-dependencies.sh does it), from

	http://filehost.perforce.com/perforce/r18.1/bin.ntx64/p4d.exe
	http://filehost.perforce.com/perforce/r18.1/bin.ntx64/p4.exe

and then prefixed PATH with $HOME/custom/p4, just to run t9820. However,
it does not work here at all:

-- snip --
[...]
++ git p4 clone '--dest=/usr/src/git/azure-pipelines/t/trash directory.t9820-git-p4-editor-handling/git' //depot
Usage: git-p4 clone [options] //depot/path[@revRange]

[...]
-- snap --

I *think* the reason for that is that the MSYS path mangling kicks in when
`git.exe` is called with the `//depot` argument (the leading `//` can be
used in MSYS and MSYS2 to indicate that the non-MSYS .exe wants an
argument that starts with a single slash, but is not a path that needs to
be translated to a Windows path).

So I did the next best thing I could do: try things in the Windows
Subsystem for Linux (AKA Bash on Ubuntu on Windows).

> The server is free to use for a small number of users, you don't need
> to do anything to make it go.
> 
> 
> >
> >  test_done () {
> >         GIT_EXIT_OK=t
> >
> > +       test -n "$immediate" || test_atexit_handler

On a slight tangent: I made up my mind on the `test -n "$immediate"` part:
I will skip that. The daemon should be stopped also when `-i` was passed
to the test script (to stop at the first failing test case). There is
very, very little use in keeping the daemon alive in that case.

The commit message of "tests: introduce `test_atexit`" already made the
case for that, but somehow I had not made up my mind yet.

> > +
> 
> + test -n
> + test_atexit_handler
> ./t9820-git-p4-editor-handling.sh: 764:
> ./t9820-git-p4-editor-handling.sh: test_atexit_handler: not found
> 
> Is that expected?

No, that is not expected. For me, it looks quite a bit different, though:

-- snip --
[...]
+ test_done
+ GIT_EXIT_OK=t
+ test -n
+ test_atexit_handler
+ test : != { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
+ setup_malloc_check
+ MALLOC_CHECK_=3 MALLOC_PERTURB_=165
+ export MALLOC_CHECK_ MALLOC_PERTURB_
+ test_eval_ { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
+ test_eval_inner_ { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
+ eval
                want_trace && set -x
                { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
+ want_trace
+ test  = t
+ kill_p4d
+ cat /home/wsl/git/wip/t/trash directory.t9820-git-p4-editor-handling/p4d.pid
+ pid=20093
+ retry_until_fail kill 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ timeout=1539681637
+ kill 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ test 1539681577 -gt 1539681637
+ sleep 1
+ true
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ test 1539681578 -gt 1539681871
+ sleep 1
+ kill 20093
+ retry_until_fail kill -9 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ timeout=1539681638
+ kill -9 20093
+ test_must_fail kill 20093
+ _test_ok=
+ kill 20093
+ exit_code=1
+ test 1 -eq 0
+ test_match_signal 13 1
+ test 1 = 141
+ test 1 = 269
+ return 1
+ test 1 -gt 129
+ test 1 -eq 127
+ test 1 -eq 126
+ return 0
[...]
-- snap --

As you can see, it works quite well over here.

Maybe I could trouble you to fetch the `vsts-ci` branch from
https://github.com/dscho/git and test things on that one, just in case
that anything important got "lost in the mail"?

> >         if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> >         then
> >
> > > And also
> > >
> > > $ ./t9800-git-p4-basic.sh
> > > <starts running tests, but I get bored easily>
> > > Ctrl-C
> >
> > Oh, you're right.

So I tested this in WSL, and the relevant part of the output is this:

-- snip --
[...]
+ eval test_prereq_lazily_UTF8_NFD_TO_NFC=$2
+ test_prereq_lazily_UTF8_NFD_TO_NFC=
        # check whether FS converts nfd unicode to nfc
        au^CTraceback (most recent call last):
  File "/home/virtualbox/git/wip/git-p4", line 4143, in <module>
    main()
  File "/home/virtualbox/git/wip/git-p4", line 4137, in main
    if not cmd.run(args):
  File "/home/virtualbox/git/wip/git-p4", line 3925, in run
    if not P4Sync.run(self, depotPaths):
  File "/home/virtualbox/git/wip/git-p4", line 3824, in run
    system(["git", "symbolic-ref", head_ref, self.branch])
  File "/home/virtualbox/git/wip/git-p4", line 278, in system
    retcode = subprocess.call(cmd, shell=expand)
  File "/usr/lib/python2.7/subprocess.py", line 523, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 1392, in wait
    pid, sts = _eintr_retry_call(os.waitpid, self.pid, 0)
  File "/usr/lib/python2.7/subprocess.py", line 476, in _eintr_retry_call
    return func(*args)
KeyboardInterrupt
+ exit 130
+ die
+ code=130
+ test_atexit_handler
+ test : != { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
[...]
-- snap --

As you can see, the Ctrl+C managed to call `die` very well, without my
proposed change:

> > I think I need to do something in this line:
> >
> >         trap 'exit $?' INT
> >
> > in t/test-lib.sh, something like
> >
> >         trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT
> >
> > would you agree? (And: could you test?)
> 
> Not sure.

I *think* what happens is that the INT trap already calls `exit 130`
alright, and then the EXIT trap comes into effect, as planned.

Just to make sure that it was not Python that caused the exit code 130, I
tried it again, and luckily hit a spot where shell code was interpreted:

-- snip --
[...]
+ test_prereq_lazily_LONG_IS_64BIT=
        test 8 -le "$(build_option sizeof-long)"

+ test_lazy_^C+ exit 130
+ die
+ code=130
+ test_atexit_handler
+ test : != { kill_p4d
                } && (exit "$eval_ret"); eval_ret=$?; :
[...]
-- snap --

Same here. The Ctrl+C triggers an `exit 130`, this time I am certain that
it comes from the INT trap, and that `die` in turn is most definitely from
the EXIT trap, as intended.

My tests were performed with Dash, BTW, not with Bash:

-- snip --
$ dpkg -S $(which sh)
diversion by dash from: /bin/sh
diversion by dash to: /bin/sh.distrib
dash: /bin/sh
-- snap --

But I also repeated the tests with Bash, with almost the same result: the
only difference was that I did not manage to get the traces of
test_atexit_handler being called, but the p4d was no longer alive, so it
must have been killed as intended.

> Send me a patch and I can try it out.

I would *love* to figure out what is happening in your setup, and to fix
it. In my hands, the patches work, though...

Ciao,
Dscho

> 
> Thanks,
> Luke
>
SZEDER Gábor Oct. 17, 2018, 11:29 p.m. UTC | #7
On Mon, Oct 15, 2018 at 03:12:11AM -0700, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.

This patch makes the test suite hang in all four Travis CI build jobs
with P4 installed without any of the P4 tests finishing.

Reverting this patch from the whole patch series makes it work again.

I've also tried to revert only this first hunk of the patch below,
because based on the comment I thought it's worth a try, but it didn't
really help.  It did make a difference: the 300s watchdog timer
eventually kicked in, and then the test scripts could finish
successfully...  but there are a lot of P4 test scripts, and with each
taking 300s the build job still timeouted.

All this may (or may not) be related to and be a different symptom of
the leftover p4d processes Luke mentioned.  I couldn't reproduce any
of this on my machine so far.

> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index c27599474c..f4f5d7d296 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -74,15 +74,6 @@ cli="$TRASH_DIRECTORY/cli"
>  git="$TRASH_DIRECTORY/git"
>  pidfile="$TRASH_DIRECTORY/p4d.pid"
>  
> -# Sometimes "prove" seems to hang on exit because p4d is still running
> -cleanup () {
> -	if test -f "$pidfile"
> -	then
> -		kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
> -	fi
> -}
> -trap cleanup EXIT
> -
>  # git p4 submit generates a temp file, which will
>  # not get cleaned up if the submission fails.  Don't
>  # clutter up /tmp on the test machine.
diff mbox series

Patch

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..f4f5d7d296 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -74,15 +74,6 @@  cli="$TRASH_DIRECTORY/cli"
 git="$TRASH_DIRECTORY/git"
 pidfile="$TRASH_DIRECTORY/p4d.pid"
 
-# Sometimes "prove" seems to hang on exit because p4d is still running
-cleanup () {
-	if test -f "$pidfile"
-	then
-		kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
-	fi
-}
-trap cleanup EXIT
-
 # git p4 submit generates a temp file, which will
 # not get cleaned up if the submission fails.  Don't
 # clutter up /tmp on the test machine.
@@ -141,6 +132,7 @@  start_p4d () {
 		# p4d failed to start
 		return 1
 	fi
+	test_atexit kill_p4d
 
 	# build a p4 user so author@example.com has an entry
 	p4_add_user author
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 8c5faa6ce1..041bd7e3ce 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -134,6 +134,7 @@  check_sub_test_lib_test_err () {
 	)
 }
 
+cat >/dev/null <<\DDD
 test_expect_success 'pretend we have a fully passing test suite' "
 	run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
 	for i in 1 2 3
@@ -820,6 +821,7 @@  test_expect_success 'tests clean up even on failures' "
 	> 1..2
 	EOF
 "
+DDD
 
 test_expect_success 'test_atexit is run' "
 	test_must_fail run_sub_test_lib_test \
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 729cd25770..5856563068 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -326,8 +326,4 @@  test_expect_success 'submit from worktree' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 6a86d6996b..50013132c8 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -610,8 +610,4 @@  test_expect_success 'Update a file in git side and submit to P4 using client vie
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 9978352d78..94edebe272 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -333,8 +333,4 @@  test_expect_success SYMLINKS 'empty symlink target' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index d5c3675100..2913277013 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh
@@ -105,8 +105,4 @@  test_expect_success 'branch with shell char' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index e30f80e617..3236457106 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -108,8 +108,4 @@  test_expect_failure 'two labels on the same changelist' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 5fbf904dc8..90ef647db7 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -98,8 +98,4 @@  test_expect_success 'no config, edited' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index 3f5291b857..4e794a01bf 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -300,9 +300,4 @@  test_expect_success 'use --git-dir option and GIT_DIR' '
 	test_path_is_file "$git"/cli_file2.t
 '
 
-
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 2325599ee6..488d916c10 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -542,8 +542,4 @@  test_expect_success 'submit --update-shelve' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 11d2b5102c..58a9b3b71e 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -83,8 +83,4 @@  test_expect_success SYMLINKS 'p4 client root symlink should stay symbolic' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 897b3c3034..3cff1fce1b 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -836,8 +836,4 @@  test_expect_success 'quotes on both sides' '
 	git_verify "cdir 1/file11" "cdir 1/file12"
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index cc53debe19..57b533dc6f 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -360,8 +360,4 @@  test_expect_failure 'Add keywords in git which do not match the default p4 value
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 602b0a5d5c..b70e81c3cd 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -259,9 +259,4 @@  test_expect_success 'importing labels with missing revisions' '
 	)
 '
 
-
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 0206771fbb..254a7c2446 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -211,8 +211,4 @@  test_expect_success 'wildcard files requiring keyword scrub' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 783c6ad165..fd018c87a8 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -138,8 +138,4 @@  test_expect_success 'not preserving user with mixed authorship' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 60baa06e27..468767cbf4 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -242,8 +242,4 @@  test_expect_success P4D_HAVE_CONFIGURABLE_RUN_MOVE_ALLOW \
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index eaf03a6563..9779dc0d11 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -422,8 +422,4 @@  test_expect_success 'cleanup chmod after submit cancel' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index d048bd33fa..932841003c 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -138,8 +138,4 @@  test_expect_failure 'move with lock taken' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index aac568eadf..96d25f0c02 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -64,8 +64,4 @@  test_expect_success 'clone, then sync with exclude' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index ce7cb22ad3..0db7ab9918 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -146,8 +146,4 @@  test_expect_success 'Clone repo with self-sizing block size' '
 	test_line_count \> 10 log
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index d808c008c1..600ce1e0b0 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -53,8 +53,4 @@  test_expect_failure 'Clone UC repo with lc name' '
 	test_must_fail git p4 clone //depot/uc/...
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index 3c22f74bd4..fa1bba1dd9 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -31,8 +31,4 @@  test_expect_success 'EDITOR with options' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
index 81e46acfa8..ef80f1690b 100755
--- a/t/t9821-git-p4-path-variations.sh
+++ b/t/t9821-git-p4-path-variations.sh
@@ -193,8 +193,4 @@  test_expect_success 'Add a new file and clone path with new file (ignorecase)' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index c78477c19b..1bf7635016 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -67,8 +67,4 @@  test_expect_success 'Delete iso8859-1 encoded paths and clone' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh
index 1f2dc369bf..88b76dc4d6 100755
--- a/t/t9823-git-p4-mock-lfs.sh
+++ b/t/t9823-git-p4-mock-lfs.sh
@@ -185,8 +185,4 @@  test_expect_success 'Run git p4 submit in repo configured with large file system
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
index ed80ca858c..a28dbbdd56 100755
--- a/t/t9824-git-p4-git-lfs.sh
+++ b/t/t9824-git-p4-git-lfs.sh
@@ -287,8 +287,4 @@  test_expect_success 'Add big files to repo and store files in LFS based on compr
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 1551845dc1..f049ff8229 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -43,8 +43,4 @@  test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '
 	git p4 clone --dest="$git" //depot
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9826-git-p4-keep-empty-commits.sh b/t/t9826-git-p4-keep-empty-commits.sh
index fa8b9daf1f..fd64afe064 100755
--- a/t/t9826-git-p4-keep-empty-commits.sh
+++ b/t/t9826-git-p4-keep-empty-commits.sh
@@ -127,8 +127,4 @@  test_expect_success 'Clone repo subdir with all history' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9827-git-p4-change-filetype.sh b/t/t9827-git-p4-change-filetype.sh
index 7433998f47..d3670bd7a2 100755
--- a/t/t9827-git-p4-change-filetype.sh
+++ b/t/t9827-git-p4-change-filetype.sh
@@ -59,8 +59,4 @@  test_expect_success SYMLINKS 'change symbolic link to file' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
index e20395c89f..ca6c2942bd 100755
--- a/t/t9828-git-p4-map-user.sh
+++ b/t/t9828-git-p4-map-user.sh
@@ -54,8 +54,4 @@  test_expect_success 'Clone repo root path with all history' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
index 971aeeea1f..88cfb1fcd3 100755
--- a/t/t9829-git-p4-jobs.sh
+++ b/t/t9829-git-p4-jobs.sh
@@ -92,8 +92,4 @@  test_expect_success 'check log message of changelist with more jobs' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
index 2ad1b0810d..3fb6960c18 100755
--- a/t/t9830-git-p4-symlink-dir.sh
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -36,8 +36,4 @@  test_expect_success 'symlinked directory' '
 
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index be44c9751a..d743ca33ee 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -96,8 +96,4 @@  test_expect_success 'submit description with extra info lines from verbose p4 ch
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 48ec7679b8..b649d1b7c3 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -131,8 +131,4 @@  test_expect_success 'try to unshelve the change' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
index 277d347012..1f3d879122 100755
--- a/t/t9833-errors.sh
+++ b/t/t9833-errors.sh
@@ -72,9 +72,4 @@  test_expect_success 'git operation with expired ticket' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
-
 test_done