[PATCH v5 0/6] Add Travis CI support

[PATCH v5 0/6] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v4:
* add Junio's "test_must_fail ok=" refactor that he sent as response
  to my patch. Did I attribute this to Junio in the right way by adding
  "Signed-off-by: Junio C Hamano <gitster@pobox.com>" to the commit message?
  Please let me know how to handle these cases properly!
* fix commit message wording according to Junio's suggestion
* do not accept sigpipe failures in "git cat-file" (thanks Junio)
* reorder patches (thanks Eric)
* fix incorrect space indent (thanks Eric)
* add a trap to git-p4 tests that ensure p4d is always killed

You can see the CI results for this patch applied on master here:
https://travis-ci.org/larsxschneider/git/builds/91216501

Thanks,
Lars

Lars Schneider (6):
  implement test_might_fail using a refactored test_must_fail
  add "ok=sigpipe" to test_must_fail and use it to fix flaky tests
  git-p4: retry kill/cleanup operations in tests with timeout
  git-p4: add p4d timeout in tests
  git-p4: add trap to kill p4d on test exit
  Add Travis CI support

 .travis.yml                     | 131 ++++++++++++++++++++++++++++++++++++++++
 t/lib-git-p4.sh                 |  57 ++++++++++++++---
 t/t5504-fetch-receive-strict.sh |   3 +-
 t/t5516-fetch-push.sh           |   6 +-
 t/test-lib-functions.sh         |  39 +++++++-----
 5 files changed, 207 insertions(+), 29 deletions(-)
 create mode 100644 .travis.yml

--
2.5.1

[PATCH v5 1/6] implement test_might_fail using a refactored test_must_fail

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Add an (optional) first parameter "ok=<special case>" to test_must_fail
and return success for "<special case>". Add "success" as
"<special case>" and use it to implement "test_might_fail". This removes
redundancies in test-lib-function.sh.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/test-lib-functions.sh | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 73e37a1..1e762da 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -582,18 +582,32 @@ test_line_count () {
 # the failure could be due to a segv.  We want a controlled failure.
 
 test_must_fail () {
+	case "$1" in
+	ok=*)
+		_test_ok=${1#ok=}
+		shift
+		;;
+	*)
+		_test_ok=
+		;;
+	esac
 	"$@"
 	exit_code=$?
-	if test $exit_code = 0; then
+	if ! case ",$_test_ok," in *,success,*) false;; esac &&
+		test $exit_code = 0
+	then
 		echo >&2 "test_must_fail: command succeeded: $*"
-		return 1
-	elif test $exit_code -gt 129 && test $exit_code -le 192; then
+		return 0
+	elif test $exit_code -gt 129 && test $exit_code -le 192
+	then
 		echo >&2 "test_must_fail: died by signal: $*"
 		return 1
-	elif test $exit_code = 127; then
+	elif test $exit_code = 127
+	then
 		echo >&2 "test_must_fail: command not found: $*"
 		return 1
-	elif test $exit_code = 126; then
+	elif test $exit_code = 126
+	then
 		echo >&2 "test_must_fail: valgrind error: $*"
 		return 1
 	fi
@@ -612,16 +626,7 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
-	"$@"
-	exit_code=$?
-	if test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_might_fail: died by signal: $*"
-		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_might_fail: command not found: $*"
-		return 1
-	fi
-	return 0
+	test_must_fail ok=success "$@"
 }
 
 # Similar to test_must_fail and test_might_fail, but check that a
-- 
2.5.1

[PATCH v5 2/6] add "ok=sigpipe" to test_must_fail and use it to fix flaky tests

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" is
flaky in the following case:
1. remote upload-pack finds out "not our ref"
2. remote sends a response and closes the pipe
3. fetch-pack still tries to write commands to the remote upload-pack
4. write call in wrapper.c dies with SIGPIPE

t5504 "9 - push with transfer.fsckobjects" is flaky, too, and returns
SIGPIPE once in a while. I had to remove the final "To dst..." output
check because there is no output if the process dies with SIGPUPE.

