build revolution lost t_client test?

Hi,

working on the test server, I noticed that t_client.sh was no longer run from "make check", and that it lost "read t_client.rc from build dir" (which is needed when building different clients from a common source dir, and wanting to run t_client with different option sets).

The attached patch restores lost functionality, and adds one extra feature, optionally waiting longer for OpenVPN startup (if the server has a high RTT, 10 seconds is not always enough).

David, this has been tested on FreeBSD 7.4 (so far) and is essential for the buildbot client tests :-) - more platform tests will follow as soon as I have all the server stanzas up and running.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

From 125b3c96dc60694eb50dc4a85e758727864e83ee Mon Sep 17 00:00:00 2001
From: Gert Doering <[email protected]...>
Date: Sun, 27 May 2012 23:19:11 +0300
Subject: [PATCH] repair t_client.sh test after build system revolution

- run t_client.sh at "make check" time
- in t_client.sh, read t_client.rc from source *or* build dir (as before)
- @IP@ evaluates to "" now (not "ip") if iproute2 not found - adapt script
- introduce $SETUP_TIME_WAIT to delay "waiting for openvpn startup" longer than the default delay of 10 seconds - this is needed for test servers with a high network RTT

---
 tests/Makefile.am    |    5 ++---
 tests/t_client.sh.in |   13 ++++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

index 6ae845b..b7980e0 100644
@@ -12,13 +12,12 @@
 MAINTAINERCLEANFILES = \
 	$(srcdir)/Makefile.in
 
-test_scripts = t_lpback.sh t_cltsrv.sh
+test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
 
 TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
 TESTS = $(test_scripts)
 
 dist_noinst_SCRIPTS = \
 	$(test_scripts) \
-	t_cltsrv-down.sh \
-	t_client.sh
+	t_cltsrv-down.sh
 

index 7ba124c..92e4d2e 100755
@@ -14,16 +14,19 @@
 
 srcdir="${srcdir:-.}"
 top_builddir="${top_builddir:-..}"
-if [ -r "${srcdir}"/t_client.rc ] ; then
+if [ -r "${top_builddir}"/t_client.rc ] ; then
+    . "${top_builddir}"/t_client.rc
+elif [ -r "${srcdir}"/t_client.rc ] ; then
     . "${srcdir}"/t_client.rc
 else
-    echo "$0: cannot find 't_client.rc' in ('${srcdir}'). SKIPPING TEST." >&2
+    echo "$0: cannot find 't_client.rc' in build dir ('${top_builddir}')" >&2
+    echo "$0: or source directory ('${srcdir}'). SKIPPING TEST." >&2
     exit 77
 fi
 
 if [ ! -x "${top_builddir}/src/openvpn/openvpn" ]
 then
-    echo "no (executable) openvpn binary in current directory. FAIL." >&2
+    echo "no (executable) openvpn binary in current build tree. FAIL." >&2
     exit 1
 fi
 

