patch, 8 partst7900: untangle test dependencies

Untangle test dependencies so that all tests only need setup tests to have been run.

For example:

``` ./t7900-maintenance.sh --run=setup,31 ```

Test with:

```
#!/bin/sh
cd t
# Every test run together with `setup` should pass
for i in $(seq 1 42)
do
    ./t7900-maintenance.sh --quiet --run=setup,$i || return 1
done &&
# Whole test suite should pass
./t7900-maintenance.sh --quiet &&
# The tests that used to depend on each other should still pass
# when run together
./t7900-maintenance.sh --quiet --run=setup,30,31 &&
./t7900-maintenance.sh --quiet --run=setup,11,12 &&
./t7900-maintenance.sh --quiet --run=setup,3,19 &&
./t7900-maintenance.sh --quiet --run=setup,23,24 &&
./t7900-maintenance.sh --quiet --run=setup,33,34,35 &&
./t7900-maintenance.sh --quiet --run=setup,36,40 &&
./t7900-maintenance.sh --quiet --run=setup,36,40 &&
./t7900-maintenance.sh --quiet --run=setup,36,37 &&
./t7900-maintenance.sh --quiet --run=setup,15,23,24 &&
printf "\nAll passed\n" ||
printf '\n***Failed***\n'
```

§ CI

The CI failed but it didn't look relevant.

https://github.com/LemmingAvalanche/git/actions/runs/6518415327/job/17703822606

Cheers

Kristoffer Haugsbakk (8):
  t7900: remove register dependency
  t7900: setup and tear down clones
  t7900: create commit so that branch is born
  t7900: factor out inheritance test dependency
  t7900: factor out common schedule setup
  t7900: fix `pfx` dependency
  t7900: fix `print-args` dependency
  t7900: factor out packfile dependency

 t/t7900-maintenance.sh | 49 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40 -- 2.42.0.2.g879ad04204

`stop from existing schedule` depends on the preceding test `start from empty cron table` because the preceding test registers the repository. Without it, the “stop” test fails because `config` fails to get the repository:

    git config --get --global --fixed-value maintenance.repo "$(pwd)"

Remove this dependency by setting up the state and tearing it down independently.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 3 +++
 1 file changed, 3 insertions(+)

Test `loose-objects task` depends on the two clones setup in `prefetch multiple remotes`.

Reuse the two clones setup and tear down the clones afterwards in both tests.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

As I already said in my response to the cover letter, while I am surprised that the series managed to make each step (and it alone) succeed after the set-up (applaud!), I am not sure if it is really worth doing. As the business of test scripts is to test git, and it means that we should always assume that we are dealing with a potentially broken version of git. By running so many git subcommands in test_when_finished, each of them may be from a buggy implementation of git, we cannot be really sure that we are resetting the environment to the pristine state. We should strive to do as little as possible in test_when_finished.

Ditto.

This is even worse; it has to redo much of what the previous test did. Developers cannot be reasonably expected to maintain this duplication when we need to change the earlier test.

While I am impressed that "set-up + individual single test" was made to work, I am not convinced that the changes that took us to get there are reasonable. The end result looks much less maintainable and more wasteful with duplicated steps.

Thanks.

On Tue, Oct 17, 2023, at 22:13, Junio C Hamano wrote:

I'll have to think more about this part in order to understand the ramifications. Thanks for the feedback.

I can rewrite this one—as well as others—to use the `setup` keyword in the original test instead.

But dropping the series is also fine. I am still very new to this test suite.

Cheers

`pack-refs task` cannot be run in isolation but does pass if `maintenance.auto config option` is run first.

Create a commit so that `HEAD` does not point to an unborn branch.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 1 +
 1 file changed, 1 insertion(+)

Factor out the dependency that test `maintenance.strategy inheritance` has on test `--schedule inheritance weekly -> daily -> hourly`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Tests `magic markers are correct` and `stop preserves surrounding schedule` depend on some setup in `start preserves existing schedule`.

Factor out the setup code.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Test `start and stop when several schedulers are available` depends on `pfx` from `start and stop macOS maintenance`.

Duplicate the behavior.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 1 +
 1 file changed, 1 insertion(+)

Test `use launchctl list to prevent extra work` depends on `print-args` from `start and stop macOS maintenance`.

Duplicate the script writing.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 3 +++
 1 file changed, 3 insertions(+)

Tests `'--schedule inheritance weekly -> daily -> hourly` and `maintenance.strategy inheritance` depend on the packfile made in `incremental-repack task`.

Factor out the packfile creation.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7900-maintenance.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

The test `maintenance.auto config option` will fail if any preceding test has run `git maintenance register` since that turns `maintenance.auto` off for that repository and later calls to `unregister` will not turn it back to the default `true` value.

Start with a fresh repository in this test.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    I found this after publishing the series.

 t/t7900-maintenance.sh | 2 ++
 1 file changed, 2 insertions(+)

On Sat, Oct 14, 2023 at 11:45:51PM +0200, Kristoffer Haugsbakk wrote:

From a brief look, I think it is that your branch is based on v2.42.0, which does not contain 0763c3a2c4 (http: update curl http/2 info matching for curl 8.3.0, 2023-09-15). And the macos CI image has since been updated to a more recent version of curl.

So yeah, not anything to worry about for your series.

-Peff

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

It is kind-of surprising that with only 8 patches you can reach such a state, but ...

... this puzzles me. What does it mean for tests to "depend on each other"? Does this mean running #31 with or without running #30 runs under different condition and potentially run different things?

One might argue that, in an ideal world, our tests should work when any non-setup tests are omitted (so, instead of $i above, you'll have an arbitrary subsequence of 1..42 and your tests still pass), and it may be a worthy goal, but at the same time, it may be a bit impractical, as setting things up is costly, but what you can do in the common "setup" will be very small. Or you'll have so much "recovering from damage" in test_when_finished for each test that makes such untangling of dependencies too costly.

On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:

What I mean is that some preceding test has a side-effect that a test depends on. Or that the test depends on some test *not* having done something; patch 9/8 changes `maintenance.auto config option` to delete and init the repository since it depends on the preceding tests *not* having run `git maintenance register`, since that turns off the default `true` value of `maintenance.auto`.

(Maybe those last meta-tests with combining tests like number 30 and 31 was a bit silly.)

I don't know what the policy is. :) My motivation was that I was working on something else which seemed to break the suite, then I tried to reduce the tests that were run to get rid of the noise (`--verbose`), but then it got confusing because I didn't know if I had really broken some tests myself or if more tests would start failing by only running a subset of them.

That last patch 9/8 deals with what I discovered when I added two tests before `maintenance.auto config option`; the test started failing, which made me think that my changes might have some side-effect on what the test is testing. But it was just an invisible dependency on `git maintenance register` *not* having been run in the whole suite up until that point.

Cheers

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

I see. And 31 used to depend on the side effect of having ran 30, but in the updated test, the precondition 31 depends on is created by itself without relying on what 30 did (and in fact, perhaps in the updated test, 30 may rewind what it did as part of the clean-up process using test_when_finished). That makes sense.

Yeah, it is a laudable goal, but I am not sure how practical it is to expect developers to maintain that propertly. Unless there is some automated test to enforce the independence of the tests, that is.

Thanks.