[PATCH 0/8] t7900: untangle test dependencies

[PATCH 0/8] t7900: untangle test dependencies

[Kristoffer Haugsbakk]

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

[PATCH 1/8] t7900: remove register dependency

[Kristoffer Haugsbakk]

`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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 487e326b3fa..ca86b2ba687 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -588,6 +588,7 @@ test_expect_success 'start --scheduler=<scheduler>' '
 '
 
 test_expect_success 'start from empty cron table' '
+	test_when_finished git maintenance unregister &&
 	GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start --scheduler=crontab &&
 
 	# start registers the repo
@@ -599,6 +600,8 @@ test_expect_success 'start from empty cron table' '
 '
 
 test_expect_success 'stop from existing schedule' '
+	test_when_finished git maintenance unregister &&
+	git maintenance register &&
 	GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance stop &&
 
 	# stop does not unregister the repo
-- 
2.42.0.2.g879ad04204

[PATCH 2/8] t7900: setup and tear down clones

[Kristoffer Haugsbakk]

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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ca86b2ba687..ebc207f1a58 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -145,6 +145,12 @@ test_expect_success 'run --task=prefetch with no remotes' '
 '
 
 test_expect_success 'prefetch multiple remotes' '
+	test_when_finished rm -r clone1 &&
+	test_when_finished rm -r clone2 &&
+	test_when_finished git remote remove remote1 &&
+	test_when_finished git remote remove remote2 &&
+	test_when_finished git tag --delete one &&
+	test_when_finished git tag --delete two &&
 	git clone . clone1 &&
 	git clone . clone2 &&
 	git remote add remote1 "file://$(pwd)/clone1" &&
@@ -175,6 +181,22 @@ test_expect_success 'prefetch multiple remotes' '
 '
 
 test_expect_success 'loose-objects task' '
+	test_when_finished rm -r clone1 &&
+	test_when_finished rm -r clone2 &&
+	test_when_finished git remote remove remote1 &&
+	test_when_finished git remote remove remote2 &&
+	test_when_finished git tag --delete one &&
+	test_when_finished git tag --delete two &&
+	git clone . clone1 &&
+	git clone . clone2 &&
+	git remote add remote1 "file://$(pwd)/clone1" &&
+	git remote add remote2 "file://$(pwd)/clone2" &&
+	git -C clone1 switch -c one &&
+	git -C clone2 switch -c two &&
+	test_commit -C clone1 one &&
+	test_commit -C clone2 two &&
+	git fetch --all &&
+
 	# Repack everything so we know the state of the object dir
 	git repack -adk &&
 
-- 
2.42.0.2.g879ad04204

Re: [PATCH 2/8] t7900: setup and tear down clones

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> 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(+)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index ca86b2ba687..ebc207f1a58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -145,6 +145,12 @@ test_expect_success 'run --task=prefetch with no remotes' '
>  '
>  
>  test_expect_success 'prefetch multiple remotes' '
> +	test_when_finished rm -r clone1 &&
> +	test_when_finished rm -r clone2 &&
> +	test_when_finished git remote remove remote1 &&
> +	test_when_finished git remote remove remote2 &&
> +	test_when_finished git tag --delete one &&
> +	test_when_finished git tag --delete two &&
>  	git clone . clone1 &&
>  	git clone . clone2 &&
>  	git remote add remote1 "file://$(pwd)/clone1" &&

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.

> @@ -175,6 +181,22 @@ test_expect_success 'prefetch multiple remotes' '
>  '
>  
>  test_expect_success 'loose-objects task' '
> +	test_when_finished rm -r clone1 &&
> +	test_when_finished rm -r clone2 &&
> +	test_when_finished git remote remove remote1 &&
> +	test_when_finished git remote remove remote2 &&
> +	test_when_finished git tag --delete one &&
> +	test_when_finished git tag --delete two &&

Ditto.

> +	git clone . clone1 &&
> +	git clone . clone2 &&
> +	git remote add remote1 "file://$(pwd)/clone1" &&
> +	git remote add remote2 "file://$(pwd)/clone2" &&
> +	git -C clone1 switch -c one &&
> +	git -C clone2 switch -c two &&
> +	test_commit -C clone1 one &&
> +	test_commit -C clone2 two &&
> +	git fetch --all &&

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.

Re: [PATCH 2/8] t7900: setup and tear down clones

[Kristoffer Haugsbakk]

On Tue, Oct 17, 2023, at 22:13, Junio C Hamano wrote:
> 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.

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

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

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

-- 
Kristoffer Haugsbakk

[PATCH 3/8] t7900: create commit so that branch is born

[Kristoffer Haugsbakk]

`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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ebc207f1a58..4bfb4ec5cf6 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -388,6 +388,7 @@ test_expect_success 'maintenance.incremental-repack.auto (when config is unset)'
 '
 
 test_expect_success 'pack-refs task' '
+	test_commit message &&
 	for n in $(test_seq 1 5)
 	do
 		git branch -f to-pack/$n HEAD || return 1
-- 
2.42.0.2.g879ad04204

[PATCH 4/8] t7900: factor out inheritance test dependency

