[2/2] ci: restore running httpd tests
diff mbox series

Message ID 20190906121326.23056-2-szeder.dev@gmail.com
State New
Headers show
Series
  • [1/2] t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP tests
Related show

Commit Message

SZEDER Gábor Sept. 6, 2019, 12:13 p.m. UTC
Once upon a time GIT_TEST_HTTPD was a tristate variable and we
exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
that we run the httpd tests in the Linux Clang and GCC build jobs, or
error out if they can't be run for any reason [1].

Then 3b072c577b (tests: replace test_tristate with "git env--helper",
2019-06-21) came along, turned GIT_TEST_HTTPD into a bool, but forgot
to update our CI scripts accordingly.  So, since GIT_TEST_HTTPD is set
explicitly, but its value is not one of the standardized true values,
our CI jobs have been simply skipping the httpd tests in the last
couple of weeks.

Set 'GIT_TEST_HTTPD=true' to restore running httpd tests in our CI
jobs.

[1] a1157b76eb (travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh',
    2017-12-12)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Sept. 6, 2019, 5:03 p.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> that we run the httpd tests in the Linux Clang and GCC build jobs, or
> error out if they can't be run for any reason [1].

Wow.  I vaguely recall having mumbled about keeping YesPlease as an
extra synomym for 'yes' for safety against misconversions, as its
use in the build/test infrastructure is so ingrained, but at the end
went with the standard set of boolean without doing such extending,
with the hope that soon any misconversion would be found out.

This discovery of a misconversion took 3 months, which may or may
not match the definition of "soon" X-<.

The fix is obviously correct.  Thanks.

> Then 3b072c577b (tests: replace test_tristate with "git env--helper",
> 2019-06-21) came along, turned GIT_TEST_HTTPD into a bool, but forgot
> to update our CI scripts accordingly.  So, since GIT_TEST_HTTPD is set
> explicitly, but its value is not one of the standardized true values,
> our CI jobs have been simply skipping the httpd tests in the last
> couple of weeks.
>
> Set 'GIT_TEST_HTTPD=true' to restore running httpd tests in our CI
> jobs.
>
> [1] a1157b76eb (travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh',
>     2017-12-12)
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  ci/lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 44db2d5cbb..29dc740d40 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -160,7 +160,7 @@ linux-clang|linux-gcc)
>  		export CC=gcc-8
>  	fi
>  
> -	export GIT_TEST_HTTPD=YesPlease
> +	export GIT_TEST_HTTPD=true
>  
>  	# The Linux build installs the defined dependency versions below.
>  	# The OS X build installs much more recent versions, whichever
Jeff King Sept. 6, 2019, 7:13 p.m. UTC | #2
On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:

> Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> that we run the httpd tests in the Linux Clang and GCC build jobs, or
> error out if they can't be run for any reason [1].

Yikes, good catch.

I wonder if it would be possible for the test suite to catch this. I
think env--helper would have written a message to stderr, but because we
use --exit-code, we can't tell the difference between that and "false".

I think we'd have go back to something more like:

  test_tristate () {
	bool=$(git env--helper --type=bool --default=true "$1") ||
		eval "error \"$1 is not a bool: \$$1\""
	test "$bool" = "true"
  }
  ...
  if test_tristate GIT_TEST_HTTPD
  then
	... use httpd ...
  fi

Not sure if it's worth it.

-Peff
SZEDER Gábor Sept. 7, 2019, 10:16 a.m. UTC | #3
On Fri, Sep 06, 2019 at 03:13:01PM -0400, Jeff King wrote:
> On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:
> 
> > Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> > exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> > that we run the httpd tests in the Linux Clang and GCC build jobs, or
> > error out if they can't be run for any reason [1].
> 
> Yikes, good catch.
> 
> I wonder if it would be possible for the test suite to catch this. I
> think env--helper would have written a message to stderr, but because we
> use --exit-code, we can't tell the difference between that and "false".

No, '--exit-code' only suppresses the printing of 'true' and 'false',
but it doesn't have any effect on the command's exit code.  And if the
value of the environment variable is not a bool, then it does print an
error message to standard error, and, more importantly, exits with 128
(as opposed to 1 indicating false).  So we could tell the difference,
but as it happens the command is invoked as 'if ! git env-helper ...',
which then interprets that 128 the same as on ordinary false.

  $ VAR=false git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  1
  $ VAR=true git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  0
  $ VAR=YesPlease git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  fatal: bad numeric config value 'YesPlease' for 'VAR': invalid unit
  128


> I think we'd have go back to something more like:
> 
>   test_tristate () {

The env var is supposed to be a bool, so there is no third state
anymore.

> 	bool=$(git env--helper --type=bool --default=true "$1") ||

Some callsites use '--default=false', so we need a second parameter to
specify the default.

> 		eval "error \"$1 is not a bool: \$$1\""
> 	test "$bool" = "true"
>   }
>   ...
>   if test_tristate GIT_TEST_HTTPD
>   then
> 	... use httpd ...
>   fi
> 
> Not sure if it's worth it.

Well...  On one hand, there are no other similar issues in our CI
scripts (there is still a GIT_TEST_CLONE_2GB=YesPlease, but it's only
ever checked with 'test -z' in 't5608-clone-2gb.sh', so it works as
expected, even though it's inconsistent, and
'GIT_TEST_CLONE_2GB=NoThanks' would do the wrong thing).

OTOH, some unsuspecting devs might still have a
'GIT_TEST_foo=YesPlease' in their 'config.mak' from the old days...

Furthermore, as for the "good catch", I just got lucky, and not only
once but several times in a row: that Perforce filehost outage the
other day errored one of my builds, so I came up with a fix [1], for
once took the extra effort and checked it on Azure Pipelines, and
since that was my first ever build over there, out of curiosity I
scrolled through the whole build output, by chance those "skipped:
Network testing disabled (unset GIT_TEST_HTTPD to enable)" lines
caught my eye, shrugged and thought that Dscho should better fix this,
but then an hour later got suspicious, so looked up a recent Travis CI
build, and...  here we are.

I have to wonder how much longer it could have been remained
unnoticed, if the Perforce filehost hadn't failed or if I hadn't made
the gitgitgadget PR for my fix, or...

Anyway, this is just a long-winded way to say that I think we should
validate those bools properly and error loudly on an invalid value
even if it doesn't seem to worth it.


[1] https://public-inbox.org/git/20190906102711.6401-1-szeder.dev@gmail.com/T/

Patch
diff mbox series

diff --git a/ci/lib.sh b/ci/lib.sh
index 44db2d5cbb..29dc740d40 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -160,7 +160,7 @@  linux-clang|linux-gcc)
 		export CC=gcc-8
 	fi
 
-	export GIT_TEST_HTTPD=YesPlease
+	export GIT_TEST_HTTPD=true
 
 	# The Linux build installs the defined dependency versions below.
 	# The OS X build installs much more recent versions, whichever