Re: [PATCH] tests: turn on network daemon tests by default

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2014, 23:58 UTC
Message-ID: <xmqqzjlxnqvw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140211200445.GA27946@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:
>
>> Those who run buildfarms may want to disable the networking test if
>> the buildfarms are not isolated well, for example.  They have to be
>> told somewhere that now they need to explicitly disable these tests
>> and how.
>
> I think they should be OK. The daemons run on the loopback interface, so
> there is hopefully not a security implication. If multiple buildfarms
> are sharing the same loopback space (e.g., running in separate
> directories on the same machine), the "auto" setting should degrade
> gracefully. One daemon will "win" the setup, and the tests will run, and
> on the other, they will be skipped.
>
>> I am in favor of this change but just pointing out possible fallouts
>> might be larger than we think.
>
> Agreed, but I think the only way to know the size of those fallouts is
> to try it and see who complains.  I would not normally be so cavalier
> with git itself, but I think for the test infrastructure, we have a
> small, tech-savvy audience that can help us iterate on it without too
> much pain.

Sure. One immediate complaint is that I would probably need to do something like this in the build automation:

	if testing a branch without this patch
        then
		: do nothing
	else
		GIT_TEST_GIT_DAEMON=false
	fi

Arguably, it is the fault of the current/original code that treated *any* non-empty value that is set in the environment variable as "true"---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we wouldn't have to have a workaround like this.

I wonder if GIT_TEST_X=$(test_tristate "$GIT_TEST_X") pattern can be made a bit more friendly, though. For example, can we behave differently depending on the reason why $GIT_TEST_X is empty?

 - People who have *not* been opting in to the expensive tests have
   not done anything special; GIT_TEST_X environment variable did
   not exist for them (i.e. unset), and we used to skip when
   "$GIT_TEST_X" is an empty string.

 - We want to encourage people who do not care to run these tests.
   If people do not do anything, their $GIT_TEST_X will continue to
   be an empty string without GIT_TEST_X variable in the
   environment.

If we let people who *do* want to opt out of the expensive tests by explicitly setting $GIT_TEST_X to an empty string in the new scheme, wouldn't the transition go a lot smoother?

The caller may have to pass the name of the variable:

	GIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)

and then the callee may become

	test_tristate () {
		variable=$1
                if eval "test x\"\${$variable+isset}\" = xisset"
		then
			# explicitly set
                        eval "value=\$$variable"
                        case "$value" in
			"")
				echo false ;;
                        false | auto)
				echo "$value" ;;
			*)
				echo true ;;
   			esac
		else
			echo auto
		fi
	}

so that

 - Any non-empty string other than the magic strings "false" and
   "auto" continue to mean "please I want to test";

 - Setting the variable explicitly to an empty string will continue
   to mean "no I do not want to test";

 - Leaving the variable unset will continue to mean "I don't really
   care; just follow the default the project gives me".