Re: Git Test Coverage Report (Thursday, May 30th)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 31, 2019, 18:59 UTC
Message-ID: <nycvar.QRO.7.76.6.1905312050330.1775@tvgsbejvaqbjf.bet>
In-Reply-To: <0d7dbfe6-53df-1df8-ac94-4dfab85bbc9f@gmail.com>

Hi Stolee,

On Fri, 31 May 2019, Derrick Stolee wrote:

Show 31 quoted lines

> On 5/30/2019 2:24 PM, Derrick Stolee wrote:
> > Further, these tests failed
> >
> > t3400-rebase.sh                           (Wstat: 256 Tests: 28 Failed: 2)
> >   Failed tests:  20, 28
> >   Non-zero exit status: 1
> > t3420-rebase-autostash.sh                 (Wstat: 256 Tests: 38 Failed: 6)
> >   Failed tests:  6, 13, 16, 23, 26, 33
> >   Non-zero exit status: 1
> > t3404-rebase-interactive.sh               (Wstat: 256 Tests: 110 Failed: 5)
> >   Failed tests:  3, 9-10, 100-101
> >   Non-zero exit status: 1
> > t5521-pull-options.sh                     (Wstat: 256 Tests: 19 Failed: 1)
> >   Failed test:  3
> >   Non-zero exit status: 1
> > t5551-http-fetch-smart.sh                 (Wstat: 256 Tests: 37 Failed: 1)
> >   Failed test:  26
> >   Non-zero exit status: 1
> >
> > They don't fail locally, so perhaps we shouldn't blindly trust the coverage data
> > until I work out why these errors occurred. (Many of the cases I called out
> > above I couldn't hit locally with a die() statement.)
>
> These tests all failed during the second run that set optional GIT_TEST
> environment variables. Specifically, GIT_TEST_REBASE_USE_BUILTIN=false
> caused these tests to break. We now output this message:
>
> 	warning: the rebase.useBuiltin support has been removed!
> 	See its entry in 'git help config' for details.
>
> I'm removing that variable from the build definition.

Would it make sense to have a file in t/ (or a script-let in ci/) specifying all of the `GIT_TEST_*` variables that are currently supported (and that actually make sense to be set)?

I saw a similar issue recently in a now-defunct Azure Pipeline that also tried to replicate what half of the `linux-gcc` job [*1*] does: to run the test suite with those variables overriding the defaults. That Pipeline broke for the exact same reason you mentioned: we now handle `GIT_TEST_REBASE_USE_BUILTIN` by showing that warning.

And issues like this could easily be avoided if we had, say, `ci/non-standard-settings.sh` that simply set all those `GIT_TEST_*` variables in the way that the `linux-gcc` job does (and of course, this job should then source that file instead of duplicating those assignments).

What do you think? Dscho

Footnote *1*: It is a thorn in my side ever since I started work on our Azure Pipeline support that the `linux-gcc` job actually runs *two* jobs: it runs the vanilla test suite, and then it runs it again after setting all supported `GIT_TEST_*` variables to the non-default settings. This almost doubles the running time of that job, often making it the very last job to finish, and it also makes it unclear whether a test failure stems from said `GIT_TEST_*` settings or not.

I got so annoyed by this, in fact, that I finally broke down and opened https://github.com/gitgitgadget/git/issues/242.