@@ -83,7 +86,7 @@ fail()
 get_ifconfig_route()
 {
     # linux / iproute2? (-> if configure got a path)
-    if [ "@IPROUTE@" != "ip" ]
+    if [ "@IPROUTE@" != "" ]
     then
 	echo "-- linux iproute2 --"
 	@IPROUTE@ addr show     | grep -v valid_lft

@@ -236,7 +239,7 @@ do
     trap "$RUN_SUDO kill $opid ; trap - 0 ; exit 1" 1 2 3 15
 
     echo "wait for connection to establish..."
-    sleep 10
+    sleep ${SETUP_TIME_WAIT:-10}
 
     # test whether OpenVPN process is still there
     if $RUN_SUDO kill -0 $opid

1.7.3.5

t_client.sh requires root/sudo privileges.
"make check" should not assume any special privilege. Because of this it is removed from "make check".
You can still run this by hand or we can add special target such as "make root-check" or similar.

Hi

There was agreement by the developers to *have* this, and no explicit agreement to *remove* it - you got an ACK for the patch, but I'm fairly sure that it was not made clear that you removed t_client.sh functionality while moving stuff around, because otherwise you would have seen NAKs.

If someone wants to run it as non-root, RUN_SUDO can be defined in t_client.rc, and for someone having no provileges at all, you can just leave off t_client.rc, and it will skip these tests with no adverse effects.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Il 28.05.2012 10:52, Gert Doering ha scritto:

It seems 34cb91 removed this functionality without explicitly mention anything about it. It was a massive patch that moved stuff around, which is why this change went unnoticed. Somebody please apply Gert's patch or provide an alternative patch, I'd need to setup connectivity tests for my buildslaves, too.

I don't really care what the make target is called. It could even be an entirely different target. Buildslaves could easily run "make check" followed by "make root-check", "make connection-test" or whatever. If nobody outside the core project is supposed to run the connectivity tests, I think separating them from "make check" would make sense.

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Hi,

Nobody who hasn't set up a t_client.rc beforehand will be bothered by the client connectivity tests anyway - "make check" will tell him "rc file hasn't been set up, skipping test", and that's all of it.

So while I can see some purity of argument here - in the end, it's wasting human life time on discussions again, and the time could be better spent on what's sorely missing right now: automated tests (unit tests, client system tests, server system tests).

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Adding integration tests that needs and/or manual intervention to make check is not something that should be encouraged.

Tiered of arguing.

But if you add this, at least use the following.

---
 tests/Makefile.am    |   11 ++++++-----
 tests/t_client.sh.in |   13 ++++++++-----
 2 files changed, 14 insertions(+), 10 deletions(-)

index 6ae845b..f6d9d1b 100644
@@ -12,13 +12,14 @@
 MAINTAINERCLEANFILES = \
 	$(srcdir)/Makefile.in
 
-test_scripts = t_lpback.sh t_cltsrv.sh
+test_scripts = t_lpback.sh t_cltsrv.sh t_client.sh
 
-TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
+TESTS_ENVIRONMENT = \
+	top_srcdir="$(top_srcdir)" \
+	top_builddir="$(top_builddir)" \
+	builddir="$(builddir)"
 TESTS = $(test_scripts)
 
 dist_noinst_SCRIPTS = \
 	$(test_scripts) \
-	t_cltsrv-down.sh \
-	t_client.sh
-
+	t_cltsrv-down.sh

index 7ba124c..a9337a0 100755
@@ -13,17 +13,20 @@
 #
 
 srcdir="${srcdir:-.}"
+builddir="${builddir:-${srcdir}}"
 top_builddir="${top_builddir:-..}"
-if [ -r "${srcdir}"/t_client.rc ] ; then
+if [ -r "${builddir}"/t_client.rc ] ; then
+    . "${builddir}"/t_client.rc
+elif [ -r "${srcdir}"/t_client.rc ] ; then
     . "${srcdir}"/t_client.rc
 else
-    echo "$0: cannot find 't_client.rc' in ('${srcdir}'). SKIPPING TEST." >&2
+    echo "$0: cannot find 't_client.rc' in ('${srcdir}', '${builddir}'). SKIPPING TEST." >&2
     exit 77
 fi
 
 if [ ! -x "${top_builddir}/src/openvpn/openvpn" ]
 then
-    echo "no (executable) openvpn binary in current directory. FAIL." >&2
+    echo "no (executable) openvpn binary in '${top_builddir}' directory. FAIL." >&2
     exit 1
 fi
 

@@ -83,7 +86,7 @@ fail()
 get_ifconfig_route()
 {
     # linux / iproute2? (-> if configure got a path)
-    if [ "@IPROUTE@" != "ip" ]
+    if [ -n "@IPROUTE@" ]
     then
 	echo "-- linux iproute2 --"
 	@IPROUTE@ addr show     | grep -v valid_lft

@@ -236,7 +239,7 @@ do
     trap "$RUN_SUDO kill $opid ; trap - 0 ; exit 1" 1 2 3 15
 
     echo "wait for connection to establish..."
-    sleep 10
+    sleep ${SETUP_TIME_WAIT:-10}
 
     # test whether OpenVPN process is still there
     if $RUN_SUDO kill -0 $opid

1.7.3.4

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Applied to master branch and pushed out to -testing and -stable trees.

commit 47c990009c79fe2a96206c54e0240cb2dd8cdf02

Author: Gert Doering <[email protected]...>
Date:   Sun May 27 23:19:11 2012 +0300

    repair t_client.sh test after build system revolution

    Signed-off-by: Gert Doering <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Message-Id: [email protected]...
    URL: http://article.gmane.org/gmane.network.openvpn.devel/6616
    Signed-off-by: David Sommerseth <[email protected]...>

This patch was preferred over the patch from Alon, due to:

- - Both solves the same issue, with minor differences
- - This patch has an informative commit message
- - The alternative doesn't provide any arguments why that approach is better; in fact, the commit message is completely lacking.
- - The alternative reduces the information in an error message which can be helpful for the user running this test.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk/Im7AACgkQDC186MBRfrp/ogCbB9GIgwl6c+aU3mjC9xZNn5n6 UPMAoKSxjgbYJOSGHBNHsIKdAwVnOiar
=ZOom
-----END PGP SIGNATURE-----