| Message ID | 20190906121326.23056-2-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP tests | expand |
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
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
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/
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
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(-)