* [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests
2015-11-06 8:58 [PATCH v4 0/4] Add Travis CI support larsxschneider
@ 2015-11-06 8:58 ` larsxschneider
2015-11-06 18:27 ` Junio C Hamano
2015-11-06 8:58 ` [PATCH v4 2/4] git-p4: add p4d timeout in tests larsxschneider
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: larsxschneider @ 2015-11-06 8:58 UTC (permalink / raw)
To: git
Cc: sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner,
Lars Schneider
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 SIGPIPE.
This patch accepts the SIGPIPE exit as legitimate test exit.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
t/t5504-fetch-receive-strict.sh | 3 +--
t/t5516-fetch-push.sh | 8 ++++----
t/test-lib-functions.sh | 23 +++++++++++++++++++++++
3 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..129efa8 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_or_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..22a941b 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_or_sigpipe git fetch ../testrepo/.git $SHA1_3 &&
+ test_must_fail_or_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 &&
+ test_must_fail_or_sigpipe 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_or_sigpipe git fetch ../testrepo/.git $SHA1_3
)
'
done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 73e37a1..19a598e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -600,6 +600,29 @@ test_must_fail () {
return 0
}
+# Similar to test_must_fail, but tolerates sigpipe signal, too.
+
+test_must_fail_or_sigpipe () {
+ "$@"
+ exit_code=$?
+ if test $exit_code = 0; then
+ echo >&2 "test_must_fail: command succeeded: $*"
+ return 1
+ elif test $exit_code -ne 141 && \
+ 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
+ echo >&2 "test_must_fail: command not found: $*"
+ return 1
+ elif test $exit_code = 126; then
+ echo >&2 "test_must_fail: valgrind error: $*"
+ return 1
+ fi
+ return 0
+}
+
# Similar to test_must_fail, but tolerates success, too. This is
# meant to be used in contexts like:
#
--
2.5.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests
2015-11-06 8:58 ` [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests larsxschneider
@ 2015-11-06 18:27 ` Junio C Hamano
2015-11-06 18:49 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2015-11-06 18:27 UTC (permalink / raw)
To: larsxschneider
Cc: git, sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner
larsxschneider@gmail.com writes:
> 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 SIGPIPE.
Thanks for a clear write-up.
> This patch accepts the SIGPIPE exit as legitimate test exit.
It is not this patch that accepts such a failure as legit ;-).
"Accept such a death-with-sigpipe also as OK when we are expecting a
failure", perhaps?
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index ec22c98..22a941b 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_or_sigpipe git fetch ../testrepo/.git $SHA1_3 &&
> + test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_1 &&
> git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
These I understand.
> git fetch ../testrepo/.git $SHA1_1 &&
> git cat-file commit $SHA1_1 &&
> - test_must_fail git cat-file commit $SHA1_2 &&
> + test_must_fail_or_sigpipe git cat-file commit $SHA1_2 &&
This I don't. Under what condition is it sane for this "cat-file
commit" to fail with sigpipe?
> git fetch ../testrepo/.git $SHA1_2 &&
> git cat-file commit $SHA1_2 &&
> - test_must_fail git fetch ../testrepo/.git $SHA1_3
> + test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3
And this I do.
> )
> '
> done
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 73e37a1..19a598e 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -600,6 +600,29 @@ test_must_fail () {
> return 0
> }
>
> +# Similar to test_must_fail, but tolerates sigpipe signal, too.
> +
> +test_must_fail_or_sigpipe () {
> + "$@"
> + exit_code=$?
> + if test $exit_code = 0; then
> + echo >&2 "test_must_fail: command succeeded: $*"
> + return 1
> + elif test $exit_code -ne 141 && \
> + 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
> + echo >&2 "test_must_fail: command not found: $*"
> + return 1
> + elif test $exit_code = 126; then
> + echo >&2 "test_must_fail: valgrind error: $*"
> + return 1
> + fi
> + return 0
> +}
> +
When you are coming up with this patch, you must have checked the
existing code around this area. Did you notice that 126 is handled
differently between must_fail and might_fail? Can you explain and
justify the difference?
One explanation I can think of is that the contributor who did
eeb69131 (tests: notice valgrind error in test_must_fail,
2013-03-31) did not notice that we do essentially the same thing in
might_fail and forgot to add it. And it is not the fault of that
contributor, but the fault of the duplicated and poorly organized
code back then.
Adding the third variant in the way this patch does is making things
worse by inviting more mistakes.
How about doing something like the attached to consolidate the
existing two into one, and then build this third one on top?
t/test-lib-functions.sh | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..54497d6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -560,22 +560,7 @@ test_line_count () {
# the failure could be due to a segv. We want a controlled failure.
test_must_fail () {
- "$@"
- exit_code=$?
- if 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
- echo >&2 "test_must_fail: died by signal: $*"
- return 1
- elif test $exit_code = 127; then
- echo >&2 "test_must_fail: command not found: $*"
- return 1
- elif test $exit_code = 126; then
- echo >&2 "test_must_fail: valgrind error: $*"
- return 1
- fi
- return 0
+ test_failure_helper test_must_fail "$@"
}
# Similar to test_must_fail, but tolerates success, too. This is
@@ -590,13 +575,29 @@ test_must_fail () {
# because we want to notice if it fails due to segv.
test_might_fail () {
+ test_failure_helper test_might_fail "$@"
+}
+
+test_failure_helper () {
+ test_expect=$1
+ shift
"$@"
exit_code=$?
- if test $exit_code -gt 129 && test $exit_code -le 192; then
- echo >&2 "test_might_fail: died by signal: $*"
+ if test $test_expect != test_might_fail && test $exit_code = 0
+ then
+ echo >&2 "$test_expect: command succeeded: $*"
+ return 1
+ elif test $exit_code -gt 129 && test $exit_code -le 192
+ then
+ echo >&2 "$test_expect: died by signal: $*"
+ return 1
+ elif test $exit_code = 127
+ then
+ echo >&2 "$test_expect: command not found: $*"
return 1
- elif test $exit_code = 127; then
- echo >&2 "test_might_fail: command not found: $*"
+ elif test $exit_code = 126
+ then
+ echo >&2 "$test_expect: valgrind error: $*"
return 1
fi
return 0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests
2015-11-06 18:27 ` Junio C Hamano
@ 2015-11-06 18:49 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2015-11-06 18:49 UTC (permalink / raw)
To: larsxschneider
Cc: git, sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner
Junio C Hamano <gitster@pobox.com> writes:
> Adding the third variant in the way this patch does is making things
> worse by inviting more mistakes.
>
> How about doing something like the attached to consolidate the
> existing two into one, and then build this third one on top?
Actually, I think this other variant I came up with is cleaner and
is more easily extensible. It already has the support for sigpipe,
e.g. you can do
test_must_fail ok=sigpipe git fetch ...
Of course, you can do
test_must_fail_or_sigpipe () {
test_must_fail ok=sigpipe "$@"
}
if it is easier to read. I do not have a very strong opinion either
way.
t/test-lib-functions.sh | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..b732f87 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -560,11 +560,26 @@ 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 ! 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: $*"
return 1
@@ -590,16 +605,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 2/4] git-p4: add p4d timeout in tests
2015-11-06 8:58 [PATCH v4 0/4] Add Travis CI support larsxschneider
2015-11-06 8:58 ` [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests larsxschneider
@ 2015-11-06 8:58 ` larsxschneider
2015-11-06 9:23 ` Eric Sunshine
2015-11-06 8:58 ` [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout larsxschneider
2015-11-06 8:58 ` [PATCH v4 4/4] Add Travis CI support larsxschneider
3 siblings, 1 reply; 20+ messages in thread
From: larsxschneider @ 2015-11-06 8:58 UTC (permalink / raw)
To: git
Cc: sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner,
Lars Schneider
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 7548225..b1660f6 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
+# 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
@@ -81,6 +85,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
@@ -131,7 +148,8 @@ kill_p4d() {
done &&
# 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/4] git-p4: add p4d timeout in tests
2015-11-06 8:58 ` [PATCH v4 2/4] git-p4: add p4d timeout in tests larsxschneider
@ 2015-11-06 9:23 ` Eric Sunshine
2015-11-06 13:20 ` Lars Schneider
0 siblings, 1 reply; 20+ messages in thread
From: Eric Sunshine @ 2015-11-06 9:23 UTC (permalink / raw)
To: Lars Schneider
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
> 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>
> ---
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> @@ -81,6 +85,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
The 'while' line is incorrectly indented with spaces rather than tabs.
> + do
> + if test $(date +%s) -gt $timeout
I don't know how portable you intend this to be, but 'date +%s' is not
universally supported (it's missing on Solaris, for instance, and
perhaps AIX). For 6a9d16a (filter-branch: add passed/remaining seconds
on progress, 2015-09-07), we ultimately settled upon detecting +%s
support dynamically:
if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
# it's supported
fi
Perhaps you'd want to detect this via a lazy prerequisite and skip
this if not supported?
> + then
> + kill -9 $pid
> + exit 1
> + fi
> + sleep 1
> + done &
> + watchdog_pid=$!
> +
> ready=
> while test $i -gt 0
> do
> @@ -131,7 +148,8 @@ kill_p4d() {
> done &&
> # 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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/4] git-p4: add p4d timeout in tests
2015-11-06 9:23 ` Eric Sunshine
@ 2015-11-06 13:20 ` Lars Schneider
0 siblings, 0 replies; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 13:20 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 10:23, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
>> 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>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -81,6 +85,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
>
> The 'while' line is incorrectly indented with spaces rather than tabs.'
Oh, you're right. One more thing that I will add to my pre mailing list submit check script :-)
>
>> + do
>> + if test $(date +%s) -gt $timeout
>
> I don't know how portable you intend this to be, but 'date +%s' is not
> universally supported (it's missing on Solaris, for instance, and
> perhaps AIX). For 6a9d16a (filter-branch: add passed/remaining seconds
> on progress, 2015-09-07), we ultimately settled upon detecting +%s
> support dynamically:
>
> if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
> # it's supported
> fi
>
> Perhaps you'd want to detect this via a lazy prerequisite and skip
> this if not supported?
AFAIK Perforce does not run on Solaris and AIX anyways (see supported platforms [1]). Therefore these tests should not be executed on these platforms. A lazy prerequisite sounds like a good idea to make this explicit!
Thanks,
Lars
[1] https://www.perforce.com/perforce/doc.current/user/relnotes.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout
2015-11-06 8:58 [PATCH v4 0/4] Add Travis CI support larsxschneider
2015-11-06 8:58 ` [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests larsxschneider
2015-11-06 8:58 ` [PATCH v4 2/4] git-p4: add p4d timeout in tests larsxschneider
@ 2015-11-06 8:58 ` larsxschneider
2015-11-06 9:28 ` Eric Sunshine
2015-11-06 8:58 ` [PATCH v4 4/4] Add Travis CI support larsxschneider
3 siblings, 1 reply; 20+ messages in thread
From: larsxschneider @ 2015-11-06 8:58 UTC (permalink / raw)
To: git
Cc: sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner,
Lars Schneider
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 b1660f6..9043b51 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -10,6 +10,10 @@ TEST_NO_CREATE_REPO=NoThanks
# the defined timeout in seconds.
P4D_TIMEOUT=300
+# 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
@@ -138,14 +142,24 @@ 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" &&
@@ -153,8 +167,9 @@ kill_p4d() {
}
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout
2015-11-06 8:58 ` [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout larsxschneider
@ 2015-11-06 9:28 ` Eric Sunshine
2015-11-06 9:47 ` Lars Schneider
2015-11-06 13:19 ` Lars Schneider
0 siblings, 2 replies; 20+ messages in thread
From: Eric Sunshine @ 2015-11-06 9:28 UTC (permalink / raw)
To: Lars Schneider
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
> 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
> +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
> +}
I'm confused by this. Patch 2/4 was already calling
retry_until_fail(), but it's introduction seems to be here in patch
3/4. Am I missing something obvious?
> 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" &&
> @@ -153,8 +167,9 @@ kill_p4d() {
> }
>
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout
2015-11-06 9:28 ` Eric Sunshine
@ 2015-11-06 9:47 ` Lars Schneider
2015-11-06 13:19 ` Lars Schneider
1 sibling, 0 replies; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 9:47 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 10:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
>> 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
>> +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
>> +}
>
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?
No, my fault. I reordered the commits and forgot about this. I will fix the order in the next roll.
Thanks,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout
2015-11-06 9:28 ` Eric Sunshine
2015-11-06 9:47 ` Lars Schneider
@ 2015-11-06 13:19 ` Lars Schneider
1 sibling, 0 replies; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 13:19 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 10:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
>> 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
>> +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
>> +}
>
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?
No, my fault. I reordered the commits and forgot about this. I will fix the order in the next roll.
Thanks,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/4] Add Travis CI support
2015-11-06 8:58 [PATCH v4 0/4] Add Travis CI support larsxschneider
` (2 preceding siblings ...)
2015-11-06 8:58 ` [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout larsxschneider
@ 2015-11-06 8:58 ` larsxschneider
2015-11-06 9:56 ` Eric Sunshine
3 siblings, 1 reply; 20+ messages in thread
From: larsxschneider @ 2015-11-06 8:58 UTC (permalink / raw)
To: git
Cc: sunshine, sschuberth, Matthieu.Moy, avila.jn, luke, dturner,
Lars Schneider
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 8:58 ` [PATCH v4 4/4] Add Travis CI support larsxschneider
@ 2015-11-06 9:56 ` Eric Sunshine
2015-11-06 13:18 ` Lars Schneider
[not found] ` <22B2C2B1-9260-4EC0-A4C5-C7F7DDD388BA@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Eric Sunshine @ 2015-11-06 9:56 UTC (permalink / raw)
To: Lars Schneider
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
> 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>
> ---
> diff --git a/.travis.yml b/.travis.yml
> @@ -0,0 +1,131 @@
> + 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
Hmm, aren't you losing test coverage by disabling some of these? Is
that desirable for continuous integration testing? Am I missing
something?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 9:56 ` Eric Sunshine
@ 2015-11-06 13:18 ` Lars Schneider
[not found] ` <22B2C2B1-9260-4EC0-A4C5-C7F7DDD388BA@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 13:18 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Sebastian Schuberth, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 10:56, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Nov 6, 2015 at 3:58 AM, <larsxschneider@gmail.com> wrote:
>> 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>
>> ---
>> diff --git a/.travis.yml b/.travis.yml
>> @@ -0,0 +1,131 @@
>> + 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
>
> Hmm, aren't you losing test coverage by disabling some of these? Is
> that desirable for continuous integration testing? Am I missing
> something?
Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One normal configuration (see the lonely "-" right under "matrix:") and one configuration with all sorts of things are disabled ("NO...").
You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal, NO_...]) here:
https://travis-ci.org/larsxschneider/git/builds/89598194
Cheers,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread[parent not found: <22B2C2B1-9260-4EC0-A4C5-C7F7DDD388BA@gmail.com>]
* Re: [PATCH v4 4/4] Add Travis CI support
[not found] ` <22B2C2B1-9260-4EC0-A4C5-C7F7DDD388BA@gmail.com>
@ 2015-11-06 13:20 ` Sebastian Schuberth
2015-11-06 13:28 ` Lars Schneider
0 siblings, 1 reply; 20+ messages in thread
From: Sebastian Schuberth @ 2015-11-06 13:20 UTC (permalink / raw)
To: Lars Schneider
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 2:11 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One
> normal configuration (see the lonely "-" right under "matrix:") and one
> configuration with all sorts of things are disabled ("NO...").
>
> You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal,
> NO_...]) here:
> https://travis-ci.org/larsxschneider/git/builds/89598194
Aren't these 8 configurations a bit too much? I see the total running
time is about 2 hours. For my taste, this is way to much to give
developers feedback about the status of their PR. It should be
something < 30 minutes.
IMO, the purpose of the Travis CI configuration mainly is to 1) save
developers work by not requiring to build manually, 2) build on other
platforms than the developer has access to. I doubt that the average
developer manually builds anything close to these 8 configurations
before we had this job, so I wouldn't make it a requirement for Travis
to do much more than a developer could / would to manually.
On the other hand, I see the point in letting a CI system test more
configurations than manually feasible. But that should be done as part
of a different job then. E.g. we could have a "fast" PR validation
job, and a "slow" nightly build job.
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 13:20 ` Sebastian Schuberth
@ 2015-11-06 13:28 ` Lars Schneider
2015-11-06 13:36 ` Sebastian Schuberth
0 siblings, 1 reply; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 13:28 UTC (permalink / raw)
To: Sebastian Schuberth
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 14:20, Sebastian Schuberth <sschuberth@gmail.com> wrote:
>
> On Fri, Nov 6, 2015 at 2:11 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One
>> normal configuration (see the lonely "-" right under "matrix:") and one
>> configuration with all sorts of things are disabled ("NO...").
>>
>> You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal,
>> NO_...]) here:
>> https://travis-ci.org/larsxschneider/git/builds/89598194
>
> Aren't these 8 configurations a bit too much? I see the total running
> time is about 2 hours. For my taste, this is way to much to give
> developers feedback about the status of their PR. It should be
> something < 30 minutes.
>
> IMO, the purpose of the Travis CI configuration mainly is to 1) save
> developers work by not requiring to build manually, 2) build on other
> platforms than the developer has access to. I doubt that the average
> developer manually builds anything close to these 8 configurations
> before we had this job, so I wouldn't make it a requirement for Travis
> to do much more than a developer could / would to manually.
>
> On the other hand, I see the point in letting a CI system test more
> configurations than manually feasible. But that should be done as part
> of a different job then. E.g. we could have a "fast" PR validation
> job, and a "slow" nightly build job.
>
Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).
That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).
What do you think?
Thanks,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 13:28 ` Lars Schneider
@ 2015-11-06 13:36 ` Sebastian Schuberth
2015-11-06 13:55 ` Lars Schneider
0 siblings, 1 reply; 20+ messages in thread
From: Sebastian Schuberth @ 2015-11-06 13:36 UTC (permalink / raw)
To: Lars Schneider
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 2:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).
Are you sure about than? I mean, what sense does it make to show how
long it *would* have taken *if* the builds were running serially? I
can see that the longest of the jobs took 21 minutes, which is ok. But
that does not mean that all jobs completed in within 21 minutes. It
could be that not all jobs started at (about) the same time due to a
lack of resources, and that the last job did not compete before the 2
hours were over because it only started to run 1 hours and 40 minutes
befor ethe first job was started.
> That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).
>
> What do you think?
I think running different configuration per branch makes sense, yes.
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 13:36 ` Sebastian Schuberth
@ 2015-11-06 13:55 ` Lars Schneider
2015-11-06 13:57 ` Sebastian Schuberth
0 siblings, 1 reply; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 13:55 UTC (permalink / raw)
To: Sebastian Schuberth
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 14:36, Sebastian Schuberth <sschuberth@gmail.com> wrote:
>
> On Fri, Nov 6, 2015 at 2:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).
>
> Are you sure about than? I mean, what sense does it make to show how
> long it *would* have taken *if* the builds were running serially? I
> can see that the longest of the jobs took 21 minutes, which is ok. But
> that does not mean that all jobs completed in within 21 minutes. It
> could be that not all jobs started at (about) the same time due to a
> lack of resources, and that the last job did not compete before the 2
> hours were over because it only started to run 1 hours and 40 minutes
> befor ethe first job was started.
I am fairly certain about this.
See, here is the first configuration and the first test case of a job:
https://travis-ci.org/larsxschneider/git/jobs/89598195
[08:21:06] t0002-gitfile.sh
Here is the last configuration and the last test case of the same job:
[08:51:22] t9903-bash-prompt.sh
~30 min for all 8 configurations. You can enable Travis CI for you GitHub account and try it easily yourself :-)
>
>> That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).
>>
>> What do you think?
>
> I think running different configuration per branch makes sense, yes.
If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?
Thanks,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 13:55 ` Lars Schneider
@ 2015-11-06 13:57 ` Sebastian Schuberth
2015-11-06 14:08 ` Lars Schneider
0 siblings, 1 reply; 20+ messages in thread
From: Sebastian Schuberth @ 2015-11-06 13:57 UTC (permalink / raw)
To: Lars Schneider
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
On Fri, Nov 6, 2015 at 2:55 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>> I think running different configuration per branch makes sense, yes.
>
> If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?
No. I think this could be addressed later as an improvements. To me
it's more important to finally get *some* sane Travis CI configuration
in.
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/4] Add Travis CI support
2015-11-06 13:57 ` Sebastian Schuberth
@ 2015-11-06 14:08 ` Lars Schneider
0 siblings, 0 replies; 20+ messages in thread
From: Lars Schneider @ 2015-11-06 14:08 UTC (permalink / raw)
To: Sebastian Schuberth
Cc: Eric Sunshine, Git List, Matthieu Moy, Jean-Noel Avila,
Luke Diamand, David Turner
> On 06 Nov 2015, at 14:57, Sebastian Schuberth <sschuberth@gmail.com> wrote:
>
> On Fri, Nov 6, 2015 at 2:55 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>>> I think running different configuration per branch makes sense, yes.
>>
>> If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?
>
> No. I think this could be addressed later as an improvements. To me
> it's more important to finally get *some* sane Travis CI configuration
> in.
True. However, as I stated in my v4 cover letter the Travis CI integration is not yet perfect. I am constantly running builds to find flaky tests. Eg. here is one of them in git-p4 area that I will tackle next:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603763/log.txt
I also see a weird "prove Tests our of sequence" error one in a while:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603770/log.txt
Does anyone have an idea what could cause this?
Thanks,
Lars
^ permalink raw reply [flat|nested] 20+ messages in thread