git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services
@ 2018-09-03 21:10 Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 1/9] ci: rename the library of common functions Johannes Schindelin via GitGitGadget
                   ` (9 more replies)
  0 siblings, 10 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

For a long time already, we have Git's source code continuously tested via
Travis CI, see e.g. https://travis-ci.org/git/git/builds/421738884. It has
served us well, and more and more developers actually pay attention and
benefit from the testing this gives us.

It is also an invaluable tool for contributors who can validate their code
contributions via PRs on GitHub, e.g. to verify that their tests do actually
run on macOS (i.e. with the BSD family of Unix tools instead of the GNU
one).

The one sad part about this is the Windows support. Travis lacks it, and we
work around that by using Visual Studio Team Services (VSTS) indirectly: one
phase in Travis would trigger a build, wait for its log, and then paste that
log.

As Git's Windows builds (and tests!) take quite a bit of time, Travis often
timed out, or somehow the trigger did not work, and for security reasons
(the Windows builds are performed in a private pool of containers), the
Windows builds are completely disabled for Pull Requests on GitHub.

One might ask why we did not use Visual Studio Team Services directly. There
were a couple of reasons for that:

 * most notably, VSTS's build logs could not be viewed anonymously,
 * while VSTS had Linux and Windows agents, it lacked macOS agents,
 * etc

The main two reasons no longer apply: macOS agents are available now
[https://docs.microsoft.com/en-us/vsts/release-notes/2018/jul-10-vsts], and
there is a limited preview of "public projects"
[https://blogs.msdn.microsoft.com/devops/2018/04/27/vsts-public-projects-limited-preview/]
, i.e. it is possible to configure a VSTS project so that anybody can view
the logs.

I had secured such a public project for Git for Windows already, and I
recently also got one for Git. For now, the latter is hooked up with my
personal git.git fork on GitHub, but it is my hope that I convince y'all
that these VSTS builds are a good idea, and then hook it up with 
https://github.com/git/git.

As a special treat, this patch series adds the ability to present the
outcome of Git's test suite as JUnit-style .xml files. This allows the VSTS
build to present fun diagrams, trends, and makes it a lot easier to drill
down to test failures than before. See for example 
https://git.visualstudio.com/git/_build/results?buildId=113&view=ms.vss-test-web.test-result-details
[https://git.visualstudio.com/git/_build/results?buildId=113&view=ms.vss-test-web.test-result-details] 
(you can click on the label of the failed test, and then see the detailed
output in the right pane).

This patch series took way more time than I had originally planned, but I
think that in particular the advanced display of the test results was worth
it. Please let me know what you think about this.

Johannes Schindelin (9):
  ci: rename the library of common functions
  ci/lib.sh: encapsulate Travis-specific things
  test-date: add a subcommand to measure times in shell scripts
  tests: optionally write results as JUnit-style .xml
  ci/lib.sh: add support for VSTS CI
  Add a build definition for VSTS
  tests: include detailed trace logs with --write-junit-xml upon failure
  tests: record more stderr with --write-junit-xml in case of failure
  README: add a build badge (status of the VSTS build)

 .vsts-ci.yml                   | 296 +++++++++++++++++++++++++++++++++
 README.md                      |   2 +
 ci/install-dependencies.sh     |   5 +-
 ci/{lib-travisci.sh => lib.sh} |  67 ++++++--
 ci/mount-fileshare.sh          |  26 +++
 ci/print-test-failures.sh      |   4 +-
 ci/run-build-and-tests.sh      |   2 +-
 ci/run-linux32-docker.sh       |   2 +-
 ci/run-static-analysis.sh      |   2 +-
 ci/run-windows-build.sh        |   2 +-
 ci/test-documentation.sh       |   3 +-
 t/.gitignore                   |   1 +
 t/helper/test-date.c           |  12 ++
 t/test-lib.sh                  | 142 ++++++++++++++++
 14 files changed, 544 insertions(+), 22 deletions(-)
 create mode 100644 .vsts-ci.yml
 rename ci/{lib-travisci.sh => lib.sh} (61%)
 create mode 100755 ci/mount-fileshare.sh


base-commit: 2f743933341f276111103550fbf383a34dfcfd38
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-31%2Fdscho%2Fvsts-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-31/dscho/vsts-ci-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/31
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 1/9] ci: rename the library of common functions
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things Johannes Schindelin via GitGitGadget
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The name is hard-coded to reflect that we use Travis CI for continuous
testing.

In the next commits, we will extend this to be able use Visual Studio
Team Services, too.

So let's adjust the name to make it more generic.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/install-dependencies.sh     | 2 +-
 ci/{lib-travisci.sh => lib.sh} | 0
 ci/print-test-failures.sh      | 2 +-
 ci/run-build-and-tests.sh      | 2 +-
 ci/run-linux32-docker.sh       | 2 +-
 ci/run-static-analysis.sh      | 2 +-
 ci/run-windows-build.sh        | 2 +-
 ci/test-documentation.sh       | 2 +-
 8 files changed, 7 insertions(+), 7 deletions(-)
 rename ci/{lib-travisci.sh => lib.sh} (100%)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 75a9fd2475..961064658e 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -3,7 +3,7 @@
 # Install dependencies required to build and test Git on Linux and macOS
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
diff --git a/ci/lib-travisci.sh b/ci/lib.sh
similarity index 100%
rename from ci/lib-travisci.sh
rename to ci/lib.sh
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index d55460a212..7aef39a2fd 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -3,7 +3,7 @@
 # Print output of failing tests
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 # Tracing executed commands would produce too much noise in the loop below.
 set +x
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2a5bff4a1c..e28ac2fb9a 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -3,7 +3,7 @@
 # Build and test Git
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 ln -s "$cache_dir/.prove" t/.prove
 
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 21637903ce..751acfcf8a 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -3,7 +3,7 @@
 # Download and run Docker image to build and test 32-bit Git
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 docker pull daald/ubuntu32:xenial
 
diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index 5688f261d0..dc189c7456 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh
@@ -3,7 +3,7 @@
 # Perform various static code analysis checks
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 make --jobs=2 coccicheck
 
diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
index d99a180e52..a73a4eca0a 100755
--- a/ci/run-windows-build.sh
+++ b/ci/run-windows-build.sh
@@ -6,7 +6,7 @@
 # supported) and a commit hash.
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 test $# -ne 2 && echo "Unexpected number of parameters" && exit 1
 test -z "$GFW_CI_TOKEN" && echo "GFW_CI_TOKEN not defined" && exit
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index a20de9ca12..d3cdbac73f 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -3,7 +3,7 @@
 # Perform sanity checks on documentation and build it.
 #
 
-. ${0%/*}/lib-travisci.sh
+. ${0%/*}/lib.sh
 
 gem install asciidoctor
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 1/9] ci: rename the library of common functions Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 23:43   ` Eric Sunshine
  2018-09-05 18:57   ` Sebastian Schuberth
  2018-09-03 21:10 ` [PATCH 3/9] test-date: add a subcommand to measure times in shell scripts Johannes Schindelin via GitGitGadget
                   ` (7 subsequent siblings)
  9 siblings, 2 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The upcoming patches will allow building git.git via VSTS CI, where
variable names and URLs look a bit different than in Travis CI.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/install-dependencies.sh |  3 ++-
 ci/lib.sh                  | 44 +++++++++++++++++++++++++++-----------
 ci/print-test-failures.sh  |  2 +-
 ci/test-documentation.sh   |  1 +
 4 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 961064658e..6d92cc1cba 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -28,7 +28,8 @@ osx-clang|osx-gcc)
 	brew update --quiet
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time
-	brew install git-lfs gettext
+	test -z "$BREW_INSTALL_PACKAGES" ||
+	eval brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 	brew install caskroom/cask/perforce
 	;;
diff --git a/ci/lib.sh b/ci/lib.sh
index 06970f7213..657ff88672 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,5 +1,26 @@
 # Library of functions shared by all CI scripts
 
+if test -n "$TRAVIS_COMMIT"
+then
+	# We are running within Travis CI
+	CI_BRANCH="$TRAVIS_BRANCH"
+	CI_COMMIT="$TRAVIS_COMMIT"
+	CI_JOB_ID="$TRAVIS_JOB_ID"
+	CI_JOB_NUMBER="$TRAVIS_JOB_NUMBER"
+	CI_OS_NAME="$TRAVIS_OS_NAME"
+	CI_REPO_SLUG="$TRAVIS_REPO_SLUG"
+
+	cache_dir="$HOME/travis-cache"
+
+	url_for_job_id () {
+		echo "https://travis-ci.org/$CI_REPO_SLUG/jobs/$1"
+	}
+
+	BREW_INSTALL_PACKAGES="git-lfs gettext"
+	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
+	export GIT_TEST_OPTS="--verbose-log -x --immediate"
+fi
+
 skip_branch_tip_with_tag () {
 	# Sometimes, a branch is pushed at the same time the tag that points
 	# at the same commit as the tip of the branch is pushed, and building
@@ -13,10 +34,10 @@ skip_branch_tip_with_tag () {
 	# we can skip the build because we won't be skipping a build
 	# of a tag.
 
-	if TAG=$(git describe --exact-match "$TRAVIS_BRANCH" 2>/dev/null) &&
-		test "$TAG" != "$TRAVIS_BRANCH"
+	if TAG=$(git describe --exact-match "$CI_BRANCH" 2>/dev/null) &&
+		test "$TAG" != "$CI_BRANCH"
 	then
-		echo "$(tput setaf 2)Tip of $TRAVIS_BRANCH is exactly at $TAG$(tput sgr0)"
+		echo "$(tput setaf 2)Tip of $CI_BRANCH is exactly at $TAG$(tput sgr0)"
 		exit 0
 	fi
 }
@@ -25,7 +46,7 @@ skip_branch_tip_with_tag () {
 # job if we encounter the same tree again and can provide a useful info
 # message.
 save_good_tree () {
-	echo "$(git rev-parse $TRAVIS_COMMIT^{tree}) $TRAVIS_COMMIT $TRAVIS_JOB_NUMBER $TRAVIS_JOB_ID" >>"$good_trees_file"
+	echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
 	# limit the file size
 	tail -1000 "$good_trees_file" >"$good_trees_file".tmp
 	mv "$good_trees_file".tmp "$good_trees_file"
@@ -35,7 +56,7 @@ save_good_tree () {
 # successfully before (e.g. because the branch got rebased, changing only
 # the commit messages).
 skip_good_tree () {
-	if ! good_tree_info="$(grep "^$(git rev-parse $TRAVIS_COMMIT^{tree}) " "$good_trees_file")"
+	if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
 	then
 		# Haven't seen this tree yet, or no cached good trees file yet.
 		# Continue the build job.
@@ -45,18 +66,18 @@ skip_good_tree () {
 	echo "$good_tree_info" | {
 		read tree prev_good_commit prev_good_job_number prev_good_job_id
 
-		if test "$TRAVIS_JOB_ID" = "$prev_good_job_id"
+		if test "$CI_JOB_ID" = "$prev_good_job_id"
 		then
 			cat <<-EOF
-			$(tput setaf 2)Skipping build job for commit $TRAVIS_COMMIT.$(tput sgr0)
+			$(tput setaf 2)Skipping build job for commit $CI_COMMIT.$(tput sgr0)
 			This commit has already been built and tested successfully by this build job.
 			To force a re-build delete the branch's cache and then hit 'Restart job'.
 			EOF
 		else
 			cat <<-EOF
-			$(tput setaf 2)Skipping build job for commit $TRAVIS_COMMIT.$(tput sgr0)
+			$(tput setaf 2)Skipping build job for commit $CI_COMMIT.$(tput sgr0)
 			This commit's tree has already been built and tested successfully in build job $prev_good_job_number for commit $prev_good_commit.
-			The log of that build job is available at https://travis-ci.org/$TRAVIS_REPO_SLUG/jobs/$prev_good_job_id
+			The log of that build job is available at $(url_for_job_id $prev_good_job_id)
 			To force a re-build delete the branch's cache and then hit 'Restart job'.
 			EOF
 		fi
@@ -81,7 +102,6 @@ check_unignored_build_artifacts ()
 # and installing dependencies.
 set -ex
 
-cache_dir="$HOME/travis-cache"
 good_trees_file="$cache_dir/good-trees"
 
 mkdir -p "$cache_dir"
@@ -91,13 +111,11 @@ skip_good_tree
 
 if test -z "$jobname"
 then
-	jobname="$TRAVIS_OS_NAME-$CC"
+	jobname="$CI_OS_NAME-$CC"
 fi
 
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
-export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-export GIT_TEST_OPTS="--verbose-log -x --immediate"
 export GIT_TEST_CLONE_2GB=YesPlease
 if [ "$jobname" = linux-gcc ]; then
 	export CC=gcc-8
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 7aef39a2fd..d2045b63a6 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -69,7 +69,7 @@ do
 	fi
 done
 
-if [ $combined_trash_size -gt 0 ]
+if [ -n "$TRAVIS_JOB_ID" -a $combined_trash_size -gt 0 ]
 then
 	echo "------------------------------------------------------------------------"
 	echo "Trash directories embedded in this log can be extracted by running:"
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index d3cdbac73f..7d0beb2832 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -5,6 +5,7 @@
 
 . ${0%/*}/lib.sh
 
+test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 gem install asciidoctor
 
 make check-builtins
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 3/9] test-date: add a subcommand to measure times in shell scripts
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 1/9] ci: rename the library of common functions Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 4/9] tests: optionally write results as JUnit-style .xml Johannes Schindelin via GitGitGadget
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the next commit, we want to teach Git's test suite to optionally
output test results in JUnit-style .xml files. These files contain
information about the time spent. So we need a way to measure time.

While we could use `date +%s` for that, this will give us only seconds,
i.e. very coarse-grained timings.

GNU `date` supports `date +%s.%N` (i.e. nanosecond-precision output),
but there is no equivalent in BSD `date` (read: on macOS, we would not
be able to obtain precise timings).

So let's introduce `test-tool date getnanos`, with an optional start
time, that outputs preciser values.

Granted, it is a bit pointless to try measuring times accurately in
shell scripts, certainly to nanosecond precision. But it is better than
second-granularity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/helper/test-date.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index a0837371ab..792a805374 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -7,6 +7,7 @@ static const char *usage_msg = "\n"
 "  test-tool date parse [date]...\n"
 "  test-tool date approxidate [date]...\n"
 "  test-tool date timestamp [date]...\n"
+"  test-tool date getnanos [start-nanos]\n"
 "  test-tool date is64bit\n"
 "  test-tool date time_t-is64bit\n";
 
@@ -82,6 +83,15 @@ static void parse_approx_timestamp(const char **argv, struct timeval *now)
 	}
 }
 
+static void getnanos(const char **argv, struct timeval *now)
+{
+	double seconds = getnanotime() / 1.0e9;
+
+	if (*argv)
+		seconds -= strtod(*argv, NULL);
+	printf("%lf\n", seconds);
+}
+
 int cmd__date(int argc, const char **argv)
 {
 	struct timeval now;
@@ -108,6 +118,8 @@ int cmd__date(int argc, const char **argv)
 		parse_approxidate(argv+1, &now);
 	else if (!strcmp(*argv, "timestamp"))
 		parse_approx_timestamp(argv+1, &now);
+	else if (!strcmp(*argv, "getnanos"))
+		getnanos(argv+1, &now);
 	else if (!strcmp(*argv, "is64bit"))
 		return sizeof(timestamp_t) == 8 ? 0 : 1;
 	else if (!strcmp(*argv, "time_t-is64bit"))
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 4/9] tests: optionally write results as JUnit-style .xml
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 3/9] test-date: add a subcommand to measure times in shell scripts Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-04  0:43   ` Eric Sunshine
  2018-09-03 21:10 ` [PATCH 5/9] ci/lib.sh: add support for VSTS CI Johannes Schindelin via GitGitGadget
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This will come in handy when publishing the results of Git's test suite
during an automated VSTS CI run.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/.gitignore  |  1 +
 t/test-lib.sh | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

diff --git a/t/.gitignore b/t/.gitignore
index 348715f0e4..91cf5772fe 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -2,3 +2,4 @@
 /test-results
 /.prove
 /chainlinttmp
+/out/
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8bb0f4348e..50a65a600e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -288,6 +288,9 @@ do
 	--verbose-log)
 		verbose_log=t
 		shift ;;
+	--write-junit-xml)
+		write_junit_xml=t
+		shift ;;
 	*)
 		echo "error: unknown test option '$1'" >&2; exit 1 ;;
 	esac
@@ -431,11 +434,24 @@ trap 'exit $?' INT
 # the test_expect_* functions instead.
 
 test_ok_ () {
+	if test -n "$write_junit_xml"
+	then
+		write_junit_xml_testcase "$*"
+	fi
 	test_success=$(($test_success + 1))
 	say_color "" "ok $test_count - $@"
 }
 
 test_failure_ () {
+	if test -n "$write_junit_xml"
+	then
+		junit_insert="<failure message=\"not ok $test_count -"
+		junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
+		junit_insert="$junit_insert $(xml_attr_encode \
+			"$(printf '%s\n' "$@" | sed 1d)")"
+		junit_insert="$junit_insert</failure>"
+		write_junit_xml_testcase "$1" "      $junit_insert"
+	fi
 	test_failure=$(($test_failure + 1))
 	say_color error "not ok $test_count - $1"
 	shift
@@ -444,11 +460,19 @@ test_failure_ () {
 }
 
 test_known_broken_ok_ () {
+	if test -n "$write_junit_xml"
+	then
+		write_junit_xml_testcase "$* (breakage fixed)"
+	fi
 	test_fixed=$(($test_fixed+1))
 	say_color error "ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
+	if test -n "$write_junit_xml"
+	then
+		write_junit_xml_testcase "$* (known breakage)"
+	fi
 	test_broken=$(($test_broken+1))
 	say_color warn "not ok $test_count - $@ # TODO known breakage"
 }
@@ -706,6 +730,10 @@ test_start_ () {
 	test_count=$(($test_count+1))
 	maybe_setup_verbose
 	maybe_setup_valgrind
+	if test -n "$write_junit_xml"
+	then
+		junit_start=$(test-tool date getnanos)
+	fi
 }
 
 test_finish_ () {
@@ -743,6 +771,13 @@ test_skip () {
 
 	case "$to_skip" in
 	t)
+		if test -n "$write_junit_xml"
+		then
+			message="$(xml_attr_encode "$skipped_reason")"
+			write_junit_xml_testcase "$1" \
+				"      <skipped message=\"$message\" />"
+		fi
+
 		say_color skip >&3 "skipping test: $@"
 		say_color skip "ok $test_count # skip $1 ($skipped_reason)"
 		: true
@@ -758,9 +793,58 @@ test_at_end_hook_ () {
 	:
 }
 
+write_junit_xml () {
+	case "$1" in
+	--truncate)
+		>"$junit_xml_path"
+		junit_have_testcase=
+		shift
+		;;
+	esac
+	printf '%s\n' "$@" >>"$junit_xml_path"
+}
+
+xml_attr_encode () {
+	# We do not translate CR to &#x0d; because BSD sed does not handle
+	# \r in the regex. In practice, the output should not even have any
+	# carriage returns.
+	printf '%s\n' "$@" |
+	sed -e 's/&/\&amp;/g' -e "s/'/\&apos;/g" -e 's/"/\&quot;/g' \
+		-e 's/</\&lt;/g' -e 's/>/\&gt;/g' \
+		-e 's/	/\&#x09;/g' -e 's/$/\&#x0a;/' -e '$s/&#x0a;$//' |
+	tr -d '\012\015'
+}
+
+write_junit_xml_testcase () {
+	junit_attrs="name=\"$(xml_attr_encode "$this_test.$test_count $1")\""
+	shift
+	junit_attrs="$junit_attrs classname=\"$this_test\""
+	junit_attrs="$junit_attrs time=\"$(test-tool \
+		date getnanos $junit_start)\""
+	write_junit_xml "$(printf '%s\n' \
+		"    <testcase $junit_attrs>" "$@" "    </testcase>")"
+	junit_have_testcase=t
+}
+
 test_done () {
 	GIT_EXIT_OK=t
 
+	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
+	then
+		test -n "$junit_have_testcase" || {
+			junit_start=$(test-tool date getnanos)
+			write_junit_xml_testcase "all tests skipped"
+		}
+
+		# adjust the overall time
+		junit_time=$(test-tool date getnanos $junit_suite_start)
+		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+			<"$junit_xml_path" >"$junit_xml_path.new"
+		mv "$junit_xml_path.new" "$junit_xml_path"
+
+		write_junit_xml "  </testsuite>" "</testsuites>"
+	fi
+
 	if test -z "$HARNESS_ACTIVE"
 	then
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -996,6 +1080,7 @@ then
 else
 	mkdir -p "$TRASH_DIRECTORY"
 fi
+
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || exit 1
@@ -1009,6 +1094,19 @@ then
 	test_done
 fi
 
+if test -n "$write_junit_xml"
+then
+	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
+	mkdir -p "$junit_xml_dir"
+	junit_xml_base=${0##*/}
+	junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
+	junit_attrs="name=\"${junit_xml_base%.sh}\""
+	junit_attrs="$junit_attrs timestamp=\"$(TZ=UTC \
+		date +%Y-%m-%dT%H:%M:%S)\""
+	write_junit_xml --truncate "<testsuites>" "  <testsuite $junit_attrs>"
+	junit_suite_start=$(test-tool date getnanos)
+fi
+
 # Provide an implementation of the 'yes' utility
 yes () {
 	if test $# = 0
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 5/9] ci/lib.sh: add support for VSTS CI
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (3 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 4/9] tests: optionally write results as JUnit-style .xml Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 6/9] Add a build definition for VSTS Johannes Schindelin via GitGitGadget
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This patch introduces a conditional arm that defines some environment
variables and a function that displays the URL given the job id (to
identify previous runs for known-good trees).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/lib.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index 657ff88672..f2f6d70c72 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -19,6 +19,29 @@ then
 	BREW_INSTALL_PACKAGES="git-lfs gettext"
 	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 	export GIT_TEST_OPTS="--verbose-log -x --immediate"
+elif test -n "$SYSTEM_TASKDEFINITIONSURI"
+then
+	# We are running in VSTS CI
+	CI_BRANCH="$BUILD_SOURCEBRANCH"
+	CI_COMMIT="$BUILD_SOURCEVERSION"
+	CI_JOB_ID="$BUILD_BUILDID"
+	CI_JOB_NUMBER="$BUILD_BUILDNUMBER"
+	CI_OS_NAME="$(echo "$AGENT_OS" | tr A-Z a-z)"
+	test darwin != "$CI_OS_NAME" || CI_OS_NAME=osx
+	CI_REPO_SLUG="$(expr "$BUILD_REPOSITORY_URI" : '.*/\([^/]*/[^/]*\)$')"
+	CC="${CC:-gcc}"
+
+	# use a subdirectory of the cache dir (because the file share is shared
+	# among *all* phases)
+	cache_dir="$HOME/vsts-cache/$SYSTEM_PHASENAME"
+
+	url_for_job_id () {
+		echo "$SYSTEM_TASKDEFINITIONSURI$SYSTEM_TEAMPROJECT/_build/results?buildId=$1"
+	}
+
+	BREW_INSTALL_PACKAGES=
+	export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
+	export GIT_TEST_OPTS="--quiet --write-junit-xml"
 fi
 
 skip_branch_tip_with_tag () {
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 6/9] Add a build definition for VSTS
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (4 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 5/9] ci/lib.sh: add support for VSTS CI Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure Johannes Schindelin via GitGitGadget
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This commit adds a .vsts-ci.yml which is Visual Studio Team Services'
equivalent to Travis CI's .travis.yml.

To make things a bit easier to understand, we refrain from using the
`matrix` feature here because (while it is powerful) it can be a bit
confusing to users who are not familiar with CI setups. Therefore, we
use a separate phase even for similar configurations (such as GCC vs
Clang on Linux, GCC vs Clang on macOS).

Also, we make use of the shiny new feature we just introduced where the
test suite can output JUnit-style .xml files. This information is made
available in a nice UI that allows the viewer to filter by phase and/or
test number, and to see trends such as: number of (failing) tests, time
spent running the test suite, etc.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .vsts-ci.yml          | 296 ++++++++++++++++++++++++++++++++++++++++++
 ci/mount-fileshare.sh |  26 ++++
 2 files changed, 322 insertions(+)
 create mode 100644 .vsts-ci.yml
 create mode 100755 ci/mount-fileshare.sh

diff --git a/.vsts-ci.yml b/.vsts-ci.yml
new file mode 100644
index 0000000000..ff1d952df9
--- /dev/null
+++ b/.vsts-ci.yml
@@ -0,0 +1,296 @@
+resources:
+- repo: self
+  fetchDepth: 1
+
+phases:
+- phase: linux_clang
+  displayName: linux-clang
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev apache2-bin
+
+       export CC=clang
+
+       ci/install-dependencies.sh
+       ci/run-build-and-tests.sh || {
+           ci/print-test-failures.sh
+           exit 1
+       }
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-build-and-tests.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'linux-clang'
+      platform: Linux
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: linux_gcc
+  displayName: linux-gcc
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev apache2-bin
+
+       ci/install-dependencies.sh
+       ci/run-build-and-tests.sh || {
+           ci/print-test-failures.sh
+           exit 1
+       }
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-build-and-tests.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'linux-gcc'
+      platform: Linux
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: osx_clang
+  displayName: osx-clang
+  condition: succeeded()
+  queue:
+    name: Hosted macOS
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       export CC=clang
+
+       ci/install-dependencies.sh
+       ci/run-build-and-tests.sh || {
+           ci/print-test-failures.sh
+           exit 1
+       }
+
+       umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-build-and-tests.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'osx-clang'
+      platform: macOS
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: osx_gcc
+  displayName: osx-gcc
+  condition: succeeded()
+  queue:
+    name: Hosted macOS
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       ci/install-dependencies.sh
+       ci/run-build-and-tests.sh || {
+           ci/print-test-failures.sh
+           exit 1
+       }
+
+       umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-build-and-tests.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'osx-gcc'
+      platform: macOS
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: gettext_poison
+  displayName: GETTEXT_POISON
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev
+
+       export jobname=GETTEXT_POISON
+
+       ci/run-build-and-tests.sh || {
+           ci/print-test-failures.sh
+           exit 1
+       }
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-build-and-tests.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'gettext-poison'
+      platform: Linux
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: windows
+  displayName: Windows
+  condition: succeeded()
+  queue:
+    name: Hosted VS2017
+    timeoutInMinutes: 240
+  steps:
+  - powershell: |
+       # Helper to check the error level of the latest command (exit with error when appropriate)
+       function c() { if (!$?) { exit(1) } }
+
+       net use s: \\gitfileshare.file.core.windows.net\vsts-cache "$(gitfileshare.pwd)" /user:AZURE\gitfileshare /persistent:no; c
+       cmd /c mklink /d "$(Build.SourcesDirectory)\vsts-cache" S:\; c
+
+       # Add build agent's MinGit to PATH
+       $env:PATH = $env:AGENT_HOMEDIRECTORY +"\externals\\git\cmd;" +$env:PATH
+
+       # Helper to initialize (or update) a Git worktree
+       function init ($path, $url, $set_origin) {
+           if (Test-Path $path) {
+               cd $path; c
+               if (Test-Path .git) {
+                   git init; c
+               } else {
+                   git status
+               }
+           } else {
+               git init $path; c
+               cd $path; c
+           }
+           git config core.autocrlf false; c
+           git config core.untrackedCache true; c
+           if (($set_origin -ne 0) -and !(git config remote.origin.url)) {
+               git remote add origin $url; c
+           }
+           git fetch --depth=1 $url master; c
+           git reset --hard FETCH_HEAD; c
+           git clean -df; c
+       }
+
+       # Initialize Git for Windows' SDK
+       $sdk_path = "$(Build.SourcesDirectory)\git-sdk-64"
+       init "$sdk_path" "https://git-for-windows.visualstudio.com/_git/git-sdk-64" 0
+       init usr\src\build-extra https://github.com/git-for-windows/build-extra 1
+
+       cd "$(Build.SourcesDirectory)"; c
+
+       $env:HOME = "$(Build.SourcesDirectory)"
+       $env:MSYSTEM = "MINGW64"
+       git-sdk-64\git-cmd --command=usr\\bin\\bash.exe -lc @"
+         . ci/lib.sh
+
+         make -j10 DEVELOPER=1 NO_PERL=1 || exit 1
+         NO_PERL=1 NO_SVN_TESTS=1 GIT_TEST_OPTS=\"--quiet --write-junit-xml\" time make -j15 -k DEVELOPER=1 test || {
+           NO_PERL=1 NO_SVN_TESTS=1 GIT_TEST_OPTS=\"-i -v -x\" make -k failed; exit 1
+         }
+
+         save_good_tree
+       "@
+
+       cmd /c rmdir "$(Build.SourcesDirectory)\vsts-cache"
+    displayName: 'build & test'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'windows'
+      platform: Windows
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: linux32
+  displayName: Linux32
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get -y install \
+           apt-transport-https \
+           ca-certificates \
+           curl \
+           software-properties-common
+       curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo apt-key add -
+       sudo add-apt-repository \
+          "deb [arch=amd64] https://download.docker.com/linux/ubuntu \
+          $(lsb_release -cs) \
+          stable"
+       sudo apt-get update
+       sudo apt-get -y install docker-ce
+
+       sudo AGENT_OS="$AGENT_OS" BUILD_BUILDNUMBER="$BUILD_BUILDNUMBER" BUILD_REPOSITORY_URI="$BUILD_REPOSITORY_URI" BUILD_SOURCEBRANCH="$BUILD_SOURCEBRANCH" BUILD_SOURCEVERSION="$BUILD_SOURCEVERSION" SYSTEM_PHASENAME="$SYSTEM_PHASENAME" SYSTEM_TASKDEFINITIONSURI="$SYSTEM_TASKDEFINITIONSURI" SYSTEM_TEAMPROJECT="$SYSTEM_TEAMPROJECT" CC=$CC MAKEFLAGS=-j3 bash -lxc ci/run-linux32-docker.sh || exit 1
+
+       sudo chmod a+r t/out/TEST-*.xml
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-linux32-docker.sh'
+  - task: PublishTestResults@2
+    displayName: 'Publish Test Results **/TEST-*.xml'
+    inputs:
+      mergeTestResults: true
+      testRunTitle: 'linux32'
+      platform: Linux
+      publishRunAttachments: false
+    condition: succeededOrFailed()
+
+- phase: static_analysis
+  displayName: StaticAnalysis
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get install -y coccinelle
+
+       export jobname=StaticAnalysis
+
+       ci/run-static-analysis.sh || exit 1
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/run-static-analysis.sh'
+
+- phase: documentation
+  displayName: Documentation
+  condition: succeeded()
+  queue:
+    name: Hosted Ubuntu 1604
+  steps:
+  - bash: |
+       ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/vsts-cache gitfileshare "$(gitfileshare.pwd)" "$HOME/vsts-cache" || exit 1
+
+       sudo apt-get update
+       sudo apt-get install -y asciidoc xmlto asciidoctor
+
+       export ALREADY_HAVE_ASCIIDOCTOR=yes.
+       export jobname=Documentation
+
+       ci/test-documentation.sh || exit 1
+
+       sudo umount "$HOME/vsts-cache" || exit 1
+    displayName: 'ci/test-documentation.sh'
diff --git a/ci/mount-fileshare.sh b/ci/mount-fileshare.sh
new file mode 100755
index 0000000000..5fb5f74b70
--- /dev/null
+++ b/ci/mount-fileshare.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+die () {
+	echo "$*" >&2
+	exit 1
+}
+
+test $# = 4 ||
+die "Usage: $0 <share> <username> <password> <mountpoint"
+
+mkdir -p "$4" || die "Could not create $4"
+
+case "$(uname -s)" in
+Linux)
+	sudo mount -t cifs -o vers=3.0,username="$2",password="$3",dir_mode=0777,file_mode=0777,serverino "$1" "$4"
+	;;
+Darwin)
+	pass="$(echo "$3" | sed -e 's/\//%2F/g' -e 's/+/%2B/g')" &&
+	mount -t smbfs,soft "smb://$2:$pass@${1#//}" "$4"
+	;;
+*)
+	die "No support for $(uname -s)"
+	;;
+esac ||
+die "Could not mount $4"
+
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (5 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 6/9] Add a build definition for VSTS Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-04  4:30   ` Eric Sunshine
  2018-09-03 21:10 ` [PATCH 8/9] tests: record more stderr with --write-junit-xml in case of failure Johannes Schindelin via GitGitGadget
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The JUnit XML format lends itself to be presented in a powerful UI,
where you can drill down to the information you are interested in very
quickly.

For test failures, this usually means that you want to see the detailed
trace of the failing tests.

With Travis CI, we passed the `--verbose-log` option to get those
traces. However, that seems excessive, as we do not need/use the logs in
almost all of those cases: only when a test fails do we have a way to
include the trace.

So let's do something different in VSTS: let's run all the tests with
`--quiet` first, and only if a failure is encountered, try to trace the
commands as they are executed.

Of course, we cannot turn on `--verbose-log` after the fact. So let's
just re-run the test with all the same options, adding `--verbose-log`.
And then munging the output file into the JUnit XML on the fly.

Note: there is an off chance that re-running the test in verbose mode
"fixes" the failures (and this does happen from time to time!). That is
a possibility we should be able to live with. Ideally, we would label
this as "Passed upon rerun", and that outcome even exists on VSTS, but
it is not available when using the JUnit XML format for now:
https://github.com/Microsoft/vsts-agent/blob/master/src/Agent.Worker/TestResults/JunitResultReader.cs

This patch contains a slightly inelegant workaround for the p4 and
git-daemon tests: when we try to re-run the p4/git-daemon tests after
the daemon has been started already, we need to kill said daemon. We do
this by detecting the presence of the `kill_p4d` and `stop_git_daemon`
shell functions and calling them if available.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 50a65a600e..ea4ed250cc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -85,6 +85,13 @@ done,*)
 	test "$(cat "$BASE.exit")" = 0
 	exit
 	;;
+*' --write-junit-xml '*)
+	# record how to call this script *with* --verbose-log, in case
+	# we encounter a breakage
+	junit_rerun_options_sq="$(printf '%s\n' "$0" --verbose-log -x "$@" |
+		sed -e "s/'/'\\\\''/g" -e "s/^/'/" -e "s/\$/'/" |
+		tr '\012' ' ')"
+	;;
 esac
 
 # For repeatability, reset the environment to known value.
@@ -445,10 +452,37 @@ test_ok_ () {
 test_failure_ () {
 	if test -n "$write_junit_xml"
 	then
+		if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
+		then
+			case "$(type kill_p4d 2>/dev/null | head -n 1)" in
+			*function*) kill_p4d;;
+			esac
+
+			case "$(type stop_git_daemon 2>/dev/null |
+				head -n 1)" in
+			*function*) stop_git_daemon;;
+			esac
+
+			# re-run with --verbose-log
+			echo "# Re-running: $junit_rerun_options_sq" >&2
+
+			cd "$TEST_DIRECTORY" &&
+			eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
+				>/dev/null 2>&1
+			status=$?
+
+			say_color "" "$(test 0 = $status ||
+				echo "not ")ok $test_count - (re-ran with trace)"
+			say "1..$test_count"
+			GIT_EXIT_OK=t
+			exit $status
+		fi
+
 		junit_insert="<failure message=\"not ok $test_count -"
 		junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
 		junit_insert="$junit_insert $(xml_attr_encode \
-			"$(printf '%s\n' "$@" | sed 1d)")"
+			"$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
+		>"$GIT_TEST_TEE_OUTPUT_FILE"
 		junit_insert="$junit_insert</failure>"
 		write_junit_xml_testcase "$1" "      $junit_insert"
 	fi
@@ -733,6 +767,10 @@ test_start_ () {
 	if test -n "$write_junit_xml"
 	then
 		junit_start=$(test-tool date getnanos)
+
+		# truncate output
+		test -z "$GIT_TEST_TEE_OUTPUT_FILE" ||
+		>"$GIT_TEST_TEE_OUTPUT_FILE"
 	fi
 }
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 8/9] tests: record more stderr with --write-junit-xml in case of failure
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (6 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-03 21:10 ` [PATCH 9/9] README: add a build badge (status of the VSTS build) Johannes Schindelin via GitGitGadget
  2018-09-05 19:01 ` [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Sebastian Schuberth
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Sometimes, failures in a test case are actually caused by issues in
earlier test cases.

To make it easier to see those issues, let's attach the output from
before the failing test case (i.e. stdout/stderr since the previous
failing test case, or the start of the test script). This will be
visible in the "Attachments" of the details of the failed test.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ea4ed250cc..6fc03d5a3b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -484,6 +484,9 @@ test_failure_ () {
 			"$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
 		>"$GIT_TEST_TEE_OUTPUT_FILE"
 		junit_insert="$junit_insert</failure>"
+		junit_insert="$junit_insert<system-err>$(xml_attr_encode \
+			"$(cat "$GIT_TEST_TEE_OUTPUT_FILE.err")")</system-err>"
+		>"$GIT_TEST_TEE_OUTPUT_FILE.err"
 		write_junit_xml_testcase "$1" "      $junit_insert"
 	fi
 	test_failure=$(($test_failure + 1))
@@ -768,9 +771,12 @@ test_start_ () {
 	then
 		junit_start=$(test-tool date getnanos)
 
-		# truncate output
-		test -z "$GIT_TEST_TEE_OUTPUT_FILE" ||
-		>"$GIT_TEST_TEE_OUTPUT_FILE"
+		# append to future <system-err>; truncate output
+		test -z "$GIT_TEST_TEE_OUTPUT_FILE" || {
+			cat "$GIT_TEST_TEE_OUTPUT_FILE" \
+				>>"$GIT_TEST_TEE_OUTPUT_FILE.err"
+			>"$GIT_TEST_TEE_OUTPUT_FILE"
+		}
 	fi
 }
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 9/9] README: add a build badge (status of the VSTS build)
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (7 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 8/9] tests: record more stderr with --write-junit-xml in case of failure Johannes Schindelin via GitGitGadget
@ 2018-09-03 21:10 ` Johannes Schindelin via GitGitGadget
  2018-09-05 19:01 ` [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Sebastian Schuberth
  9 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-09-03 21:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Just like so many other OSS projects, we now also have a build badge.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 README.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/README.md b/README.md
index f920a42fad..f260e78042 100644
--- a/README.md
+++ b/README.md
@@ -1,3 +1,5 @@
+[![Build Status](https://git.visualstudio.com/git/_apis/build/status/test-git.git)](https://git.visualstudio.com/git/_build/latest?definitionId=2)
+
 Git - fast, scalable, distributed revision control system
 =========================================================
 
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things
  2018-09-03 21:10 ` [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things Johannes Schindelin via GitGitGadget
@ 2018-09-03 23:43   ` Eric Sunshine
  2018-09-04 11:04     ` Johannes Schindelin
  2018-09-05 18:57   ` Sebastian Schuberth
  1 sibling, 1 reply; 28+ messages in thread
From: Eric Sunshine @ 2018-09-03 23:43 UTC (permalink / raw)
  To: gitgitgadget; +Cc: Git List, Junio C Hamano, Johannes Schindelin

On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The upcoming patches will allow building git.git via VSTS CI, where
> variable names and URLs look a bit different than in Travis CI.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> @@ -28,7 +28,8 @@ osx-clang|osx-gcc)
>         # Uncomment this if you want to run perf tests:
>         # brew install gnu-time
> -       brew install git-lfs gettext
> +       test -z "$BREW_INSTALL_PACKAGES" ||
> +       eval brew install $BREW_INSTALL_PACKAGES

This 'eval' is unnecessary, isn't it?

    brew install $BREW_INSTALL_PACKAGES

should give the same result.

>         brew link --force gettext
>         brew install caskroom/cask/perforce
> diff --git a/ci/lib.sh b/ci/lib.sh
> @@ -1,5 +1,26 @@
> +       BREW_INSTALL_PACKAGES="git-lfs gettext"

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 4/9] tests: optionally write results as JUnit-style .xml
  2018-09-03 21:10 ` [PATCH 4/9] tests: optionally write results as JUnit-style .xml Johannes Schindelin via GitGitGadget
@ 2018-09-04  0:43   ` Eric Sunshine
  2018-09-04 10:59     ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Sunshine @ 2018-09-04  0:43 UTC (permalink / raw)
  To: gitgitgadget; +Cc: Git List, Junio C Hamano, Johannes Schindelin

On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> This will come in handy when publishing the results of Git's test suite
> during an automated VSTS CI run.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> @@ -431,11 +434,24 @@ trap 'exit $?' INT
>  test_failure_ () {
> +       if test -n "$write_junit_xml"
> +       then
> +               junit_insert="<failure message=\"not ok $test_count -"
> +               junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> +               junit_insert="$junit_insert $(xml_attr_encode \
> +                       "$(printf '%s\n' "$@" | sed 1d)")"
> +               junit_insert="$junit_insert</failure>"

This is a genuine failure, so you're creating a <failure> node. Okay.

> +               write_junit_xml_testcase "$1" "      $junit_insert"
> +       fi
> @@ -444,11 +460,19 @@ test_failure_ () {
>  test_known_broken_ok_ () {
> +       if test -n "$write_junit_xml"
> +       then
> +               write_junit_xml_testcase "$* (breakage fixed)"
> +       fi
>         test_fixed=$(($test_fixed+1))
>         say_color error "ok $test_count - $@ # TODO known breakage vanished"
>  }

This was expected to fail but didn't, which means it probably needs
some sort of attention. test_known_broken_ok_() prints this result in
the 'error' color, and test_done() re-inforces that by printing a
message, also in 'error' color:

    42 known breakage(s) vanished; please update test(s)

So, should this emit a <failure> node also, perhaps with 'type'
attribute set to "warning" or something? (<failure type="WARNING"
message="...">)

> @@ -758,9 +793,58 @@ test_at_end_hook_ () {
> +xml_attr_encode () {
> +       # We do not translate CR to &#x0d; because BSD sed does not handle
> +       # \r in the regex. In practice, the output should not even have any
> +       # carriage returns.
> +       printf '%s\n' "$@" |
> +       sed -e 's/&/\&amp;/g' -e "s/'/\&apos;/g" -e 's/"/\&quot;/g' \
> +               -e 's/</\&lt;/g' -e 's/>/\&gt;/g' \
> +               -e 's/  /\&#x09;/g' -e 's/$/\&#x0a;/' -e '$s/&#x0a;$//' |
> +       tr -d '\012\015'
> +}

It's possible to insert a literal CR in the 'sed' expression, which
does match correctly on BSD (and MacOS). For instance:

    CR=$(printf "\r")
    sed -e "s/$CR/\&#x0d;/g"

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-03 21:10 ` [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure Johannes Schindelin via GitGitGadget
@ 2018-09-04  4:30   ` Eric Sunshine
  2018-09-04 11:09     ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Sunshine @ 2018-09-04  4:30 UTC (permalink / raw)
  To: gitgitgadget; +Cc: Git List, Junio C Hamano, Johannes Schindelin

On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> So let's do something different in VSTS: let's run all the tests with
> `--quiet` first, and only if a failure is encountered, try to trace the
> commands as they are executed. [...]
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> @@ -445,10 +452,37 @@ test_ok_ () {
>  test_failure_ () {
>         if test -n "$write_junit_xml"
>         then
> +               if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
> +               then
> +                       case "$(type kill_p4d 2>/dev/null | head -n 1)" in
> +                       *function*) kill_p4d;;
> +                       esac
> +
> +                       case "$(type stop_git_daemon 2>/dev/null |
> +                               head -n 1)" in
> +                       *function*) stop_git_daemon;;
> +                       esac

In the long run, it might make more sense, and be more scalable, to
have those scripts define a "prepare_for_rerun" variable or function
which this code then runs generically rather than having special
knowledge of those facilities.

I could imagine, for instance, test-lib.sh defining a no-op:

    test_failure_prepare_rerun () {}

and then each of those scripts overriding the function:

    # in lib-git-p4.sh
    test_failure_prepare_rerun () {
        kill_p4d
    }

    # in lib-git-daemon.sh
    test_failure_prepare_rerun () {
        stop_git_daemon
    }

> +                       # re-run with --verbose-log
> +                       echo "# Re-running: $junit_rerun_options_sq" >&2
> +
> +                       cd "$TEST_DIRECTORY" &&
> +                       eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
> +                               >/dev/null 2>&1
> +                       status=$?
> +
> +                       say_color "" "$(test 0 = $status ||
> +                               echo "not ")ok $test_count - (re-ran with trace)"
> +                       say "1..$test_count"
> +                       GIT_EXIT_OK=t
> +                       exit $status
> +               fi
> +
>                 junit_insert="<failure message=\"not ok $test_count -"
>                 junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
>                 junit_insert="$junit_insert $(xml_attr_encode \
> -                       "$(printf '%s\n' "$@" | sed 1d)")"
> +                       "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
> +               >"$GIT_TEST_TEE_OUTPUT_FILE"
>                 junit_insert="$junit_insert</failure>"
>                 write_junit_xml_testcase "$1" "      $junit_insert"
>         fi

This junit-related stuff is getting pretty lengthy. I wonder if it
would make sense to pull it out to its own function at some point
(again, in the long run).

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 4/9] tests: optionally write results as JUnit-style .xml
  2018-09-04  0:43   ` Eric Sunshine
@ 2018-09-04 10:59     ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-04 10:59 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: gitgitgadget, Git List, Junio C Hamano

Hi Eric,

On Mon, 3 Sep 2018, Eric Sunshine wrote:

> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > This will come in handy when publishing the results of Git's test suite
> > during an automated VSTS CI run.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > @@ -431,11 +434,24 @@ trap 'exit $?' INT
> >  test_failure_ () {
> > +       if test -n "$write_junit_xml"
> > +       then
> > +               junit_insert="<failure message=\"not ok $test_count -"
> > +               junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> > +               junit_insert="$junit_insert $(xml_attr_encode \
> > +                       "$(printf '%s\n' "$@" | sed 1d)")"
> > +               junit_insert="$junit_insert</failure>"
> 
> This is a genuine failure, so you're creating a <failure> node. Okay.
> 
> > +               write_junit_xml_testcase "$1" "      $junit_insert"
> > +       fi
> > @@ -444,11 +460,19 @@ test_failure_ () {
> >  test_known_broken_ok_ () {
> > +       if test -n "$write_junit_xml"
> > +       then
> > +               write_junit_xml_testcase "$* (breakage fixed)"
> > +       fi
> >         test_fixed=$(($test_fixed+1))
> >         say_color error "ok $test_count - $@ # TODO known breakage vanished"
> >  }
> 
> This was expected to fail but didn't, which means it probably needs
> some sort of attention. test_known_broken_ok_() prints this result in
> the 'error' color, and test_done() re-inforces that by printing a
> message, also in 'error' color:
> 
>     42 known breakage(s) vanished; please update test(s)
> 
> So, should this emit a <failure> node also, perhaps with 'type'
> attribute set to "warning" or something? (<failure type="WARNING"
> message="...">)

My primary aim is to display the test results in the web interface, see
e.g.
https://git.visualstudio.com/git/_build/results?buildId=128&view=ms.vss-test-web.test-result-details

The parser for JUnit XML (and in fact, the JUnit XML schema itself) do not
allow for such a warning. If you add a `<failure>`, then the build fails.

And we do not want the build to fail. Historically, I saw quite a couple
of "vanished" breakages depending on the platform where I ran the tests.

> > @@ -758,9 +793,58 @@ test_at_end_hook_ () {
> > +xml_attr_encode () {
> > +       # We do not translate CR to &#x0d; because BSD sed does not handle
> > +       # \r in the regex. In practice, the output should not even have any
> > +       # carriage returns.
> > +       printf '%s\n' "$@" |
> > +       sed -e 's/&/\&amp;/g' -e "s/'/\&apos;/g" -e 's/"/\&quot;/g' \
> > +               -e 's/</\&lt;/g' -e 's/>/\&gt;/g' \
> > +               -e 's/  /\&#x09;/g' -e 's/$/\&#x0a;/' -e '$s/&#x0a;$//' |
> > +       tr -d '\012\015'
> > +}
> 
> It's possible to insert a literal CR in the 'sed' expression, which
> does match correctly on BSD (and MacOS). For instance:
> 
>     CR=$(printf "\r")
>     sed -e "s/$CR/\&#x0d;/g"

Okay. But since we are talking about displaying some chunk of text, I
would rather just delete the CR here anyway.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things
  2018-09-03 23:43   ` Eric Sunshine
@ 2018-09-04 11:04     ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-04 11:04 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: gitgitgadget, Git List, Junio C Hamano

Hi Eric,

On Mon, 3 Sep 2018, Eric Sunshine wrote:

> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > The upcoming patches will allow building git.git via VSTS CI, where
> > variable names and URLs look a bit different than in Travis CI.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > @@ -28,7 +28,8 @@ osx-clang|osx-gcc)
> >         # Uncomment this if you want to run perf tests:
> >         # brew install gnu-time
> > -       brew install git-lfs gettext
> > +       test -z "$BREW_INSTALL_PACKAGES" ||
> > +       eval brew install $BREW_INSTALL_PACKAGES
> 
> This 'eval' is unnecessary, isn't it?
> 
>     brew install $BREW_INSTALL_PACKAGES
> 
> should give the same result.

Oh right! Fixed in https://github.com/gitgitgadget/git/pull/31 (and I also
opened https://github.com/git/git/pull/531 to verify that Travis CI still
works).

Thanks,
Dscho

> >         brew link --force gettext
> >         brew install caskroom/cask/perforce
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > @@ -1,5 +1,26 @@
> > +       BREW_INSTALL_PACKAGES="git-lfs gettext"
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-04  4:30   ` Eric Sunshine
@ 2018-09-04 11:09     ` Johannes Schindelin
  2018-09-05  5:32       ` Luke Diamand
  0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-04 11:09 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: gitgitgadget, Git List, Junio C Hamano

Hi Eric,

On Tue, 4 Sep 2018, Eric Sunshine wrote:

> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > So let's do something different in VSTS: let's run all the tests with
> > `--quiet` first, and only if a failure is encountered, try to trace the
> > commands as they are executed. [...]
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > @@ -445,10 +452,37 @@ test_ok_ () {
> >  test_failure_ () {
> >         if test -n "$write_junit_xml"
> >         then
> > +               if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
> > +               then
> > +                       case "$(type kill_p4d 2>/dev/null | head -n 1)" in
> > +                       *function*) kill_p4d;;
> > +                       esac
> > +
> > +                       case "$(type stop_git_daemon 2>/dev/null |
> > +                               head -n 1)" in
> > +                       *function*) stop_git_daemon;;
> > +                       esac
> 
> In the long run, it might make more sense, and be more scalable, to
> have those scripts define a "prepare_for_rerun" variable or function
> which this code then runs generically rather than having special
> knowledge of those facilities.
> 
> I could imagine, for instance, test-lib.sh defining a no-op:
> 
>     test_failure_prepare_rerun () {}
> 
> and then each of those scripts overriding the function:
> 
>     # in lib-git-p4.sh
>     test_failure_prepare_rerun () {
>         kill_p4d
>     }
> 
>     # in lib-git-daemon.sh
>     test_failure_prepare_rerun () {
>         stop_git_daemon
>     }

Or we could implement `test_atexit` (similar to `test_when_finished`, but
to be executed at `test_done` time). I guess that's what the p4 and daemon
tests really needed to begin with (and probably also the apache2-using
tests).

> 
> > +                       # re-run with --verbose-log
> > +                       echo "# Re-running: $junit_rerun_options_sq" >&2
> > +
> > +                       cd "$TEST_DIRECTORY" &&
> > +                       eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
> > +                               >/dev/null 2>&1
> > +                       status=$?
> > +
> > +                       say_color "" "$(test 0 = $status ||
> > +                               echo "not ")ok $test_count - (re-ran with trace)"
> > +                       say "1..$test_count"
> > +                       GIT_EXIT_OK=t
> > +                       exit $status
> > +               fi
> > +
> >                 junit_insert="<failure message=\"not ok $test_count -"
> >                 junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> >                 junit_insert="$junit_insert $(xml_attr_encode \
> > -                       "$(printf '%s\n' "$@" | sed 1d)")"
> > +                       "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
> > +               >"$GIT_TEST_TEE_OUTPUT_FILE"
> >                 junit_insert="$junit_insert</failure>"
> >                 write_junit_xml_testcase "$1" "      $junit_insert"
> >         fi
> 
> This junit-related stuff is getting pretty lengthy. I wonder if it
> would make sense to pull it out to its own function at some point
> (again, in the long run).

Now that you mention it... I agree. This is getting long.

In the short run, I have two things to consider, though: I want to make
this work first, then think about introducing a layer of abstraction, and
I want to go on vacation tomorrow.

So I agree that this is something to be considered in the long run, i.e.
not right now ;-)

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-04 11:09     ` Johannes Schindelin
@ 2018-09-05  5:32       ` Luke Diamand
  2018-09-05 12:39         ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Luke Diamand @ 2018-09-05  5:32 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Eric Sunshine, gitgitgadget, Git List, Junio C Hamano

On 4 September 2018 at 12:09, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Eric,
>
> On Tue, 4 Sep 2018, Eric Sunshine wrote:
>
>> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>> > So let's do something different in VSTS: let's run all the tests with
>> > `--quiet` first, and only if a failure is encountered, try to trace the
>> > commands as they are executed. [...]

Is this re-running just an individual test on its own or all the tests
within a single file?

The latter shouldn't need this at all. And the former, I'm not sure
will actually work - most of the tests assume some particular p4
state. But perhaps I'm missing something?

I also think it does look kind of ugly. And if there's one thing I've
learned, it's that the ugly hack you write today with the words "we'll
tidy this up later" goes on to live with you forever!

>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > @@ -445,10 +452,37 @@ test_ok_ () {
>> >  test_failure_ () {
>> >         if test -n "$write_junit_xml"
>> >         then
>> > +               if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
>> > +               then
>> > +                       case "$(type kill_p4d 2>/dev/null | head -n 1)" in
>> > +                       *function*) kill_p4d;;
>> > +                       esac
>> > +
>> > +                       case "$(type stop_git_daemon 2>/dev/null |
>> > +                               head -n 1)" in
>> > +                       *function*) stop_git_daemon;;
>> > +                       esac
>>
>> In the long run, it might make more sense, and be more scalable, to
>> have those scripts define a "prepare_for_rerun" variable or function
>> which this code then runs generically rather than having special
>> knowledge of those facilities.
>>
>> I could imagine, for instance, test-lib.sh defining a no-op:
>>
>>     test_failure_prepare_rerun () {}
>>
>> and then each of those scripts overriding the function:
>>
>>     # in lib-git-p4.sh
>>     test_failure_prepare_rerun () {
>>         kill_p4d
>>     }
>>
>>     # in lib-git-daemon.sh
>>     test_failure_prepare_rerun () {
>>         stop_git_daemon
>>     }
>
> Or we could implement `test_atexit` (similar to `test_when_finished`, but
> to be executed at `test_done` time). I guess that's what the p4 and daemon
> tests really needed to begin with (and probably also the apache2-using
> tests).
>
>>
>> > +                       # re-run with --verbose-log
>> > +                       echo "# Re-running: $junit_rerun_options_sq" >&2
>> > +
>> > +                       cd "$TEST_DIRECTORY" &&
>> > +                       eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
>> > +                               >/dev/null 2>&1
>> > +                       status=$?
>> > +
>> > +                       say_color "" "$(test 0 = $status ||
>> > +                               echo "not ")ok $test_count - (re-ran with trace)"
>> > +                       say "1..$test_count"
>> > +                       GIT_EXIT_OK=t
>> > +                       exit $status
>> > +               fi
>> > +
>> >                 junit_insert="<failure message=\"not ok $test_count -"
>> >                 junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
>> >                 junit_insert="$junit_insert $(xml_attr_encode \
>> > -                       "$(printf '%s\n' "$@" | sed 1d)")"
>> > +                       "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
>> > +               >"$GIT_TEST_TEE_OUTPUT_FILE"
>> >                 junit_insert="$junit_insert</failure>"
>> >                 write_junit_xml_testcase "$1" "      $junit_insert"
>> >         fi
>>
>> This junit-related stuff is getting pretty lengthy. I wonder if it
>> would make sense to pull it out to its own function at some point
>> (again, in the long run).
>
> Now that you mention it... I agree. This is getting long.
>
> In the short run, I have two things to consider, though: I want to make
> this work first, then think about introducing a layer of abstraction, and
> I want to go on vacation tomorrow.
>
> So I agree that this is something to be considered in the long run, i.e.
> not right now ;-)
>
> Thanks,
> Dscho

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05  5:32       ` Luke Diamand
@ 2018-09-05 12:39         ` Johannes Schindelin
  2018-09-05 13:03           ` Luke Diamand
  2018-09-05 18:38           ` Eric Sunshine
  0 siblings, 2 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-05 12:39 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Eric Sunshine, gitgitgadget, Git List, Junio C Hamano

