On 2023.11.13 13:49, Jeff King wrote:

On Mon, Nov 13, 2023 at 05:00:37PM +0000, Johannes Schindelin via GitGitGadget wrote:

quoted

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?

quoted

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.