Re: [PATCH 2/2] CI: enable t1509 in Travis and GitHub Actions

From: Junio C Hamano <gitster@pobox.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/2] CI: enable t1509 in Travis and GitHub Actions
Date: Wed, 13 May 2020 09:51:56 -0700	[thread overview]
Message-ID: <xmqq8shvvkur.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <fa2e1f7b44262eac1fa26161fc5d3f3b9b6bdb47.1589375923.git.congdanhqx@gmail.com> (=?utf-8?B?IsSQb8OgbiBUcuG6p24gQ8O0bmc=?= Danh"'s message of "Wed, 13 May 2020 20:23:07 +0700")

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  ci/lib.sh              | 13 +++++++++++++
>  ci/run-docker-build.sh |  3 +++
>  2 files changed, 16 insertions(+)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index dac36886e3..e9c22ae718 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -113,6 +113,7 @@ then
>  	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>  	export GIT_TEST_OPTS="--verbose-log -x --immediate"
>  	MAKEFLAGS="$MAKEFLAGS --jobs=2"
> +	t1509_allowed=YES
>  elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
>  then
>  	CI_TYPE=azure-pipelines
> @@ -162,6 +163,7 @@ then
>  	echo "::add-matcher::ci/git-problem-matcher.json"
>  	test linux-musl = "$jobname" ||
>  	MAKEFLAGS="$MAKEFLAGS TEST_SHELL_PATH=/bin/sh"
> +	t1509_allowed=YES
>  else
>  	echo "Could not identify CI type" >&2
>  	env >&2
> @@ -184,6 +186,17 @@ export DEVELOPER=1
>  export DEFAULT_TEST_TARGET=prove
>  export GIT_TEST_CLONE_2GB=true
>  
> +if test "$t1509_allowed" = YES
> +then
> +	case "$jobname" in
> +	osx-*) ;;
> +	*)
> +		chmod a+w / || sudo chmod a+w / || true
> +		export IKNOWWHATIAMDOING=YES

Eeeww ;-)  This makes readers wonder where we did not enable the
test and why.  Perhaps throw in a matching

	t1509_allowed=NO

in the azure thing for completeness?

Also, do we want to give a more descriptive name than t1509 to the
variable, say, ROOT_WORK_TREE_TEST_ALLOWED?

> diff --git a/ci/run-docker-build.sh b/ci/run-docker-build.sh
> index 8d47a5fda3..026afe275a 100755
> --- a/ci/run-docker-build.sh
> +++ b/ci/run-docker-build.sh
> @@ -58,6 +58,8 @@ else
>  	test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir"
>  fi
>  
> +chmod a+w /
> +
>  # Build and test
>  command $switch_cmd su -m -l $CI_USER -c "
>  	set -ex
> @@ -68,6 +70,7 @@ command $switch_cmd su -m -l $CI_USER -c "
>  	export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB'
>  	export MAKEFLAGS='$MAKEFLAGS'
>  	export cache_dir='$cache_dir'
> +	export IKNOWWHATIAMDOING=YES
>  	cd /usr/src/git
>  	test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
>  	make

Big EWWWWWWwwww.  Do we need to do this for _all_ tests, not
selectively only while running t1509?  This makes me worried as a
test by mistake can easily corrupt the VM and invalidating the
tests; I know we get a fresh one every time, so there is no
permanent harm done by corrupting it, but having one fewer thing we
have to worry about is always better than having one more thing.