Hi Luke,

On Wed, 5 Sep 2018, Luke Diamand wrote:

> On 4 September 2018 at 12:09, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 4 Sep 2018, Eric Sunshine wrote:
> >
> >> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> >> <gitgitgadget@gmail.com> wrote:
> >> > So let's do something different in VSTS: let's run all the tests with
> >> > `--quiet` first, and only if a failure is encountered, try to trace the
> >> > commands as they are executed. [...]
> 
> Is this re-running just an individual test on its own or all the tests
> within a single file?

Upon encountering a failed test, it is re-running the entire test script
afresh.

> The latter shouldn't need this at all.

Please do not let me die dumb. In other words, I would love for you to
explain what exactly you mean by that sentence.

> And the former, I'm not sure will actually work - most of the tests
> assume some particular p4 state. But perhaps I'm missing something?

No, the former would not work at all. Not only for the p4 tests: Git's
tests frequently commit the deadly sin of relying on output of one another
(wreaking havoc e.g. when test cases are skipped due to missing
prerequisites, and latter test cases relying on their output). It is not
the only thing that is wrong with the test suite, of course.

> I also think it does look kind of ugly. And if there's one thing I've
> learned, it's that the ugly hack you write today with the words "we'll
> tidy this up later" goes on to live with you forever!

