Re: Side effects in Git's test suite, was Re: [PATCH] revert: optionally refer to commit in the "reference" format

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 31, 2022, 08:03 UTC
Message-ID: <220531.86y1yi15xt.gmgdl@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2205301840410.349@tvgsbejvaqbjf.bet>

On Mon, May 30 2022, Johannes Schindelin wrote:

Show 41 quoted lines

> Hi Junio,
>
> On Mon, 23 May 2022, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> >> +test_expect_success 'identification of reverted commit (reference)' '
>> >> +	git checkout --detach to-ident &&
>> >> +	git revert --reference --no-edit HEAD &&
>> >> +	git cat-file commit HEAD >actual.raw &&
>> >> +	grep "^This reverts " actual.raw >actual &&
>> >> +	echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect &&
>> >> +	test_cmp expect actual
>> >> +'
>> >
>> > If it was up to me, I would combine these three test cases, if only to
>> > help the `--run=<single-number>` case (the latter two depend on the
>> > side effect of the first one to create a `to-ident` tag).
>>
>> I wonder if our prereq infrastructure is lightweight and scalable enough
>> so that we can easily add a support a pseudo-prerequisite PREVIOUS that
>> lets us say
>>
>> 	test_expect_success PREVIOUS "identification ..." '
>> 		...
>> 	'
>>
>> to mean that this test requires the previous test has not been
>> skipped.
>
> In theory, this sounds good to me.
>
> In practice, however, side effects are awful and make everything harder,
> from developing code to debugging to helping new contributors. I wish we
> would do away with them altogether and have something more akin to the
> before/after constructs known from e.g. TestNG (think `@BeforeTest` and
> `@BeforeClass`).
>
> One option would be to mark `setup` steps completely differently, sort of
> imitating the prereq infrastructure instead of using
> `test_expect_success`. Kind of prereqs, but required to pass.

I've suggested a test_expect_setup before: https://lore.kernel.org/git/8735vrvg39.fsf@evledraar.gmail.com/; basically a test_expect_success that ignores GIT_SKIP_TESTS and --run.

I think that even if we could imagine much more complex relationships (up to and including writing these tests as Makefiles instead) it's better to just have the simpler "this is a setup".

Then for everything more complex be more eager to split up tests.

> This could potentially allow us to randomize the order in which the test
> cases are run, to identify and fix (unintended) side effects.

Yes, a "chaos monkey" mode similar to --stress would be nice.

> A complication is that we have nothing in the way of `@AfterClass`, i.e.
> we do not have a way to, say, run an Apache instance for the lifecycle of
> a given test script _and tear it down at the end_.

I think this is generally a feature in that if you find yourself needing this you should split the test up so that the Apache setup is in only one file.

E.g. in the case of some http tests we have a prereq on something for git:// and a different thing (apache) for http:// tests, so that depending on what combination you have we might end up needlessly skipping tests.

> Another, rather obvious complication is that we have 17 years of commit
> history introducing side effects left and right :laughing: