Re: [PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function
On Wed, Dec 05, 2018 at 01:20:35PM +0100, SZEDER Gábor wrote: > On Wed, Dec 05, 2018 at 12:17:09AM -0500, Jeff King wrote: > > On Tue, Dec 04, 2018 at 05:34:56PM +0100, SZEDER Gábor wrote: > > > > > Several test scripts run daemons like 'git-daemon' or Apache, and > > > communicate with them through TCP sockets. To have unique ports where > > > these daemons are accessible, the ports are usually the number of the > > > corresponding test scripts, unless the user overrides them via > > > environment variables, and thus all those tests and test libs contain > > > more or less the same bit of one-liner boilerplate code to find out > > > the port. The last patch in this series will make this a bit more > > > complicated. > > > > > > Factor out finding the port for a daemon into the common helper > > > function 'test_set_port' to avoid repeating ourselves. > > > > OK. Looks like this should keep the same port numbers for all of the > > existing tests, which I think is good. > > Not for all existing tests, though: t0410 and the 'git p4' tests do > get different ports. Yeah, I should have said "most". The important thing is that they remain static and predictable for normal non-stress runs, which they do. -Peff
Re: [PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function
On Wed, Dec 05, 2018 at 12:17:09AM -0500, Jeff King wrote: > On Tue, Dec 04, 2018 at 05:34:56PM +0100, SZEDER Gábor wrote: > > > Several test scripts run daemons like 'git-daemon' or Apache, and > > communicate with them through TCP sockets. To have unique ports where > > these daemons are accessible, the ports are usually the number of the > > corresponding test scripts, unless the user overrides them via > > environment variables, and thus all those tests and test libs contain > > more or less the same bit of one-liner boilerplate code to find out > > the port. The last patch in this series will make this a bit more > > complicated. > > > > Factor out finding the port for a daemon into the common helper > > function 'test_set_port' to avoid repeating ourselves. > > OK. Looks like this should keep the same port numbers for all of the > existing tests, which I think is good. Not for all existing tests, though: t0410 and the 'git p4' tests do get different ports. Hrm, speaking of affecting 'git p4' tests... I should have put Luke on Cc, so doing it now.
Re: [PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function
On Tue, Dec 04, 2018 at 05:34:56PM +0100, SZEDER Gábor wrote:
> Several test scripts run daemons like 'git-daemon' or Apache, and
> communicate with them through TCP sockets. To have unique ports where
> these daemons are accessible, the ports are usually the number of the
> corresponding test scripts, unless the user overrides them via
> environment variables, and thus all those tests and test libs contain
> more or less the same bit of one-liner boilerplate code to find out
> the port. The last patch in this series will make this a bit more
> complicated.
>
> Factor out finding the port for a daemon into the common helper
> function 'test_set_port' to avoid repeating ourselves.
OK. Looks like this should keep the same port numbers for all of the
existing tests, which I think is good. As nice as choosing random high
port numbers might be for some cases, it can also be confusing when
there are random conflicts.
I've also run into non-random conflicts, but at least once you figure
them out they're predictable (the most confusing I've seen is that adb,
the android debugger, sometimes but not always leaves a daemon hanging
around on port 5561).
> Take special care of test scripts with "low" numbers:
>
> - Test numbers below 1024 would result in a port that's only usable
> as root, so set their port to '1 + test-nr' to make sure it
> doesn't interfere with other tests in the test suite. This makes
> the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
> remove it.
This part is what made me think a bit more about conflicting with
dynamically allocated ports. Arguably the http parts of t0410 ought to
be in a much higher-numbered script, but I don't know that we've held
over the years very well to the idea that scripts should only depend on
the bits from lower numbered scripts.
> ---
> t/lib-git-daemon.sh | 2 +-
> t/lib-git-p4.sh | 9 +
> t/lib-git-svn.sh | 2 +-
> t/lib-httpd.sh | 2 +-
> t/t0410-partial-clone.sh | 1 -
> t/t5512-ls-remote.sh | 2 +-
> t/test-lib-functions.sh | 36
> 7 files changed, 41 insertions(+), 13 deletions(-)
The patch itself looks good to me.
> + eval port=\"\${$var}\"
> + case "$port" in
The quotes in the eval'd variable assignment aren't necessary. Usually I
don't mind them in a simple:
FOO="$bar"
even though they're redundant (and there were a few instances in the
prior patch, I think). But here the escaping makes it harder to read,
compared to:
eval port=\$$var
It might be worth simplifying, but I don't feel strongly about it (we'd
run into problems if $var contained spaces with either variant, but I
don't think that's worth caring about).
-Peff
[PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function
Several test scripts run daemons like 'git-daemon' or Apache, and
communicate with them through TCP sockets. To have unique ports where
these daemons are accessible, the ports are usually the number of the
corresponding test scripts, unless the user overrides them via
environment variables, and thus all those tests and test libs contain
more or less the same bit of one-liner boilerplate code to find out
the port. The last patch in this series will make this a bit more
complicated.
Factor out finding the port for a daemon into the common helper
function 'test_set_port' to avoid repeating ourselves.
Take special care of test scripts with "low" numbers:
- Test numbers below 1024 would result in a port that's only usable
as root, so set their port to '1 + test-nr' to make sure it
doesn't interfere with other tests in the test suite. This makes
the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
remove it.
- The shell's arithmetic evaluation interprets numbers with leading
zeros as octal values, which means that test number below 1000 and
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
Note that the Perforce tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
is, but does a bit of additional arithmetic on top [1].
- The port is not overridable via an environment variable.
With this patch even Perforce tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
[1] Commit fc00233071 (git-p4 tests: refactor and cleanup, 2011-08-22)
introduced that "unusual" unique port computation without
explaining why it was necessary (as opposed to simply using the
test number as is). It seems to be just unnecessary complication,
and in any case that commit came way before the "test nr as unique
port" got "standardized" for other daemons in commits c44132fcf3
(tests: auto-set git-daemon port, 2014-02-10), 3bb486e439 (tests:
auto-set LIB_HTTPD_PORT from test name, 2014-02-10), and
bf9d7df950 (t/lib-git-svn.sh: improve svnserve tests with parallel
make test, 2017-12-01).
Signed-off-by: SZEDER Gábor
---
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 36
7 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..41eb1e3ae8 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -28,7 +28,7 @@ then
test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support
FIFOs"
fi
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
+test_set_port LIB_GIT_DAEMON_PORT
GIT_DAEMON_PID=
GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..b3be3ba011 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -53,14 +53,7 @@ time_in_seconds () {
(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
}
-# Try to pick a unique port: guess a large number, then hope
-# no more than one of each test is running.
-#
-# This does not handle the case where somebody else is running the
-# same tests and has chosen the same ports.
-testid=${this_test#t}
-git_p4_test_start=9800
-P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+test_set_port P4DPORT
P4PORT=localhost:$P4DPORT
P4CLIENT=client
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a8130f9119..f3b478c307 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -13,6 +13,7 @@ fi
GIT_DIR=$PWD/.git
GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
SVN_TREE=$GIT_SVN_DIR/svn-tree
+test_set_port SVNSERVE_PORT
svn >/dev/null 2>&1
if test $? -ne 1
@@ -119,7 +120,6 @@ require_svnserve () {
}
start_svnserve () {
- SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
--root "$rawsvnrepo" \
--listen-once \
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..e465116ef9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,7 +82,7 @@ case $(uname) in
esac
LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
+test_set_port LIB_HTTPD_PORT
TEST_PATH="$TEST_DIRECTORY"/lib-httpd
HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..0aca8d7588 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -480,7 +480,6 @@ test_expect_success 'gc stops traversal when a missing but
promised object is re
! grep "$TREE_HASH" out
'
-LIB_HTTPD_PORT=12345 # default port, 410, cannot be