Okay.

(And having read lib-git-p4.sh, I kind of see where you learned that.)

But maybe you also have some splendid idea what to do instead? Because
doing something about it, that we need. We can't just say "oh, the only
solution we found is ugly, so let's not do it at all".

I am even going so far as to say: unless you have a better idea, it is
pretty detrimental to criticize the current approach. It is the opposite
of constructive.

So let's hear some ideas how to improve the situation, m'kay?

Just as a reminder, this is the problem I want to solve: I want to run the
tests in a light-weight manner, with minimal output, and only in case of
an error do I want to crank up the verbosity. Instead of wasting most of the
effort to log everything and then throwing it away in most of the common
cases, I suggest to re-run the entire test.

What do you suggest to solve this?

Ciao,
Johannes

> >> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> > ---
> >> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> > @@ -445,10 +452,37 @@ test_ok_ () {
> >> >  test_failure_ () {
> >> >         if test -n "$write_junit_xml"
> >> >         then
> >> > +               if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
> >> > +               then
> >> > +                       case "$(type kill_p4d 2>/dev/null | head -n 1)" in
> >> > +                       *function*) kill_p4d;;
> >> > +                       esac
> >> > +
> >> > +                       case "$(type stop_git_daemon 2>/dev/null |
> >> > +                               head -n 1)" in
> >> > +                       *function*) stop_git_daemon;;
> >> > +                       esac
> >>
> >> In the long run, it might make more sense, and be more scalable, to
> >> have those scripts define a "prepare_for_rerun" variable or function
> >> which this code then runs generically rather than having special
> >> knowledge of those facilities.
> >>
> >> I could imagine, for instance, test-lib.sh defining a no-op:
> >>
> >>     test_failure_prepare_rerun () {}
> >>
> >> and then each of those scripts overriding the function:
> >>
> >>     # in lib-git-p4.sh
> >>     test_failure_prepare_rerun () {
> >>         kill_p4d
> >>     }
> >>
> >>     # in lib-git-daemon.sh
> >>     test_failure_prepare_rerun () {
> >>         stop_git_daemon
> >>     }
> >
> > Or we could implement `test_atexit` (similar to `test_when_finished`, but
> > to be executed at `test_done` time). I guess that's what the p4 and daemon
> > tests really needed to begin with (and probably also the apache2-using
> > tests).
> >
> >>
> >> > +                       # re-run with --verbose-log
> >> > +                       echo "# Re-running: $junit_rerun_options_sq" >&2
> >> > +
> >> > +                       cd "$TEST_DIRECTORY" &&
> >> > +                       eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
> >> > +                               >/dev/null 2>&1
> >> > +                       status=$?
> >> > +
> >> > +                       say_color "" "$(test 0 = $status ||
> >> > +                               echo "not ")ok $test_count - (re-ran with trace)"
> >> > +                       say "1..$test_count"
> >> > +                       GIT_EXIT_OK=t
> >> > +                       exit $status
> >> > +               fi
> >> > +
> >> >                 junit_insert="<failure message=\"not ok $test_count -"
> >> >                 junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> >> >                 junit_insert="$junit_insert $(xml_attr_encode \
> >> > -                       "$(printf '%s\n' "$@" | sed 1d)")"
> >> > +                       "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
> >> > +               >"$GIT_TEST_TEE_OUTPUT_FILE"
> >> >                 junit_insert="$junit_insert</failure>"
> >> >                 write_junit_xml_testcase "$1" "      $junit_insert"
> >> >         fi
> >>
> >> This junit-related stuff is getting pretty lengthy. I wonder if it
> >> would make sense to pull it out to its own function at some point
> >> (again, in the long run).
> >
> > Now that you mention it... I agree. This is getting long.
> >
> > In the short run, I have two things to consider, though: I want to make
> > this work first, then think about introducing a layer of abstraction, and
> > I want to go on vacation tomorrow.
> >
> > So I agree that this is something to be considered in the long run, i.e.
> > not right now ;-)
> >
> > Thanks,
> > Dscho
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 12:39         ` Johannes Schindelin
@ 2018-09-05 13:03           ` Luke Diamand
  2018-09-14 18:46             ` Johannes Schindelin
  2018-09-05 18:38           ` Eric Sunshine
  1 sibling, 1 reply; 28+ messages in thread
From: Luke Diamand @ 2018-09-05 13:03 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Eric Sunshine, gitgitgadget, Git List, Junio C Hamano

On 5 September 2018 at 13:39, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Luke,
>
> On Wed, 5 Sep 2018, Luke Diamand wrote:
>
>> On 4 September 2018 at 12:09, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > On Tue, 4 Sep 2018, Eric Sunshine wrote:
>> >
>> >> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
>> >> <gitgitgadget@gmail.com> wrote:
>> >> > So let's do something different in VSTS: let's run all the tests with
>> >> > `--quiet` first, and only if a failure is encountered, try to trace the
>> >> > commands as they are executed. [...]
>>
>> Is this re-running just an individual test on its own or all the tests
>> within a single file?
>
> Upon encountering a failed test, it is re-running the entire test script
> afresh.
>
>> The latter shouldn't need this at all.
>
> Please do not let me die dumb. In other words, I would love for you to
> explain what exactly you mean by that sentence.

Just re-run the script. You shouldn't need to kill p4d, as each script
starts up its own instance of p4d, and shuts it down when it exits.

$ cd t
$ ./t9800-git-p4-basic.sh
Ctrl^C
$ ./t9800-git-p4-basic.sh -v

There's a cleanup() function in lib-git-p4.sh which kills the p4d
server, and that's invoked via:

  trap cleanup EXIT