[Kristoffer Haugsbakk]

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(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4bfb4ec5cf6..6e3ee365ccd 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -408,14 +408,16 @@ test_expect_success 'invalid --schedule value' '
 	test_i18ngrep "unrecognized --schedule" err
 '
 
-test_expect_success '--schedule inheritance weekly -> daily -> hourly' '
+test_expect_success 'setup for inheritance' '
 	git config maintenance.loose-objects.enabled true &&
 	git config maintenance.loose-objects.schedule hourly &&
 	git config maintenance.commit-graph.enabled true &&
 	git config maintenance.commit-graph.schedule daily &&
 	git config maintenance.incremental-repack.enabled true &&
-	git config maintenance.incremental-repack.schedule weekly &&
+	git config maintenance.incremental-repack.schedule weekly
+'
 
+test_expect_success '--schedule inheritance weekly -> daily -> hourly' '
 	GIT_TRACE2_EVENT="$(pwd)/hourly.txt" \
 		git maintenance run --schedule=hourly 2>/dev/null &&
 	test_subcommand git prune-packed --quiet <hourly.txt &&
-- 
2.42.0.2.g879ad04204

[PATCH 5/8] t7900: factor out common schedule setup

[Kristoffer Haugsbakk]

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(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 6e3ee365ccd..ebde3e8a212 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -637,9 +637,12 @@ test_expect_success 'stop from existing schedule' '
 	test_must_be_empty cron.txt
 '
 
-test_expect_success 'start preserves existing schedule' '
+test_expect_success 'setup important information for schedule' '
 	echo "Important information!" >cron.txt &&
-	GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start --scheduler=crontab &&
+	GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt" git maintenance start --scheduler=crontab
+'
+
+test_expect_success 'start preserves existing schedule' '
 	grep "Important information!" cron.txt
 '
 
-- 
2.42.0.2.g879ad04204

[PATCH 6/8] t7900: fix `pfx` dependency

[Kristoffer Haugsbakk]

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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ebde3e8a212..15a8653b583 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -794,6 +794,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 '
 
 test_expect_success 'start and stop when several schedulers are available' '
+	pfx=$(cd "$HOME" && pwd) &&
 	write_script print-args <<-\EOF &&
 	printf "%s\n" "$*" | sed "s:gui/[0-9][0-9]*:gui/[UID]:; s:\(schtasks /create .* /xml\).*:\1:;" >>args
 	EOF
-- 
2.42.0.2.g879ad04204

[PATCH 7/8] t7900: fix `print-args` dependency

[Kristoffer Haugsbakk]

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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 15a8653b583..99279e41787 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -709,6 +709,9 @@ test_expect_success 'start and stop macOS maintenance' '
 '
 
 test_expect_success 'use launchctl list to prevent extra work' '
+	write_script print-args <<-\EOF &&
+	echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args
+	EOF
 	# ensure we are registered
 	GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start --scheduler=launchctl &&
 
-- 
2.42.0.2.g879ad04204

[PATCH 8/8] t7900: factor out packfile dependency

[Kristoffer Haugsbakk]

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(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 99279e41787..bc417b518b5 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -257,13 +257,15 @@ test_expect_success 'maintenance.loose-objects.auto' '
 	test_subcommand git prune-packed --quiet <trace-loC
 '
 
-test_expect_success 'incremental-repack task' '
+test_expect_success 'setup packfile' '
 	packDir=.git/objects/pack &&
 	for i in $(test_seq 1 5)
 	do
 		test_commit $i || return 1
-	done &&
+	done
+'
 
+test_expect_success 'incremental-repack task' '
 	# Create three disjoint pack-files with size BIG, small, small.
 	echo HEAD~2 | git pack-objects --revs $packDir/test-1 &&
 	test_tick &&
-- 
2.42.0.2.g879ad04204

[PATCH 9/8] t7900: fix register dependency

[Kristoffer Haugsbakk]

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(+)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index bc417b518b..dbc5e1eb44 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -55,6 +55,8 @@ test_expect_success 'run [--auto|--quiet]' '
 '
 
 test_expect_success 'maintenance.auto config option' '
+	rm -rf .git &&
+	git init &&
 	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 	test_subcommand git maintenance run --auto --quiet <default &&
 	GIT_TRACE2_EVENT="$(pwd)/true" \
-- 
2.42.0.2.g879ad04204

Re: [PATCH 0/8] t7900: untangle test dependencies

[Jeff King]

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

> § CI
> 
> The CI failed but it didn't look relevant.
> 
> https://github.com/LemmingAvalanche/git/actions/runs/6518415327/job/17703822606

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

Re: [PATCH 0/8] t7900: untangle test dependencies

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> #!/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 &&

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

> # The tests that used to depend on each other should still pass
> # when run together
> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&

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

Re: [PATCH 0/8] t7900: untangle test dependencies

[Kristoffer Haugsbakk]

On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:
> It is kind-of surprising that with only 8 patches you can reach such
> a state, but ...
>
>> # The tests that used to depend on each other should still pass
>> # when run together
>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
>
> ... 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?

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

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

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

Re: [PATCH 0/8] t7900: untangle test dependencies

[Junio C Hamano]

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

> On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:
>> It is kind-of surprising that with only 8 patches you can reach such
>> a state, but ...
>>
>>> # The tests that used to depend on each other should still pass
>>> # when run together
>>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
>>
>> ... 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?
>
> What I mean is that some preceding test has a side-effect that a test
> depends on.

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.

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

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.