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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 30, 2022, 16:50 UTC
Message-ID: <nycvar.QRO.7.76.6.2205301840410.349@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq35gzn9vk.fsf@gitster.g>

Hi Junio,

On Mon, 23 May 2022, Junio C Hamano wrote:

Show 25 quoted lines

> 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.

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

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_.

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

Ciao, Dscho