That's the only cleanup that each of the scripts require AFAIK.

>
>> And the former, I'm not sure will actually work - most of the tests
>> assume some particular p4 state. But perhaps I'm missing something?
>
> No, the former would not work at all. Not only for the p4 tests: Git's
> tests frequently commit the deadly sin of relying on output of one another
> (wreaking havoc e.g. when test cases are skipped due to missing
> prerequisites, and latter test cases relying on their output). It is not
> the only thing that is wrong with the test suite, of course.
>
>> I also think it does look kind of ugly. And if there's one thing I've
>> learned, it's that the ugly hack you write today with the words "we'll
>> tidy this up later" goes on to live with you forever!
>
> Okay.
>
> (And having read lib-git-p4.sh, I kind of see where you learned that.)
>
> But maybe you also have some splendid idea what to do instead? Because
> doing something about it, that we need. We can't just say "oh, the only
> solution we found is ugly, so let's not do it at all".
>
> I am even going so far as to say: unless you have a better idea, it is
> pretty detrimental to criticize the current approach. It is the opposite
> of constructive.
>
> So let's hear some ideas how to improve the situation, m'kay?
>
> Just as a reminder, this is the problem I want to solve: I want to run the
> tests in a light-weight manner, with minimal output, and only in case of
> an error do I want to crank up the verbosity. Instead of wasting most of the
> effort to log everything and then throwing it away in most of the common
> cases, I suggest to re-run the entire test.
>
> What do you suggest to solve this?
>

