Re: [PATCH] ci: avoid running the test suite _twice_

From: Josh Steadmon <steadmon@google.com>
Date: Nov 15, 2023, 21:28 UTC
Message-ID: <ZVU4EVcj0MDrSNcG@google.com>
In-Reply-To: <20231113184909.GB3838361@coredump.intra.peff.net>

On 2023.11.13 13:49, Jeff King wrote:

Show 42 quoted lines

> On Mon, Nov 13, 2023 at 05:00:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> > This is a late amendment of 19ec39aab54 (ci: stop linking the `prove`
> > cache, 2022-07-10), fixing a bug that had been hidden so far.
> 
> We don't seem to have that commit in Junio's tree; it is only in
> git-for-windows.
> 
> Not that we should not fix things if they are broken, but I am trying
> to understand if git/git is experiencing the same bug. It sounds like
> not yet, though from looking at 19ec39aab54, I would expect to get these
> doubled runs any time we store the prove state. But maybe without that
> commit our state-file symlink is going somewhere invalid, and prove
> fails to actually store anything?
> 
> > But starting with that commit, we run `prove` _twice_ in CI, and with
> > completely different sets of tests to run. Due to the bug, the second
> > invocation re-runs all of the tests that were already run as part of the
> > first invocation. This not only wastes build minutes, it also frequently
> > causes the `osx-*` jobs to fail because they already take a long time
> > and now are likely to run into a timeout.
> > 
> > The worst part about it is that there is actually no benefit to keep
> > running with `--state=slow,save`, ever since we decided no longer to
> > try to reuse the Prove cache between CI runs.
> > 
> > So let's just drop that Prove option and live happily ever after.
> 
> Yes, I think this is the right thing to do regardless. If we are not
> saving the state to use between two related runs, there is no point
> storing it in the first place.
> 
> I do have to wonder, though, as somebody who did not follow the
> unit-test topic closely: why are the unit tests totally separate from
> the rest of the suite? I would think we'd want them run from one or more
> t/t*.sh scripts. That would make bugs like this impossible, but also:
> 
>   1. They'd be run via "make test", so developers don't have to remember
>      to run them separately.
> 
>   2. They can be run in parallel with all of the other tests when using
>      "prove -j", etc.

The first part is easy, but I don't see a good way to get both shell tests and unit tests executing under the same `prove` process. For shell tests, we pass `--exec '$(TEST_SHELL_PATH_SQ)'` to prove, meaning that we use the specified shell as an interpreter for the test files. That will not work for unit test executables.

We could bundle all the unit tests into a single shell script, but then we lose parallelization and add hoops to jump through to determine what breaks. Or we could autogenerate a corresponding shell script to run each individual unit test, but that seems gross. Of course, these are hypothetical concerns for now, since we only have a single unit test at the moment.

There's also the issue that the shell test arguments we pass on from prove would be shared with the unit tests. That's fine for now, as t-strbuf doesn't accept any runtime arguments, but it's possible that either the framework or individual unit tests might grow to need arguments, and it might not be convenient to stay compatible with the shell tests.

Personally, I lean towards keeping things simple and just running a second `prove` process as part of `make test`. If I was forced to pick a way to get everything under one process, I'd lean towards autogenerating individual shell script wrappers for each unit test. But I'm open to discussion, especially if people have other approaches I haven't thought of.