Accept such a death-with-sigpipe also as OK when we are expecting a
failure.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t5504-fetch-receive-strict.sh | 3 +--
 t/t5516-fetch-push.sh           | 6 +++---
 t/test-lib-functions.sh         | 4 ++++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..a9e382c 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -111,8 +111,7 @@ test_expect_success 'push with transfer.fsckobjects' '
 		cd dst &&
 		git config transfer.fsckobjects true
 	) &&
-	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
-	test_cmp exp act
+	test_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act
 '
 
 cat >bogus-commit <<\EOF
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ec22c98..0a87e19 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1162,15 +1162,15 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
 			git cat-file commit $SHA1_1 &&
 			test_must_fail git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
 			git cat-file commit $SHA1_2 &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_3
+			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
 		)
 	'
 done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1e762da..1fdc58c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -598,6 +598,10 @@ test_must_fail () {
 	then
 		echo >&2 "test_must_fail: command succeeded: $*"
 		return 0
+	elif ! case ",$_test_ok," in *,sigpipe,*) false;; esac &&
+		test $exit_code = 141
+	then
+		return 0
 	elif test $exit_code -gt 129 && test $exit_code -le 192
 	then
 		echo >&2 "test_must_fail: died by signal: $*"
-- 
2.5.1

[PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

In rare cases kill/cleanup operations in tests fail. Retry these
operations with a timeout to make the test less flaky.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 7548225..8d6b48f 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -6,6 +6,10 @@
 # a subdirectory called "$git"
 TEST_NO_CREATE_REPO=NoThanks
 
+# Some operations require multiple attempts to be successful. Define
+# here the maximal retry timeout in seconds.
+RETRY_TIMEOUT=60
+
 . ./test-lib.sh
 
 if ! test_have_prereq PYTHON
@@ -121,22 +125,33 @@ p4_add_user() {
 	EOF
 }
 
+retry_until_success() {
+    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
+    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
+    do :
+    done
+}
+
+retry_until_fail() {
+    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
+    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
+    do :
+    done
+}
+
 kill_p4d() {
 	pid=$(cat "$pidfile")
-	# it had better exist for the first kill
-	kill $pid &&
-	for i in 1 2 3 4 5 ; do
-		kill $pid >/dev/null 2>&1 || break
-		sleep 1
-	done &&
+	retry_until_fail kill $pid
+	retry_until_fail kill -9 $pid
 	# complain if it would not die
 	test_must_fail kill $pid >/dev/null 2>&1 &&
 	rm -rf "$db" "$cli" "$pidfile"
 }
 
 cleanup_git() {
-	rm -rf "$git" &&
-	mkdir "$git"
+	retry_until_success rm -r "$git"
+	test_must_fail test -d "$git" &&
+	retry_until_success mkdir "$git"
 }
 
 marshal_dump() {
-- 
2.5.1

[PATCH v5 4/6] git-p4: add p4d timeout in tests

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

In rare cases p4d seems to hang. This watchdog will kill the p4d
process after 300s in any case. That means each individual git p4 test
needs to finish before 300s or it will fail.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/lib-git-p4.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 8d6b48f..f2a009c 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -10,6 +10,10 @@ TEST_NO_CREATE_REPO=NoThanks
 # here the maximal retry timeout in seconds.
 RETRY_TIMEOUT=60
 
+# Sometimes p4d seems to hang. Terminate the p4d process automatically after
+# the defined timeout in seconds.
+P4D_TIMEOUT=300
+
 . ./test-lib.sh
 
 if ! test_have_prereq PYTHON
@@ -85,6 +89,19 @@ start_p4d() {
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
 	pid=$(cat "$pidfile")
+
+	timeout=$(($(date +%s) + $P4D_TIMEOUT))
+	while true
+	do
+		if test $(date +%s) -gt $timeout
+		then
+			kill -9 $pid
+			exit 1
+		fi
+		sleep 1
+	done &
+	watchdog_pid=$!
+
 	ready=
 	while test $i -gt 0
 	do
@@ -145,7 +162,8 @@ kill_p4d() {
 	retry_until_fail kill -9 $pid
 	# complain if it would not die
 	test_must_fail kill $pid >/dev/null 2>&1 &&
-	rm -rf "$db" "$cli" "$pidfile"
+	rm -rf "$db" "$cli" "$pidfile" &&
+	retry_until_fail kill -9 $watchdog_pid
 }
 
 cleanup_git() {
-- 
2.5.1

[PATCH v5 5/6] git-p4: add trap to kill p4d on test exit

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Sometimes the "prove" test runner hangs on test exit because p4d is
still running. Add a trap to always kill "p4d" on test exit.

You can reproduce the problem by commenting "P4D_TIMEOUT" in
"lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
---
 t/lib-git-p4.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index f2a009c..f9c68d4 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -65,6 +65,12 @@ 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() {
+	kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
+}
+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.
-- 
2.5.1

[PATCH v5 6/6] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
64 bit" and on "OS X Mavericks" using gcc and clang.

Perforce and Git-LFS are installed and therefore available for the
respective tests.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 .travis.yml | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 .travis.yml

diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 0000000..61c70fa
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,131 @@
+language: c
+
+os:
+  - linux
+  - osx
+
+compiler:
+  - clang
+  - gcc
+
+addons:
+  apt:
+    packages:
+    - language-pack-is
+
+env:
+  global:
+    - P4_VERSION="15.1"
+    - GIT_LFS_VERSION="1.0.2"
+    - DEFAULT_TEST_TARGET=prove
+    - GIT_PROVE_OPTS="--timer --jobs 3"
+    - GIT_TEST_OPTS="--verbose --tee"
+    - GETTEXT_ISO_LOCALE=YesPlease
+    - GETTEXT_LOCALE=YesPlease
+    # - GETTEXT_POISON=YesPlease
+    - GIT_TEST_CHAIN_LINT=YesPlease
+    - GIT_TEST_CLONE_2GB=YesPlease
+    # - GIT_TEST_LONG=YesPlease
+  matrix:
+    -
+      # NO_ICONV=YesPlease
+    - >
+      NO_CURL=YesPlease
+      NO_D_INO_IN_DIRENT=YesPlease
+      NO_DEFLATE_BOUND=YesPlease
+      NO_EXPAT=YesPlease
+      NO_GECOS_IN_PWENT=YesPlease
+      NO_GETTEXT=YesPlease
+      NO_HMAC_CTX_CLEANUP=YesPlease
+      NO_HSTRERROR=YesPlease
+      NO_INET_NTOP=YesPlease
+      NO_INET_PTON=YesPlease
+      NO_INITGROUPS=YesPlease
+      NO_INTTYPES_H=YesPlease
+      NO_IPV6=YesPlease
+      NO_IPV6=YesPlease
+      NO_LIBGEN_H=YesPlease
+      NO_MEMMEM=YesPlease
+      NO_MKDTEMP=YesPlease
+      NO_MKSTEMPS=YesPlease
+      NO_MMAP=YesPlease
+      NO_NSEC=YesPlease
+      NO_OPENSSL=YesPlease
+      NO_PERL=YesPlease
+      NO_PTHREADS=YesPlease
+      NO_REGEX=YesPlease
+      NO_SETENV=YesPlease
+      NO_SETITIMER=YesPlease
+      NO_SOCKADDR_STORAGE=YesPlease
+      NO_STRCASESTR=YesPlease
+      NO_STRLCPY=YesPlease
+      NO_STRTOUMAX=YesPlease
+      NO_STRUCT_ITIMERVAL=YesPlease
+      NO_SYMLINK_HEAD=YesPlease
+      NO_SYS_POLL_H=YesPlease
+      NO_SYS_SELECT_H=YesPlease
+      NO_UINTMAX_T=YesPlease
+      NO_UNSETENV=YesPlease
+
+before_install:
+  - >
+    case "${TRAVIS_OS_NAME:-linux}" in
+    linux)
+      mkdir --parents custom/p4
+      pushd custom/p4
+        wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4d
+        wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4
+        chmod u+x p4d
+        chmod u+x p4
+        export PATH="$(pwd):$PATH"
+      popd
+      mkdir --parents custom/git-lfs
+      pushd custom/git-lfs
+        wget --quiet https://github.com/github/git-lfs/releases/download/v$GIT_LFS_VERSION/git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz
+        tar --extract --gunzip --file "git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz"
+        cp git-lfs-$GIT_LFS_VERSION/git-lfs .
+        export PATH="$(pwd):$PATH"
+      popd
+      ;;
+    osx)
+      brew_force_set_latest_binary_hash () {
+        FORMULA=$1
+        SHA=$(brew fetch --force $FORMULA 2>&1 | grep ^SHA256: | cut -d ' ' -f 2)
+        sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$SHA\"/g" \
+          /usr/local/Library/Taps/homebrew/homebrew-binary/$FORMULA.rb
+      }
+      brew update --quiet
+      brew tap homebrew/binary --quiet
+      brew_force_set_latest_binary_hash perforce
+      brew_force_set_latest_binary_hash perforce-server
+      brew install git-lfs perforce-server perforce
+      ;;
+    esac;
+    echo "$(tput setaf 6)Perfoce Server Version$(tput sgr0)";
+    p4d -V | grep Rev.;
+    echo "$(tput setaf 6)Perfoce Client Version$(tput sgr0)";
+    p4 -V | grep Rev.;
+    echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
+    git-lfs version;
+
+before_script: make configure && ./configure && make --jobs=2
+
+script: make --quiet test
+
+after_failure:
+  - >
+    : '<-- Click here to see detailed test output!                                                        ';
+    for TEST_EXIT in t/test-results/*.exit;
+    do
+      if [ "$(cat "$TEST_EXIT")" != "0" ];
+      then
+        TEST_OUT="${TEST_EXIT%exit}out";
+        echo "------------------------------------------------------------------------";
+        echo "$(tput setaf 1)${TEST_OUT}...$(tput sgr0)";
+        echo "------------------------------------------------------------------------";
+        cat "${TEST_OUT}";
+      fi;
+    done;
+
+notifications:
+  email: false
-- 
2.5.1

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Luke Diamand]

On 15/11/15 13:08, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> In rare cases kill/cleanup operations in tests fail. Retry these
> operations with a timeout to make the test less flaky.

Should there be a sleep in that retry_until_success loop so that it 
doesn't spin sending signals to p4d?

Do we need to worry about the time offset being updated (e.g. NTP) while 
this is running?

>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 7548225..8d6b48f 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -6,6 +6,10 @@
>   # a subdirectory called "$git"
>   TEST_NO_CREATE_REPO=NoThanks
>
> +# Some operations require multiple attempts to be successful. Define
> +# here the maximal retry timeout in seconds.
> +RETRY_TIMEOUT=60
> +
>   . ./test-lib.sh
>
>   if ! test_have_prereq PYTHON
> @@ -121,22 +125,33 @@ p4_add_user() {
>   	EOF
>   }
>
> +retry_until_success() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :

  sleep here?

> +    done
> +}
> +
> +retry_until_fail() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :

  sleep here?

> +    done
> +}
> +
>   kill_p4d() {
>   	pid=$(cat "$pidfile")
> -	# it had better exist for the first kill
> -	kill $pid &&
> -	for i in 1 2 3 4 5 ; do
> -		kill $pid >/dev/null 2>&1 || break
> -		sleep 1
> -	done &&
> +	retry_until_fail kill $pid
> +	retry_until_fail kill -9 $pid
>   	# complain if it would not die
>   	test_must_fail kill $pid >/dev/null 2>&1 &&
>   	rm -rf "$db" "$cli" "$pidfile"
>   }
>
>   cleanup_git() {
> -	rm -rf "$git" &&
> -	mkdir "$git"
> +	retry_until_success rm -r "$git"
> +	test_must_fail test -d "$git" &&
> +	retry_until_success mkdir "$git"
>   }
>
>   marshal_dump() {
>

Re: [PATCH v5 4/6] git-p4: add p4d timeout in tests

[Luke Diamand]

On 15/11/15 13:08, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> In rare cases p4d seems to hang. This watchdog will kill the p4d
> process after 300s in any case. That means each individual git p4 test
> needs to finish before 300s or it will fail.

Looks good to me.


>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   t/lib-git-p4.sh | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 8d6b48f..f2a009c 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -10,6 +10,10 @@ TEST_NO_CREATE_REPO=NoThanks
>   # here the maximal retry timeout in seconds.
>   RETRY_TIMEOUT=60
>
> +# Sometimes p4d seems to hang. Terminate the p4d process automatically after
> +# the defined timeout in seconds.
> +P4D_TIMEOUT=300
> +
>   . ./test-lib.sh
>
>   if ! test_have_prereq PYTHON
> @@ -85,6 +89,19 @@ start_p4d() {
>   	# will be caught with the "kill -0" check below.
>   	i=${P4D_START_PATIENCE:-300}
>   	pid=$(cat "$pidfile")
> +
> +	timeout=$(($(date +%s) + $P4D_TIMEOUT))
> +	while true
> +	do
> +		if test $(date +%s) -gt $timeout
> +		then
> +			kill -9 $pid
> +			exit 1
> +		fi
> +		sleep 1
> +	done &
> +	watchdog_pid=$!
> +
>   	ready=
>   	while test $i -gt 0
>   	do
> @@ -145,7 +162,8 @@ kill_p4d() {
>   	retry_until_fail kill -9 $pid
>   	# complain if it would not die
>   	test_must_fail kill $pid >/dev/null 2>&1 &&
> -	rm -rf "$db" "$cli" "$pidfile"
> +	rm -rf "$db" "$cli" "$pidfile" &&
> +	retry_until_fail kill -9 $watchdog_pid
>   }
>
>   cleanup_git() {
>

Re: [PATCH v5 5/6] git-p4: add trap to kill p4d on test exit

[Luke Diamand]

On 15/11/15 13:08, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Sometimes the "prove" test runner hangs on test exit because p4d is
> still running. Add a trap to always kill "p4d" on test exit.

With this change, I've started seeing this when running the tests:

cat: /home/lgd/git/git/t/trash 
directory.t9819-git-p4-case-folding/p4d.pid: No such file or directory

Probably just needs the obvious "test -f" adding.

Other than, all looks good to me. Particularly nice that I can now do:

$ make T=t98* -j10

and it actually works!





>
> You can reproduce the problem by commenting "P4D_TIMEOUT" in
> "lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
> ---
>   t/lib-git-p4.sh | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index f2a009c..f9c68d4 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -65,6 +65,12 @@ 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() {
> +	kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
> +}
> +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.
>

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Eric Sunshine]

On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschneider@gmail.com> wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> In rare cases kill/cleanup operations in tests fail. Retry these
> operations with a timeout to make the test less flaky.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> @@ -121,22 +125,33 @@ p4_add_user() {
>         EOF
>  }
>
> +retry_until_success() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))

There was some discussion previously[1] about detecting dynamically
whether 'date +%s' was supported. Was this something that you intended
to do, or did you decide against it since p4 is not supported on such
platforms?

Same question also applies to patch 4/6.

[1]: http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite

> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}
> +
> +retry_until_fail() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

On 16 Nov 2015, at 09:36, Luke Diamand <luke@diamand.org> wrote:

> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
> 
> Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d?
Agreed. I will add a sleep in the next roll!


> 
> Do we need to worry about the time offset being updated (e.g. NTP) while this is running?
Interesting question! I would consider this an edge case but I can see how it could happen.
Do you see a way to handle that in an easy way?

Thanks,
Lars

> 
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>>  t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index 7548225..8d6b48f 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -6,6 +6,10 @@
>>  # a subdirectory called "$git"
>>  TEST_NO_CREATE_REPO=NoThanks
>> 
>> +# Some operations require multiple attempts to be successful. Define
>> +# here the maximal retry timeout in seconds.
>> +RETRY_TIMEOUT=60
>> +
>>  . ./test-lib.sh
>> 
>>  if ! test_have_prereq PYTHON
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>  	EOF
>>  }
>> 
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
> 
> sleep here?
> 
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
> 
> sleep here?
> 
>> +    done
>> +}
>> +
>>  kill_p4d() {
>>  	pid=$(cat "$pidfile")
>> -	# it had better exist for the first kill
>> -	kill $pid &&
>> -	for i in 1 2 3 4 5 ; do
>> -		kill $pid >/dev/null 2>&1 || break
>> -		sleep 1
>> -	done &&
>> +	retry_until_fail kill $pid
>> +	retry_until_fail kill -9 $pid
>>  	# complain if it would not die
>>  	test_must_fail kill $pid >/dev/null 2>&1 &&
>>  	rm -rf "$db" "$cli" "$pidfile"
>>  }
>> 
>>  cleanup_git() {
>> -	rm -rf "$git" &&
>> -	mkdir "$git"
>> +	retry_until_success rm -r "$git"
>> +	test_must_fail test -d "$git" &&
>> +	retry_until_success mkdir "$git"
>>  }
>> 
>>  marshal_dump() {

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

On 16 Nov 2015, at 22:14, Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>        EOF
>> }
>> 
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> 
> There was some discussion previously[1] about detecting dynamically
> whether 'date +%s' was supported. Was this something that you intended
> to do, or did you decide against it since p4 is not supported on such
> platforms?
> 
> Same question also applies to patch 4/6.

While implementing it I thought more about it. P4D is only supported on platforms that support the date function. That means these tests will only run on platforms that support the date function. Consequently I wondered if this would justify the slightly more complicated code. However, if you think this change would help the patch to get accepted then I will add it.

Thanks,
Lars


> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite
> 
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}

Re: [PATCH v5 5/6] git-p4: add trap to kill p4d on test exit

[Lars Schneider]

On 16 Nov 2015, at 09:43, Luke Diamand <luke@diamand.org> wrote:

> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Sometimes the "prove" test runner hangs on test exit because p4d is
>> still running. Add a trap to always kill "p4d" on test exit.
> 
> With this change, I've started seeing this when running the tests:
> 
> cat: /home/lgd/git/git/t/trash directory.t9819-git-p4-case-folding/p4d.pid: No such file or directory
> 
> Probably just needs the obvious "test -f" adding.
> 
> Other than, all looks good to me. Particularly nice that I can now do:
> 
> $ make T=t98* -j10
> 
> and it actually works!

Great! I can see where the cat error comes from. I will add the "test -f" condition in the next roll.

Thanks,
Lars

> 
> 
> 
> 
> 
>> 
>> You can reproduce the problem by commenting "P4D_TIMEOUT" in
>> "lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
>> ---
>>  t/lib-git-p4.sh | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index f2a009c..f9c68d4 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -65,6 +65,12 @@ 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() {
>> +	kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
>> +}
>> +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.
>> 
> 

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Eric Sunshine]

On Tue, Nov 17, 2015 at 3:28 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> On 16 Nov 2015, at 22:14, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschneider@gmail.com> wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> In rare cases kill/cleanup operations in tests fail. Retry these
>>> operations with a timeout to make the test less flaky.
>>>
>>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>>> ---
>>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>>> @@ -121,22 +125,33 @@ p4_add_user() {
>>>        EOF
>>> }
>>>
>>> +retry_until_success() {
>>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>>
>> There was some discussion previously[1] about detecting dynamically
>> whether 'date +%s' was supported. Was this something that you intended
>> to do, or did you decide against it since p4 is not supported on such
>> platforms?
>>
>> Same question also applies to patch 4/6.
>
> While implementing it I thought more about it. P4D is only
> supported on platforms that support the date function. That means
> these tests will only run on platforms that support the date
> function. Consequently I wondered if this would justify the
> slightly more complicated code. However, if you think this change
> would help the patch to get accepted then I will add it.

I don't feel strongly about it, and it's not my call anyhow. Opinions
of Junio, Peff (as interim maintainer), and Luke weigh much more
heavily than my own. Punting on dynamic detection of "date +%s" may be
perfectly acceptable with the attitude that it can be implemented
later if someone runs across a case where it's actually needed.

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Luke Diamand]

>>
>> While implementing it I thought more about it. P4D is only
>> supported on platforms that support the date function. That means
>> these tests will only run on platforms that support the date
>> function. Consequently I wondered if this would justify the
>> slightly more complicated code. However, if you think this change
>> would help the patch to get accepted then I will add it.
>
> I don't feel strongly about it, and it's not my call anyhow. Opinions
> of Junio, Peff (as interim maintainer), and Luke weigh much more
> heavily than my own. Punting on dynamic detection of "date +%s" may be
> perfectly acceptable with the attitude that it can be implemented
> later if someone runs across a case where it's actually needed.

Which other platforms are we talking about here?

https://www.perforce.com/downloads/helix

 From there, you can get Solaris10, HP-UX, AIX and various flavours of 
BSD. Solaris supports "date +%s".

HP-UX and AIX, I really don't know.

Windows? I assume 'date +%s' will work for people using mingw.

Is it possible to get the time in seconds by doing something like this:

time_in_seconds() {
	python -c 'import time; print time.time()'
}

Luke

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Luke Diamand]

On 17/11/15 08:22, Lars Schneider wrote:
>
> On 16 Nov 2015, at 09:36, Luke Diamand <luke@diamand.org> wrote:
>
>> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> In rare cases kill/cleanup operations in tests fail. Retry these
>>> operations with a timeout to make the test less flaky.
>>
>> Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d?
> Agreed. I will add a sleep in the next roll!
>
>
>>
>> Do we need to worry about the time offset being updated (e.g. NTP) while this is running?
> Interesting question! I would consider this an edge case but I can see how it could happen.
> Do you see a way to handle that in an easy way?
>

You want to somehow call clock_gettime(CLOCK_MONOTONIC). That's not in 
python until 3.3. Writing a C program seems like overkill but could be a 
solution if this becomes a problem.

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Eric Sunshine]

On Tue, Nov 17, 2015 at 4:34 AM, Luke Diamand <luke@diamand.org> wrote:
>>> While implementing it I thought more about it. P4D is only
>>> supported on platforms that support the date function. That means
>>> these tests will only run on platforms that support the date
>>> function. Consequently I wondered if this would justify the
>>> slightly more complicated code. However, if you think this change
>>> would help the patch to get accepted then I will add it.
>>
>> I don't feel strongly about it, and it's not my call anyhow. Opinions
>> of Junio, Peff (as interim maintainer), and Luke weigh much more
>> heavily than my own. Punting on dynamic detection of "date +%s" may be
>> perfectly acceptable with the attitude that it can be implemented
>> later if someone runs across a case where it's actually needed.
>
> Which other platforms are we talking about here?
>
> https://www.perforce.com/downloads/helix
>
> From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD.
> Solaris supports "date +%s".

The question about "date +%s" portability arose with a suggestion to
employ it in git-filter-branch[1]. Apparently, the concern about
Solaris was raised in response to a Stack Overflow question[2] which
claimed that +%s was not supported by that OS. Unfortunately, however,
[2] did not indicate to which (older?) versions of Solaris that
shortcoming applied. If Solaris 10 does support +%s, and the Perforce
product is only available for Solaris 10, then perhaps concern about
+%s a non-issue(?).

[1]: http://git.661346.n2.nabble.com/FEATURE-REQUEST-Filter-branch-extend-progress-with-a-simple-estimated-time-remaning-td7638200.html#a7638504
[2]: http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Luke Diamand]

>>
>> Which other platforms are we talking about here?
>>
>> https://www.perforce.com/downloads/helix
>>
>> From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD.
>> Solaris supports "date +%s".
>
> The question about "date +%s" portability arose with a suggestion to
> employ it in git-filter-branch[1]. Apparently, the concern about
> Solaris was raised in response to a Stack Overflow question[2] which
> claimed that +%s was not supported by that OS. Unfortunately, however,
> [2] did not indicate to which (older?) versions of Solaris that
> shortcoming applied. If Solaris 10 does support +%s, and the Perforce
> product is only available for Solaris 10, then perhaps concern about
> +%s a non-issue(?).
>
> [1]: http://git.661346.n2.nabble.com/FEATURE-REQUEST-Filter-branch-extend-progress-with-a-simple-estimated-time-remaning-td7638200.html#a7638504
> [2]: http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell

Rereading the man page more carefully, you're right, Solaris 10
doesn't do "%s". Using python to get the seconds should work though.