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

From: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Date: May 13, 2020, 23:52 UTC
Message-ID: <20200513235215.GA10618@danh.dev>
In-Reply-To: <xmqq8shvvkur.fsf@gitster.c.googlers.com>

On 2020-05-13 09:51:56-0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> > +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?

I was thinking about allowing people set it via environment variable and check, but it seems too risky, now.

Perhaps, always reset it to NO before the checking for $CI_TYPE, and enable it selectively for only Travis, and GitHub Actions.

I didn't enable it for Azure because I can't assure it ;).

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

Yeah, I think all caps is better for this risky variable. I think using T1509_ROOT_WORK_TREE_TEST_ALLOWED is better, to point out which test is risky. But it require future tests with root work-tree must be written in t1509, since it's rare usecase, It'd be fine, I think.

Show 29 quoted lines

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

Perhaps pass this variable all the way down from ci/lib.sh? Adding another variable into t1509 (except T1509_*) doesn't make it less risky. Or should we add T1509_ prefix to this env var?

-- 
Danh