threads / patch / 40833

v6, 6 partsAdd Travis CI support

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

## tl;dr

14 messages between Nov 19, 2015 and Nov 23, 2015. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · lore
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

larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

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

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(-)
Show changes to t/test-lib-functions.sh +20 −15
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
larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

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

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(-)
Show changes to 3 files +8 −5

t/t5504-fetch-receive-strict.sh, t/t5516-fetch-push.sh, t/test-lib-functions.sh

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
larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

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

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(-)
Show changes to t/lib-git-p4.sh +34 −8
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
larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

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

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(-)
Show changes to t/lib-git-p4.sh +19 −1
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
larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

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

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(+)
Show changes to t/lib-git-p4.sh +9 −1
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
larsxschneider@gmail.com· Nov 19, 2015, 08:58 UTC · re: larsxschneider@gmail.com · lore

[PATCH v6 6/6] Add Travis CI support

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
Show changes to .travis.yml +131 −0
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
Jeff King· Nov 19, 2015, 14:35 UTC · re: larsxschneider@gmail.com · lore

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

On Thu, Nov 19, 2015 at 09:58:11AM +0100, larsxschneider@gmail.com wrote:
Show 7 quoted lines
> 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
Lars Schneider· Nov 20, 2015, 09:29 UTC · re: Jeff King · lore

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

On 19 Nov 2015, at 15:35, Jeff King <peff@peff.net> wrote:
Show 23 quoted lines
> 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.
Show 8 quoted lines
> 
>> +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.
Show 13 quoted lines
> 
>> +    - 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.
Show 5 quoted lines
> 
>> +    # - 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.
Show 6 quoted lines
> 
>> +  matrix:
>> +    -
>> +      # NO_ICONV=YesPlease
> 
> Ditto here on the commenting.
OK. Setting NO_ICONV brakes a lot of tests. Do we really "support" that flag?
Show 27 quoted lines
> 
>> +    - >
>> +      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_
Show 7 quoted lines
> 
>> +    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 :)
Show 7 quoted lines
>> +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

Jeff King· Nov 19, 2015, 14:14 UTC · re: larsxschneider@gmail.com · lore

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

On Thu, Nov 19, 2015 at 09:58:05AM +0100, larsxschneider@gmail.com wrote:
Show 11 quoted lines
> 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?
Show 5 quoted lines
> 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
Lars Schneider· Nov 20, 2015, 08:46 UTC · re: Jeff King · lore

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

On 19 Nov 2015, at 15:14, Jeff King <peff@peff.net> wrote:
Show 15 quoted lines
> 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.
Show 10 quoted lines
> 
>> 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!
Show 6 quoted lines
> 
>> 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 ;-)
Show 8 quoted lines
> 
>>  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.
Show 5 quoted lines
> 
>>  Add Travis CI support
> 
> I'll leave some comments directly in response to this one.
> 
Thanks for taking the time to review this!
- Lars
Luke Diamand· Nov 20, 2015, 12:56 UTC · re: Lars Schneider · lore

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

On 20 November 2015 at 08:46, Lars Schneider <larsxschneider@gmail.com> wrote:
Show 14 quoted lines
>
> 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
Jeff King· Nov 23, 2015, 18:59 UTC · re: Luke Diamand · lore

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

On Fri, Nov 20, 2015 at 12:56:04PM +0000, Luke Diamand wrote:
Show 15 quoted lines
> >>>  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
Luke Diamand· Nov 23, 2015, 21:19 UTC · re: larsxschneider@gmail.com · lore

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

On 19/11/15 08:58, larsxschneider@gmail.com wrote:
Show 7 quoted lines
> 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

← back to recent threads