Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 19, 2021, 16:37 UTC
Message-ID: <211119.86o86g2j4h.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211119154036.5n5kpecgnptzkaqn@fs>

On Fri, Nov 19 2021, Fabian Stelzer wrote:

Show 52 quoted lines

> On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote:
>>
>>On Fri, Nov 19 2021, Fabian Stelzer wrote:
>>
>>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>>>>
>>>>> In certain environments or for specific test scenarios we might expect a
>>>>> specific prerequisite check to succeed. Therefore we would like to
>>>>> trigger an error when running our tests if this is not the case.
>>>>
>>>>trigger an error but...
>>>>
>>>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ
>>>>> which can be set to a comma separated list of prereqs. If one of these
>>>>> prereq tests fail then the whole test run will abort.
>>>>
>>>>..here it's "abort the whole test run". If that's what you want use
>>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>>>>syntax on bad SANITIZE=leak use, 2021-10-14)
>>>>
>>>
>>> Hm, while testing this change i noticed another problem that i really
>>> have no idea how to fix.
>>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed
>>> when run with '-v'. This is not the case when the prereq is specified
>>> in the test header. The test run will abort, but no error will be
>>> printed which can be quite confusing :/
>>> I guess this has something to do with how tests are run in subshells and
>>> their outputs only printed with -v. Maybe there should be some kind of
>>> override for BAIL_OUT at least? Not sure if/how this could be done.
>>
>>It has to do with how we juggle file descriptors around, see test_eval_
>>in test-lib.sh.
>>
>>So the "real" stdout is fd 5, not 1 when you're in a prereq.
>>
>>Just:
>>
>>    BAIL_OUT "bad" >&5
>>
>>Will work, maybe it's a good idea to have:
>>
>>	BAIL_OUT_PREREQ () {
>>		BAIL_OUT $@ >&5
>>	}
>>
>>Sorry, I forgot about that caveat when suggesting it.
>
> Hm. Any reason to not do this in BAIL_OUT itself?  As far as i can see
> the setup of the additional fd's would only need to move up a few lines.

That does look like a better solution, I've tried it just now locally & it works for me. Perhaps there's some subtlety I'm missing, but that should Just Work.

This is by far not the first time I've poked at something in test-lib.sh only to discover that its pattern of doing setup A, setup C, setup B etc. caused a problem solved by moving B & C around :(

It could really do with a change to move everything it's now doing to functions, which we'd then call, so what setup we do in what order would fit on a single screen, but that's a much larger change...