[PATCH v6 0/6] Add Travis CI support

[PATCH v6 0/6] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v5:
* check if PID file still exists on P4D cleanup (thanks Luke)
* fix space/tab formatting error
* add sleep to timeout loops (thanks Luke)
* replace 'date +%s' with platform independent Python function (thanks Eric and Luke)

With the patches of this series the Travis CI test stability increases.
However, as I am "stress testing" the Travis CI infrastructure you can
see that it is not perfect: https://travis-ci.org/larsxschneider/git/builds

Nevertheless, I believe that Travis CI integration has still value as
contributors can test their patches easily on Linux and OS X before
posting them.

@junio / @peff: Do you consider merging this?

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                 |  71 +++++++++++++++++++---
 t/t5504-fetch-receive-strict.sh |   3 +-
 t/t5516-fetch-push.sh           |   6 +-
 t/test-lib-functions.sh         |  39 +++++++-----
 5 files changed, 221 insertions(+), 29 deletions(-)
 create mode 100644 .travis.yml

--
2.5.1

[PATCH v6 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 v6 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 v6 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 | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 7548225..3c9ad9a 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
@@ -36,6 +40,15 @@ native_path() {
 	echo "$path"
 }
 
+# On Solaris the 'date +%s' function is not supported and therefore we
+# need this replacement.
+# Attention: This function is not safe again against time offset updates
+# at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
+# function could fix that but it is not in Python until 3.3.
+time_in_seconds() {
+	python -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.
 #
@@ -121,22 +134,35 @@ p4_add_user() {
 	EOF
 }
 
+retry_until_success() {
+	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
+	until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
+	do
+		sleep 1
+	done
+}
+
+retry_until_fail() {
+	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
+	until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
+	do
+		sleep 1
+	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 v6 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>
Acked-by: Luke Diamand <luke@diamand.org>
---
 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 3c9ad9a..acd5578 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
@@ -94,6 +98,19 @@ start_p4d() {
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
 	pid=$(cat "$pidfile")
+
+	timeout=$(($(time_in_seconds) + $P4D_TIMEOUT))
+	while true
+	do
+		if test $(time_in_seconds) -gt $timeout
+		then
+			kill -9 $pid
+			exit 1
+		fi
+		sleep 1
+	done &
+	watchdog_pid=$!
+
 	ready=
 	while test $i -gt 0
 	do
@@ -156,7 +173,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 v6 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".

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

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index acd5578..f9ae1d7 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -74,6 +74,15 @@ 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.
--
2.5.1

[PATCH v6 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 v6 0/6] Add Travis CI support

[Jeff King]

On Thu, Nov 19, 2015 at 09:58:05AM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> diff to v5:
> * check if PID file still exists on P4D cleanup (thanks Luke)
> * fix space/tab formatting error
> * add sleep to timeout loops (thanks Luke)
> * replace 'date +%s' with platform independent Python function (thanks Eric and Luke)
> 
> With the patches of this series the Travis CI test stability increases.
> However, as I am "stress testing" the Travis CI infrastructure you can
> see that it is not perfect: https://travis-ci.org/larsxschneider/git/builds

I peeked at a few, and it looks like just p4 tests failing now?

> Nevertheless, I believe that Travis CI integration has still value as
> contributors can test their patches easily on Linux and OS X before
> posting them.
> 
> @junio / @peff: Do you consider merging this?

I think I'd prefer to split it into 3 separate topics (de-flaking
test_must_fail, p4 test improvements, and the Travis file). Then they
can proceed independently. I can take care of that split when applying.

> Lars Schneider (6):
>   implement test_might_fail using a refactored test_must_fail

You mentioned in the v5 cover that this one was from Junio. Should it be
"From: Junio ..." in the pseudo-header?

>   add "ok=sigpipe" to test_must_fail and use it to fix flaky tests

Looks OK.

>   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

These are all fairly gross, and I don't have p4d to test with myself.
But if we assume they're all necessary, I suppose it's the best we can
do.

>   Add Travis CI support

I'll leave some comments directly in response to this one.

-Peff

Re: [PATCH v6 6/6] Add Travis CI support

[Jeff King]

On Thu, Nov 19, 2015 at 09:58:11AM +0100, larsxschneider@gmail.com wrote:

> 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.

My overall impression is that this is a lot more complicated than I was
expecting. Can we start with something simpler to gain experience with
Travis?  And then we can add in more complexity later.

For example:

> +addons:
> +  apt:
> +    packages:
> +    - language-pack-is

I doubt most people are running the t0204 right now. Maybe we should
start without it.

> +env:
> +  global:
> +    - P4_VERSION="15.1"
> +    - GIT_LFS_VERSION="1.0.2"

I know p4 is your area of interest, but from a project perspective, it
seems like an odd choice for the initial set of tests.

> +    - DEFAULT_TEST_TARGET=prove
> +    - GIT_PROVE_OPTS="--timer --jobs 3"
> +    - GIT_TEST_OPTS="--verbose --tee"

These all make sense, I guess.

> +    - GETTEXT_ISO_LOCALE=YesPlease
> +    - GETTEXT_LOCALE=YesPlease

What are these? I don't think we have any such Makefile knobs. These
look like variables that get used internally by the test suite, but we
shouldn't need to set them.

> +    # - GETTEXT_POISON=YesPlease

I'm assuming the "#" means this is commented out. Can we just drop such
cruft?

> +    - GIT_TEST_CHAIN_LINT=YesPlease

This is the default, and we don't need to specify it.

> +    - GIT_TEST_CLONE_2GB=YesPlease

Is it a good idea to run such an expensive test?

> +  matrix:
> +    -
> +      # NO_ICONV=YesPlease

Ditto here on the commenting.

> +    - >
> +      NO_CURL=YesPlease

So if I understand correctly, the point of this list is to test
alternate configurations. So compiling without curl makes some sense, I
guess. Though mostly it will just shut off a bunch off tests.

> +      NO_D_INO_IN_DIRENT=YesPlease

But setting platform compat knobs like this seems weird. Nobody sets
this manually. Either your platform has it, or it does not. And we are
already testing on alternate platforms to cover such things.

If there's a specific config setup of interest, it makes sense to me to
try to increase test coverage there. But it feels like you've just
picked a random laundry list of Makefile knobs and tweaked them, without
any sense of whether they even make sense together, or match the
platform.

For instance:

> +      NO_STRCASESTR=YesPlease
> +      NO_STRLCPY=YesPlease

I wouldn't not be surprised if these cause problems building on some
platforms that _do_ have these functions.

> +    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.;

s/Perfoce/Perforce/ :)

> +before_script: make configure && ./configure && make --jobs=2

Hmm. I wonder if we actually want to use autoconf here at all; most devs
do not use it, and Git should generally compile out of the box based on
config.mak.uname (and if it doesn't, it would be nice to know).

There may be some optional packages that autoconf helps with, though.


So overall, I dunno. I do not want to create a bunch of work for you in
splitting it up. But I'm hesitant to merge something that has a bunch of
lines that I'm not sure I understand the reasoning for. :-/

I was hoping the first cut could just be telling Travis to run "make
test", without much fanfare beyond that. Maybe that's not realistic.

-Peff

Re: [PATCH v6 0/6] Add Travis CI support

[Lars Schneider]

On 19 Nov 2015, at 15:14, Jeff King <peff@peff.net> wrote:

> On Thu, Nov 19, 2015 at 09:58:05AM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> diff to v5:
>> * check if PID file still exists on P4D cleanup (thanks Luke)
>> * fix space/tab formatting error
>> * add sleep to timeout loops (thanks Luke)
>> * replace 'date +%s' with platform independent Python function (thanks Eric and Luke)
>> 
>> With the patches of this series the Travis CI test stability increases.
>> However, as I am "stress testing" the Travis CI infrastructure you can
>> see that it is not perfect: https://travis-ci.org/larsxschneider/git/builds
> 
> I peeked at a few, and it looks like just p4 tests failing now?

Yes, in particular t9810-git-p4-rcs.sh and t9816-git-p4-locked.sh. I would probably disable these test in Travis CI until I've found a way to make it stable.

> 
>> Nevertheless, I believe that Travis CI integration has still value as
>> contributors can test their patches easily on Linux and OS X before
>> posting them.
>> 
>> @junio / @peff: Do you consider merging this?
> 
> I think I'd prefer to split it into 3 separate topics (de-flaking
> test_must_fail, p4 test improvements, and the Travis file). Then they
> can proceed independently. I can take care of that split when applying.

Sounds good to me!

> 
>> Lars Schneider (6):
>>  implement test_might_fail using a refactored test_must_fail
> 
> You mentioned in the v5 cover that this one was from Junio. Should it be
> "From: Junio ..." in the pseudo-header?

Yes, this one was from Junio with a minor fix from my end if I recall correctly. What do you mean by "pseudo-header"? The "email-header" in the patch file? 

> 
>>  add "ok=sigpipe" to test_must_fail and use it to fix flaky tests
> 
> Looks OK.

"Looks OK" means I can/should add "Acked-by: Jeff King <peff@peff.net>" ? Bare with me, I am still learning ;-)

> 
>>  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
> 
> These are all fairly gross, and I don't have p4d to test with myself.
> But if we assume they're all necessary, I suppose it's the best we can
> do.

Unfortunately I think they are necessary. However, if someone finds a better way for stable p4d tests then I would be happy to see them go away, again.

> 
>>  Add Travis CI support
> 
> I'll leave some comments directly in response to this one.
> 

Thanks for taking the time to review this!

- Lars

Re: [PATCH v6 6/6] Add Travis CI support

[Lars Schneider]

On 19 Nov 2015, at 15:35, Jeff King <peff@peff.net> wrote:

> On Thu, Nov 19, 2015 at 09:58:11AM +0100, larsxschneider@gmail.com wrote:
> 
>> 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.
> 
> My overall impression is that this is a lot more complicated than I was
> expecting. Can we start with something simpler to gain experience with
> Travis?  And then we can add in more complexity later.
> 
> For example:
> 
>> +addons:
>> +  apt:
>> +    packages:
>> +    - language-pack-is
> 
> I doubt most people are running the t0204 right now. Maybe we should
> start without it.

Well, I think the entire point of a CI is to run automatically tests that most people are *not* running. Plus I can't recall any problems with language-pack-is and t0204.

> 
>> +env:
>> +  global:
>> +    - P4_VERSION="15.1"
>> +    - GIT_LFS_VERSION="1.0.2"
> 
> I know p4 is your area of interest, but from a project perspective, it
> seems like an odd choice for the initial set of tests.

See above. Most people do not execute the p4 tests because they don't have p4 installed. Therefore these tests have the highest risk to brake unnoticed.

> 
>> +    - DEFAULT_TEST_TARGET=prove
>> +    - GIT_PROVE_OPTS="--timer --jobs 3"
>> +    - GIT_TEST_OPTS="--verbose --tee"
> 
> These all make sense, I guess.
> 
>> +    - GETTEXT_ISO_LOCALE=YesPlease
>> +    - GETTEXT_LOCALE=YesPlease
> 
> What are these? I don't think we have any such Makefile knobs. These
> look like variables that get used internally by the test suite, but we
> shouldn't need to set them.

These are used in t020*. I enabled them to run the tests and I haven't seen any problems. However, I am no expert in that area. I'll remove them.

> 
>> +    # - GETTEXT_POISON=YesPlease
> 
> I'm assuming the "#" means this is commented out. Can we just drop such
> cruft?

Yes, for the final submission of course. I just wanted to make it stick out as I wasn't able to run the tests successfully with this flag. I wonder if someone can explain to me how it works.

> 
>> +    - GIT_TEST_CHAIN_LINT=YesPlease
> 
> This is the default, and we don't need to specify it.

OK

> 
>> +    - GIT_TEST_CLONE_2GB=YesPlease
> 
> Is it a good idea to run such an expensive test?

Well, it's a CI machine that does not block anyone. I doubt that people run these tests on their local machines for exactly the reason you just described. Therefore I would rather run these tests.

> 
>> +  matrix:
>> +    -
>> +      # NO_ICONV=YesPlease
> 
> Ditto here on the commenting.

OK. Setting NO_ICONV brakes a lot of tests. Do we really "support" that flag?

> 
>> +    - >
>> +      NO_CURL=YesPlease
> 
> So if I understand correctly, the point of this list is to test
> alternate configurations. So compiling without curl makes some sense, I
> guess. Though mostly it will just shut off a bunch off tests.
> 
>> +      NO_D_INO_IN_DIRENT=YesPlease
> 
> But setting platform compat knobs like this seems weird. Nobody sets
> this manually. Either your platform has it, or it does not. And we are
> already testing on alternate platforms to cover such things.
> 
> If there's a specific config setup of interest, it makes sense to me to
> try to increase test coverage there. But it feels like you've just
> picked a random laundry list of Makefile knobs and tweaked them, without
> any sense of whether they even make sense together, or match the
> platform.
> 
> For instance:
> 
>> +      NO_STRCASESTR=YesPlease
>> +      NO_STRLCPY=YesPlease
> 
> I wouldn't not be surprised if these cause problems building on some
> platforms that _do_ have these functions.

Agreed. As mentioned in gmane/280963 [1] I am no expert in these flags.  Should I remove the "NO_*" test configuration all together?

[1] http://article.gmane.org/gmane.comp.version-control.git/280963/match=no_

> 
>> +    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.;
> 
> s/Perfoce/Perforce/ :)

Thanks :)


>> +before_script: make configure && ./configure && make --jobs=2
> 
> Hmm. I wonder if we actually want to use autoconf here at all; most devs
> do not use it, and Git should generally compile out of the box based on
> config.mak.uname (and if it doesn't, it would be nice to know).
> 
> There may be some optional packages that autoconf helps with, though.

Compiling without autoconf works for Linux but not for OS X:
https://travis-ci.org/larsxschneider/git/jobs/92222770


> So overall, I dunno. I do not want to create a bunch of work for you in
> splitting it up. But I'm hesitant to merge something that has a bunch of
> lines that I'm not sure I understand the reasoning for. :-/

I understand. How about this: I create a new roll where I...
... remove the "NO_*" configuration for now.
... remove GETTEXT_* flags
... disable flaky t9810 and t9816 in the Travis CI run
... disable autoconf and try to fix the OS X issue

> 
> I was hoping the first cut could just be telling Travis to run "make
> test", without much fanfare beyond that. Maybe that's not realistic.


Thanks,
Lars

Re: [PATCH v6 0/6] Add Travis CI support

[Luke Diamand]

On 20 November 2015 at 08:46, Lars Schneider <larsxschneider@gmail.com> wrote:
>
> On 19 Nov 2015, at 15:14, Jeff King <peff@peff.net> wrote:
>
>
>>
>>>  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
>>
>> These are all fairly gross, and I don't have p4d to test with myself.
>> But if we assume they're all necessary, I suppose it's the best we can
>> do.
>
> Unfortunately I think they are necessary. However, if someone finds a better way for stable p4d tests then I would be happy to see them go away, again.

I think that's just how p4d is I'm afraid. It doesn't like being
stopped and started quickly (I guess it's not a normal use-case for
most p4 users). I've made various unsuccessful attempts in the past to
make these tests work reliably, and Lars' changes are far better than
anything I ever managed.

Luke

Re: [PATCH v6 0/6] Add Travis CI support

[Jeff King]

On Fri, Nov 20, 2015 at 12:56:04PM +0000, Luke Diamand wrote:

> >>>  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
> >>
> >> These are all fairly gross, and I don't have p4d to test with myself.
> >> But if we assume they're all necessary, I suppose it's the best we can
> >> do.
> >
> > Unfortunately I think they are necessary. However, if someone finds a better way for stable p4d tests then I would be happy to see them go away, again.
> 
> I think that's just how p4d is I'm afraid. It doesn't like being
> stopped and started quickly (I guess it's not a normal use-case for
> most p4 users). I've made various unsuccessful attempts in the past to
> make these tests work reliably, and Lars' changes are far better than
> anything I ever managed.

Thanks for the extra context. I hope I didn't sound too negative in my
initial assessment. It was meant to be "this is lamentable but probably
necessary".

-Peff

Re: [PATCH v6 0/6] Add Travis CI support

[Luke Diamand]

On 19/11/15 08:58, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> diff to v5:
> * check if PID file still exists on P4D cleanup (thanks Luke)
> * fix space/tab formatting error
> * add sleep to timeout loops (thanks Luke)
> * replace 'date +%s' with platform independent Python function (thanks Eric and Luke)

The first three of these (which fix the git-p4 tests) all look good to 
me. With these changes, running the t98* tests in parallel now works, 
which it did not used to do. The changes are not pretty but I think 
that's inevitable.

Ack from me.

Luke