I don't know about any other tests, but the git-p4 tests don't take
any longer in verbose mode. So one simple solution is to just run it
in verbose mode - unless there are other tests which have more
overhead.

The trap/exit/cleanup method that the git-p4 tests already use would
seem to be ideally suited to cleaning up everything on exit.

There might be some specific tests where this doesn't quite work out,
if you let me know what they are I can have a look at fixing them for
you.

Thanks,
Luke

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 12:39         ` Johannes Schindelin
  2018-09-05 13:03           ` Luke Diamand
@ 2018-09-05 18:38           ` Eric Sunshine
  2018-09-05 20:24             ` Jeff King
  2018-09-14 18:51             ` Johannes Schindelin
  1 sibling, 2 replies; 28+ messages in thread
From: Eric Sunshine @ 2018-09-05 18:38 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Luke Diamand, gitgitgadget, Git List, Junio C Hamano, Jeff King,
	Jonathan Nieder

On Wed, Sep 5, 2018 at 8:39 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> So let's hear some ideas how to improve the situation, m'kay?
> Just as a reminder, this is the problem I want to solve: I want to run the
> tests in a light-weight manner, with minimal output, and only in case of
> an error do I want to crank up the verbosity. Instead of wasting most of the
> effort to log everything and then throwing it away in most of the common
> cases, I suggest to re-run the entire test.

What about the very different approach of capturing the full "verbose"
output the executed tests in addition to whatever is actually output
to the terminal? If a test fails, then (and only then) you can insert
the captured verbose output into the JUnit XML file. This way (if we
always have the full verbose output at hand), you don't need to re-run
the test at all.

I've cc:'d Peff and Jonathan since I believe they are more familiar
with how all the capturing / output-redirection works in the test
suite.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things
  2018-09-03 21:10 ` [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things Johannes Schindelin via GitGitGadget
  2018-09-03 23:43   ` Eric Sunshine
@ 2018-09-05 18:57   ` Sebastian Schuberth
  2018-09-14 19:07     ` Johannes Schindelin
  1 sibling, 1 reply; 28+ messages in thread
From: Sebastian Schuberth @ 2018-09-05 18:57 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

On 9/3/2018 11:10 PM, Johannes Schindelin via GitGitGadget wrote:

> +if test -n "$TRAVIS_COMMIT"
> +then
> +	# We are running within Travis CI

Personally, I'd find a check like

if test "$TRAVIS" = "true"

more speaking (also see [1]).

[1] https://docs.travis-ci.com/user/environment-variables/

-- 
Sebastian Schuberth


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services
  2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
                   ` (8 preceding siblings ...)
  2018-09-03 21:10 ` [PATCH 9/9] README: add a build badge (status of the VSTS build) Johannes Schindelin via GitGitGadget
@ 2018-09-05 19:01 ` Sebastian Schuberth
  2018-09-05 19:08   ` Stefan Beller
  9 siblings, 1 reply; 28+ messages in thread
From: Sebastian Schuberth @ 2018-09-05 19:01 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git; +Cc: Junio C Hamano

On 9/3/2018 11:10 PM, Johannes Schindelin via GitGitGadget wrote:

> The one sad part about this is the Windows support. Travis lacks it, and we
> work around that by using Visual Studio Team Services (VSTS) indirectly: one
> phase in Travis would trigger a build, wait for its log, and then paste that
> log.

I'm sorry if this has been discussed before, but as this recap doesn't 
mention it: Has AppVeyor been considered as an option? It seems to be 
the defacto standard for Windows CI for projects on GitHub.

-- 
Sebastian Schuberth

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services
  2018-09-05 19:01 ` [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Sebastian Schuberth
@ 2018-09-05 19:08   ` Stefan Beller
  0 siblings, 0 replies; 28+ messages in thread
From: Stefan Beller @ 2018-09-05 19:08 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: gitgitgadget, git, Junio C Hamano

On Wed, Sep 5, 2018 at 12:02 PM Sebastian Schuberth
<sschuberth@gmail.com> wrote:
>
> On 9/3/2018 11:10 PM, Johannes Schindelin via GitGitGadget wrote:
>
> > The one sad part about this is the Windows support. Travis lacks it, and we
> > work around that by using Visual Studio Team Services (VSTS) indirectly: one
> > phase in Travis would trigger a build, wait for its log, and then paste that
> > log.
>
> I'm sorry if this has been discussed before, but as this recap doesn't
> mention it: Has AppVeyor been considered as an option? It seems to be
> the defacto standard for Windows CI for projects on GitHub.

There was
https://public-inbox.org/git/BAY169-W30CD27F2F7606F4DF52944A78F0@phx.gbl/
and
https://public-inbox.org/git/alpine.DEB.2.20.1703241242210.17768@tvnag.unkk.fr/

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 18:38           ` Eric Sunshine
@ 2018-09-05 20:24             ` Jeff King
  2018-09-14 19:04               ` Johannes Schindelin
  2018-09-14 18:51             ` Johannes Schindelin
  1 sibling, 1 reply; 28+ messages in thread
From: Jeff King @ 2018-09-05 20:24 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Johannes Schindelin, Luke Diamand, gitgitgadget, Git List,
	Junio C Hamano, Jonathan Nieder

On Wed, Sep 05, 2018 at 02:38:34PM -0400, Eric Sunshine wrote:

> On Wed, Sep 5, 2018 at 8:39 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > So let's hear some ideas how to improve the situation, m'kay?
> > Just as a reminder, this is the problem I want to solve: I want to run the
> > tests in a light-weight manner, with minimal output, and only in case of
> > an error do I want to crank up the verbosity. Instead of wasting most of the
> > effort to log everything and then throwing it away in most of the common
> > cases, I suggest to re-run the entire test.
> 
> What about the very different approach of capturing the full "verbose"
> output the executed tests in addition to whatever is actually output
> to the terminal? If a test fails, then (and only then) you can insert
> the captured verbose output into the JUnit XML file. This way (if we
> always have the full verbose output at hand), you don't need to re-run
> the test at all.
> 
> I've cc:'d Peff and Jonathan since I believe they are more familiar
> with how all the capturing / output-redirection works in the test
> suite.

I don't think there's much to know beyond what you wrote. The
"--verbose" case does not really cost any more than the non-verbose one,
because the only difference is whether output goes to a file versus
/dev/null. But the commands all call write() regardless.

For --verbose-log, it does of course cost a little extra to run one
`tee` per script, and to write a few kb of logs. I doubt those are
measurable versus the rest of a script run, but I'm happy to be
disproven by numbers. There are some gymnastics done to re-exec the test
script with the same shell, but AFAIK those are robust and don't cost a
lot (again, one extra process per script run).

I'm not overly concerned about the cost of re-running a test, since the
idea is that failed tests should be rare. I would be a little worried
about flaky tests mysteriously righting themselves on the second run (so
you know a failure happened, but you have no good output to describe
it).

I do agree that a test_atexit() cleanup would make a lot more sense than
what's in the original patch. And that's nicer than the exit trap we're
using already, because you may note that each caller has to manually
restore the original 'die' handler that test-lib.sh installs.

That would also help with bitrot. If this funky cleanup case only causes
problems with junit output, then other people are likely to forget to do
it, and the mess falls onto the junit folks (i.e., Dscho). But if the
tests start _relying_ on test_atexit() being called (i.e., don't call
stop_git_daemon manually, but assume that the atexit handler does so),
then the responsible authors are a lot more likely to notice and fix it
early.

-Peff

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 13:03           ` Luke Diamand
@ 2018-09-14 18:46             ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-14 18:46 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Eric Sunshine, gitgitgadget, Git List, Junio C Hamano

Hi Luke,

On Wed, 5 Sep 2018, Luke Diamand wrote:

> On 5 September 2018 at 13:39, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 5 Sep 2018, Luke Diamand wrote:
> >
> >> On 4 September 2018 at 12:09, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >> >
> >> > On Tue, 4 Sep 2018, Eric Sunshine wrote:
> >> >
> >> >> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> >> >> <gitgitgadget@gmail.com> wrote:
> >> >> > So let's do something different in VSTS: let's run all the tests with
> >> >> > `--quiet` first, and only if a failure is encountered, try to trace the
> >> >> > commands as they are executed. [...]
> >>
> >> Is this re-running just an individual test on its own or all the tests
> >> within a single file?
> >
> > Upon encountering a failed test, it is re-running the entire test script
> > afresh.
> >
> >> The latter shouldn't need this at all.
> >
> > Please do not let me die dumb. In other words, I would love for you to
> > explain what exactly you mean by that sentence.
> 
> Just re-run the script. You shouldn't need to kill p4d, as each script
> starts up its own instance of p4d, and shuts it down when it exits.
> 
> $ cd t
> $ ./t9800-git-p4-basic.sh
> Ctrl^C

That Ctrl^C is not possible in the automated builds (which are the subject
of this patch series).

What *is* possible is to detect in the test script that there was a
breakage, and then re-run the test script.

Unfortunately, at this stage the first test script has not exited yet, so
your p4d has not exited yet, and we have to clean up in some way before
re-running (because the port in use is determined by the test script's
number, so we really need to kill the old daemon).

And this is exactly what my patches try to achieve here.

> $ ./t9800-git-p4-basic.sh -v
> 
> There's a cleanup() function in lib-git-p4.sh which kills the p4d
> server, and that's invoked via:
> 
>   trap cleanup EXIT
> 
> That's the only cleanup that each of the scripts require AFAIK.

Good.

However, we really will want to consider introducing something consistent
here, something that works for *all* test scripts. IOW if any test script
wants to start a process for the life-time of that script, and needs to
kill that process at the end, we will want to have a consistent,
documented way to register a function (or commands) to be called at the
end.

And that function (or commands) need to be run also when stopping
everything in preparation for a re-run.

> >> And the former, I'm not sure will actually work - most of the tests
> >> assume some particular p4 state. But perhaps I'm missing something?
> >
> > No, the former would not work at all. Not only for the p4 tests: Git's
> > tests frequently commit the deadly sin of relying on output of one another
> > (wreaking havoc e.g. when test cases are skipped due to missing
> > prerequisites, and latter test cases relying on their output). It is not
> > the only thing that is wrong with the test suite, of course.
> >
> >> I also think it does look kind of ugly. And if there's one thing I've
> >> learned, it's that the ugly hack you write today with the words "we'll
> >> tidy this up later" goes on to live with you forever!
> >
> > Okay.
> >
> > (And having read lib-git-p4.sh, I kind of see where you learned that.)
> >
> > But maybe you also have some splendid idea what to do instead? Because
> > doing something about it, that we need. We can't just say "oh, the only
> > solution we found is ugly, so let's not do it at all".
> >
> > I am even going so far as to say: unless you have a better idea, it is
> > pretty detrimental to criticize the current approach. It is the opposite
> > of constructive.
> >
> > So let's hear some ideas how to improve the situation, m'kay?
> >
> > Just as a reminder, this is the problem I want to solve: I want to run the
> > tests in a light-weight manner, with minimal output, and only in case of
> > an error do I want to crank up the verbosity. Instead of wasting most of the
> > effort to log everything and then throwing it away in most of the common
> > cases, I suggest to re-run the entire test.
> >
> > What do you suggest to solve this?
> >
> 
> I don't know about any other tests, but the git-p4 tests don't take
> any longer in verbose mode.

That statement is too general to be correct. It may be the case in your
setup, but I found that even a redirected, chatty stderr can slow down
things on Windows substantially, probably due to the POSIX emulation going
on in the background that needs to prepare for all kinds of eventualities
(that we do not even need, but the MSYS2 runtime has no way of knowing
that).

> So one simple solution is to just run it in verbose mode - unless there
> are other tests which have more overhead.

It is a bit of an overhead. As the entire test run on Windows takes
*quite* a while (due to said POSIX emulation issues), I was not able to
quantify it exactly.

> The trap/exit/cleanup method that the git-p4 tests already use would
> seem to be ideally suited to cleaning up everything on exit.

Yes. On exit. But I cannot exit here, not before re-running the script.

> There might be some specific tests where this doesn't quite work out,
> if you let me know what they are I can have a look at fixing them for
> you.

Thanks. I will let you know when I encounter other tests than the p4 and
git-daemon tests (which I fixed in this patch series).

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 18:38           ` Eric Sunshine
  2018-09-05 20:24             ` Jeff King
@ 2018-09-14 18:51             ` Johannes Schindelin
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-14 18:51 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Luke Diamand, gitgitgadget, Git List, Junio C Hamano, Jeff King,
	Jonathan Nieder

Hi Eric,

On Wed, 5 Sep 2018, Eric Sunshine wrote:

> On Wed, Sep 5, 2018 at 8:39 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > So let's hear some ideas how to improve the situation, m'kay?  Just as
> > a reminder, this is the problem I want to solve: I want to run the
> > tests in a light-weight manner, with minimal output, and only in case
> > of an error do I want to crank up the verbosity. Instead of wasting
> > most of the effort to log everything and then throwing it away in most
> > of the common cases, I suggest to re-run the entire test.
> 
> What about the very different approach of capturing the full "verbose"
> output the executed tests in addition to whatever is actually output
> to the terminal?

I fear it is not really possible to do a "verbose but not really" mode. I
want the console output to be quiet, there is no use in chatting up the
build log with these messages, as we have to run the tests in parallel, so
the output would be utterly hard to interpret anyway. At the same time, I
want verbose output for use in the test results. It is not really possible
to `tee` all output, then "quiet down" the part that makes it into the
log.

> If a test fails, then (and only then) you can insert the captured
> verbose output into the JUnit XML file.

Yep. That's what my patch series does.

If a test fails, then (and only then) I re-run the script with verbose
output that is immediately moved into that JUnit XML file.

> This way (if we always have the full verbose output at hand), you don't
> need to re-run the test at all.

But that way, if we always have the full verbose output, the build log
will be unnecessarily verbose and confusing.

> I've cc:'d Peff and Jonathan since I believe they are more familiar
> with how all the capturing / output-redirection works in the test
> suite.

Okay.

Ciao,
Dscho

P.S.: An unintended side effect of re-running the tests is to identify
flakey tests. I do not yet have a way to represent this outcome in the
test result, but I deem this an additional benefit in favor of keeping my
current strategy.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure
  2018-09-05 20:24             ` Jeff King
@ 2018-09-14 19:04               ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-14 19:04 UTC (permalink / raw)
  To: Jeff King
  Cc: Eric Sunshine, Luke Diamand, gitgitgadget, Git List,
	Junio C Hamano, Jonathan Nieder

Hi Peff,

On Wed, 5 Sep 2018, Jeff King wrote:

> On Wed, Sep 05, 2018 at 02:38:34PM -0400, Eric Sunshine wrote:
> 
> > On Wed, Sep 5, 2018 at 8:39 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > So let's hear some ideas how to improve the situation, m'kay?  Just
> > > as a reminder, this is the problem I want to solve: I want to run
> > > the tests in a light-weight manner, with minimal output, and only in
> > > case of an error do I want to crank up the verbosity. Instead of
> > > wasting most of the effort to log everything and then throwing it
> > > away in most of the common cases, I suggest to re-run the entire
> > > test.
> > 
> > What about the very different approach of capturing the full "verbose"
> > output the executed tests in addition to whatever is actually output
> > to the terminal? If a test fails, then (and only then) you can insert
> > the captured verbose output into the JUnit XML file. This way (if we
> > always have the full verbose output at hand), you don't need to re-run
> > the test at all.
> > 
> > I've cc:'d Peff and Jonathan since I believe they are more familiar
> > with how all the capturing / output-redirection works in the test
> > suite.
> 
> I don't think there's much to know beyond what you wrote. The
> "--verbose" case does not really cost any more than the non-verbose one,
> because the only difference is whether output goes to a file versus
> /dev/null. But the commands all call write() regardless.

This really only holds true on systems where redirected output is
essentially a no-op.

For the MSYS2-emulated case, this is incorrect. And as that case is
already suffering, time-wise, I would be loathe to pile more onto it.

> For --verbose-log, it does of course cost a little extra to run one
> `tee` per script, and to write a few kb of logs. I doubt those are
> measurable versus the rest of a script run, but I'm happy to be
> disproven by numbers.

I tried, and failed, to quantify this properly. Essentially because a
single test run takes over 1h15m, so I tried to do it in the cloud, but
those timings are too variable for anything approaching an accurate
measurement.

> There are some gymnastics done to re-exec the test script with the same
> shell, but AFAIK those are robust and don't cost a lot (again, one extra
> process per script run).

It *is* actually a bit worse here (but only in the case of failures, which
should be the minority of the cases): imagine an expensive test like t0027
that takes something like 14 minutes to run, and then a test failure near
the end: re-running with verbose output will cost another 14 minutes.

However, the benefit of allowing to pass flakey tests outweighs the costs
here, if you ask me.

> I'm not overly concerned about the cost of re-running a test, since the
> idea is that failed tests should be rare. I would be a little worried
> about flaky tests mysteriously righting themselves on the second run (so
> you know a failure happened, but you have no good output to describe
> it).

This is actually a benefit.

Pretty much all the times I can remember a test being flakey (with the one
notable exception of the O_APPEND issue with GIT_TRACE), the *tests* were
buggy.

Plus, it is less concerning if a test fails occasionally vs a test that
fails all the time.

My plan is to indicate the outcome of "Passed upon rerun" in the test
output eventually, as soon as there is server-side support for it.

(Sidenote: there is technically already server-side support for it, but we
would have to generate Visual Studio-style test output, and I had the
hunch that some die-hard Linuxers among the core Git developers would take
objection to that.)

> I do agree that a test_atexit() cleanup would make a lot more sense than
> what's in the original patch. And that's nicer than the exit trap we're
> using already, because you may note that each caller has to manually
> restore the original 'die' handler that test-lib.sh installs.

Okay, then. I will work on this.

Ciao,
Dscho

> That would also help with bitrot. If this funky cleanup case only causes
> problems with junit output, then other people are likely to forget to do
> it, and the mess falls onto the junit folks (i.e., Dscho). But if the
> tests start _relying_ on test_atexit() being called (i.e., don't call
> stop_git_daemon manually, but assume that the atexit handler does so),
> then the responsible authors are a lot more likely to notice and fix it
> early.
> 
> -Peff
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things
  2018-09-05 18:57   ` Sebastian Schuberth
@ 2018-09-14 19:07     ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-09-14 19:07 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: git, Junio C Hamano

Hi Sebastian,

On Wed, 5 Sep 2018, Sebastian Schuberth wrote:

> On 9/3/2018 11:10 PM, Johannes Schindelin via GitGitGadget wrote:
> 
> > +if test -n "$TRAVIS_COMMIT"
> > +then
> > +	# We are running within Travis CI
> 
> Personally, I'd find a check like
> 
> if test "$TRAVIS" = "true"
> 
> more speaking (also see [1]).

Good call.

Will fix,
Dscho

> 
> [1] https://docs.travis-ci.com/user/environment-variables/
> 
> -- 
> Sebastian Schuberth
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, back to index

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 21:10 [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 1/9] ci: rename the library of common functions Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 2/9] ci/lib.sh: encapsulate Travis-specific things Johannes Schindelin via GitGitGadget
2018-09-03 23:43   ` Eric Sunshine
2018-09-04 11:04     ` Johannes Schindelin
2018-09-05 18:57   ` Sebastian Schuberth
2018-09-14 19:07     ` Johannes Schindelin
2018-09-03 21:10 ` [PATCH 3/9] test-date: add a subcommand to measure times in shell scripts Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 4/9] tests: optionally write results as JUnit-style .xml Johannes Schindelin via GitGitGadget
2018-09-04  0:43   ` Eric Sunshine
2018-09-04 10:59     ` Johannes Schindelin
2018-09-03 21:10 ` [PATCH 5/9] ci/lib.sh: add support for VSTS CI Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 6/9] Add a build definition for VSTS Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure Johannes Schindelin via GitGitGadget
2018-09-04  4:30   ` Eric Sunshine
2018-09-04 11:09     ` Johannes Schindelin
2018-09-05  5:32       ` Luke Diamand
2018-09-05 12:39         ` Johannes Schindelin
2018-09-05 13:03           ` Luke Diamand
2018-09-14 18:46             ` Johannes Schindelin
2018-09-05 18:38           ` Eric Sunshine
2018-09-05 20:24             ` Jeff King
2018-09-14 19:04               ` Johannes Schindelin
2018-09-14 18:51             ` Johannes Schindelin
2018-09-03 21:10 ` [PATCH 8/9] tests: record more stderr with --write-junit-xml in case of failure Johannes Schindelin via GitGitGadget
2018-09-03 21:10 ` [PATCH 9/9] README: add a build badge (status of the VSTS build) Johannes Schindelin via GitGitGadget
2018-09-05 19:01 ` [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services Sebastian Schuberth
2018-09-05 19:08   ` Stefan Beller

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox