{"thread":{"id":"60368","subject":"[PATCH 0/8] t7900: untangle test dependencies","startedAt":"2023-10-14T21:46:55Z","lastAt":"2023-10-17T20:49:41Z","messageCount":16,"participants":["Kristoffer Haugsbakk","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"483262","messageId":"cover.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":null,"subject":"[PATCH 0/8] t7900: untangle test dependencies","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:51Z","receivedAt":"2023-10-14T21:46:55Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Untangle test dependencies so that all tests only need setup tests to have\nbeen run.\n\nFor example:\n\n```\n./t7900-maintenance.sh --run=setup,31\n```\n\nTest with:\n\n```\n#!/bin/sh\ncd t\n# Every test run together with `setup` should pass\nfor i in $(seq 1 42)\ndo\n ./t7900-maintenance.sh --quiet --run=setup,$i || return 1\ndone &&\n# Whole test suite should pass\n./t7900-maintenance.sh --quiet &&\n# The tests that used to depend on each other should still pass\n# when run together\n./t7900-maintenance.sh --quiet --run=setup,30,31 &&\n./t7900-maintenance.sh --quiet --run=setup,11,12 &&\n./t7900-maintenance.sh --quiet --run=setup,3,19 &&\n./t7900-maintenance.sh --quiet --run=setup,23,24 &&\n./t7900-maintenance.sh --quiet --run=setup,33,34,35 &&\n./t7900-maintenance.sh --quiet --run=setup,36,40 &&\n./t7900-maintenance.sh --quiet --run=setup,36,40 &&\n./t7900-maintenance.sh --quiet --run=setup,36,37 &&\n./t7900-maintenance.sh --quiet --run=setup,15,23,24 &&\nprintf \"\\nAll passed\\n\" ||\nprintf '\\n***Failed***\\n'\n```\n\n§ CI\n\nThe CI failed but it didn't look relevant.\n\nhttps://github.com/LemmingAvalanche/git/actions/runs/6518415327/job/17703822606\n\nCheers\n\nKristoffer Haugsbakk (8):\n t7900: remove register dependency\n t7900: setup and tear down clones\n t7900: create commit so that branch is born\n t7900: factor out inheritance test dependency\n t7900: factor out common schedule setup\n t7900: fix `pfx` dependency\n t7900: fix `print-args` dependency\n t7900: factor out packfile dependency\n\n t/t7900-maintenance.sh | 49 ++++++++++++++++++++++++++++++++++++------\n 1 file changed, 43 insertions(+), 6 deletions(-)\n\n\nbase-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n--\n2.42.0.2.g879ad04204\n"},{"id":"483263","messageId":"6d9398e64d0acb69219877c54ba3fdfa0faa0dbf.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 1/8] t7900: remove register dependency","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:52Z","receivedAt":"2023-10-14T21:46:58Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"`stop from existing schedule` depends on the preceding test `start from\nempty cron table` because the preceding test registers the\nrepository. Without it, the “stop” test fails because `config` fails to\nget the repository:\n\n git config --get --global --fixed-value maintenance.repo \"$(pwd)\"\n\nRemove this dependency by setting up the state and tearing it down\nindependently.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 487e326b3fa..ca86b2ba687 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -588,6 +588,7 @@ test_expect_success 'start --scheduler=' '\n '\n \n test_expect_success 'start from empty cron table' '\n+\ttest_when_finished git maintenance unregister &&\n \tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab cron.txt\" git maintenance start --scheduler=crontab &&\n \n \t# start registers the repo\n@@ -599,6 +600,8 @@ test_expect_success 'start from empty cron table' '\n '\n \n test_expect_success 'stop from existing schedule' '\n+\ttest_when_finished git maintenance unregister &&\n+\tgit maintenance register &&\n \tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab cron.txt\" git maintenance stop &&\n \n \t# stop does not unregister the repo\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483264","messageId":"e3987cda75e4db72393f85de4bbb71d2ebaa097b.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 2/8] t7900: setup and tear down clones","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:53Z","receivedAt":"2023-10-14T21:46:59Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Test `loose-objects task` depends on the two clones setup in `prefetch\nmultiple remotes`.\n\nReuse the two clones setup and tear down the clones afterwards in both\ntests.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex ca86b2ba687..ebc207f1a58 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -145,6 +145,12 @@ test_expect_success 'run --task=prefetch with no remotes' '\n '\n \n test_expect_success 'prefetch multiple remotes' '\n+\ttest_when_finished rm -r clone1 &&\n+\ttest_when_finished rm -r clone2 &&\n+\ttest_when_finished git remote remove remote1 &&\n+\ttest_when_finished git remote remove remote2 &&\n+\ttest_when_finished git tag --delete one &&\n+\ttest_when_finished git tag --delete two &&\n \tgit clone . clone1 &&\n \tgit clone . clone2 &&\n \tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n@@ -175,6 +181,22 @@ test_expect_success 'prefetch multiple remotes' '\n '\n \n test_expect_success 'loose-objects task' '\n+\ttest_when_finished rm -r clone1 &&\n+\ttest_when_finished rm -r clone2 &&\n+\ttest_when_finished git remote remove remote1 &&\n+\ttest_when_finished git remote remove remote2 &&\n+\ttest_when_finished git tag --delete one &&\n+\ttest_when_finished git tag --delete two &&\n+\tgit clone . clone1 &&\n+\tgit clone . clone2 &&\n+\tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n+\tgit remote add remote2 \"file://$(pwd)/clone2\" &&\n+\tgit -C clone1 switch -c one &&\n+\tgit -C clone2 switch -c two &&\n+\ttest_commit -C clone1 one &&\n+\ttest_commit -C clone2 two &&\n+\tgit fetch --all &&\n+\n \t# Repack everything so we know the state of the object dir\n \tgit repack -adk &&\n \n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483265","messageId":"ba932076d8a14745f32fbdf63ed0e8cc74774869.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 3/8] t7900: create commit so that branch is born","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:54Z","receivedAt":"2023-10-14T21:47:01Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"`pack-refs task` cannot be run in isolation but does pass if\n`maintenance.auto config option` is run first.\n\nCreate a commit so that `HEAD` does not point to an unborn branch.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex ebc207f1a58..4bfb4ec5cf6 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -388,6 +388,7 @@ test_expect_success 'maintenance.incremental-repack.auto (when config is unset)'\n '\n \n test_expect_success 'pack-refs task' '\n+\ttest_commit message &&\n \tfor n in $(test_seq 1 5)\n \tdo\n \t\tgit branch -f to-pack/$n HEAD || return 1\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483270","messageId":"a4491ff0411be82179a2f40c36ce427a5d7c39f6.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 4/8] t7900: factor out inheritance test dependency","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:55Z","receivedAt":"2023-10-14T21:47:03Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Factor out the dependency that test `maintenance.strategy inheritance` has\non test `--schedule inheritance weekly -> daily -> hourly`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4bfb4ec5cf6..6e3ee365ccd 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -408,14 +408,16 @@ test_expect_success 'invalid --schedule value' '\n \ttest_i18ngrep \"unrecognized --schedule\" err\n '\n \n-test_expect_success '--schedule inheritance weekly -> daily -> hourly' '\n+test_expect_success 'setup for inheritance' '\n \tgit config maintenance.loose-objects.enabled true &&\n \tgit config maintenance.loose-objects.schedule hourly &&\n \tgit config maintenance.commit-graph.enabled true &&\n \tgit config maintenance.commit-graph.schedule daily &&\n \tgit config maintenance.incremental-repack.enabled true &&\n-\tgit config maintenance.incremental-repack.schedule weekly &&\n+\tgit config maintenance.incremental-repack.schedule weekly\n+'\n \n+test_expect_success '--schedule inheritance weekly -> daily -> hourly' '\n \tGIT_TRACE2_EVENT=\"$(pwd)/hourly.txt\" \\\n \t\tgit maintenance run --schedule=hourly 2>/dev/null &&\n \ttest_subcommand git prune-packed --quiet \n---\n t/t7900-maintenance.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 6e3ee365ccd..ebde3e8a212 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -637,9 +637,12 @@ test_expect_success 'stop from existing schedule' '\n \ttest_must_be_empty cron.txt\n '\n \n-test_expect_success 'start preserves existing schedule' '\n+test_expect_success 'setup important information for schedule' '\n \techo \"Important information!\" >cron.txt &&\n-\tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab cron.txt\" git maintenance start --scheduler=crontab &&\n+\tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab cron.txt\" git maintenance start --scheduler=crontab\n+'\n+\n+test_expect_success 'start preserves existing schedule' '\n \tgrep \"Important information!\" cron.txt\n '\n \n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483267","messageId":"5b70e635e2bdd8fc16ff6ff3c1eaecd10ba66634.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 6/8] t7900: fix `pfx` dependency","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:57Z","receivedAt":"2023-10-14T21:47:08Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Test `start and stop when several schedulers are available` depends on\n`pfx` from `start and stop macOS maintenance`.\n\nDuplicate the behavior.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex ebde3e8a212..15a8653b583 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -794,6 +794,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n '\n \n test_expect_success 'start and stop when several schedulers are available' '\n+\tpfx=$(cd \"$HOME\" && pwd) &&\n \twrite_script print-args <<-\\EOF &&\n \tprintf \"%s\\n\" \"$*\" | sed \"s:gui/[0-9][0-9]*:gui/[UID]:; s:\\(schtasks /create .* /xml\\).*:\\1:;\" >>args\n \tEOF\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483268","messageId":"c22183d7cdd0442dfef139b79e9e37f5c070de44.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 7/8] t7900: fix `print-args` dependency","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:58Z","receivedAt":"2023-10-14T21:47:09Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Test `use launchctl list to prevent extra work` depends on `print-args`\nfrom `start and stop macOS maintenance`.\n\nDuplicate the script writing.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 15a8653b583..99279e41787 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -709,6 +709,9 @@ test_expect_success 'start and stop macOS maintenance' '\n '\n \n test_expect_success 'use launchctl list to prevent extra work' '\n+\twrite_script print-args <<-\\EOF &&\n+\techo $* | sed \"s:gui/[0-9][0-9]*:gui/[UID]:\" >>args\n+\tEOF\n \t# ensure we are registered\n \tGIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start --scheduler=launchctl &&\n \n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483269","messageId":"ec4caa88fd2dd9d5ffb96b3cecf6ed89797266dd.1697319294.git.code@khaugsbakk.name","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"[PATCH 8/8] t7900: factor out packfile dependency","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:45:59Z","receivedAt":"2023-10-14T21:47:13Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Tests `'--schedule inheritance weekly -> daily -> hourly` and\n`maintenance.strategy inheritance` depend on the packfile made in\n`incremental-repack task`.\n\nFactor out the packfile creation.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n t/t7900-maintenance.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 99279e41787..bc417b518b5 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -257,13 +257,15 @@ test_expect_success 'maintenance.loose-objects.auto' '\n \ttest_subcommand git prune-packed --quiet \n---\n\nNotes (series):\n I found this after publishing the series.\n\n t/t7900-maintenance.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex bc417b518b..dbc5e1eb44 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -55,6 +55,8 @@ test_expect_success 'run [--auto|--quiet]' '\n '\n \n test_expect_success 'maintenance.auto config option' '\n+\trm -rf .git &&\n+\tgit init &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/default\" git commit --quiet --allow-empty -m 1 &&\n \ttest_subcommand git maintenance run --auto --quiet § CI\n> \n> The CI failed but it didn't look relevant.\n> \n> https://github.com/LemmingAvalanche/git/actions/runs/6518415327/job/17703822606\n\nFrom a brief look, I think it is that your branch is based on v2.42.0,\nwhich does not contain 0763c3a2c4 (http: update curl http/2 info\nmatching for curl 8.3.0, 2023-09-15). And the macos CI image has since\nbeen updated to a more recent version of curl.\n\nSo yeah, not anything to worry about for your series.\n\n-Peff\n"},{"id":"483359","messageId":"xmqqbkcxhvf9.fsf@gitster.g","threadId":"60368","inReplyTo":"cover.1697319294.git.code@khaugsbakk.name","subject":"Re: [PATCH 0/8] t7900: untangle test dependencies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T19:59:38Z","receivedAt":"2023-10-17T19:59:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> #!/bin/sh\n> cd t\n> # Every test run together with `setup` should pass\n> for i in $(seq 1 42)\n> do\n> ./t7900-maintenance.sh --quiet --run=setup,$i || return 1\n> done &&\n\nIt is kind-of surprising that with only 8 patches you can reach such\na state, but ...\n\n> # The tests that used to depend on each other should still pass\n> # when run together\n> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&\n\n... this puzzles me. What does it mean for tests to \"depend on each\nother\"? Does this mean running #31 with or without running #30 runs\nunder different condition and potentially run different things?\n\nOne might argue that, in an ideal world, our tests should work when\nany non-setup tests are omitted (so, instead of $i above, you'll\nhave an arbitrary subsequence of 1..42 and your tests still pass),\nand it may be a worthy goal, but at the same time, it may be a bit\nimpractical, as setting things up is costly, but what you can do in\nthe common \"setup\" will be very small. Or you'll have so much\n\"recovering from damage\" in test_when_finished for each test that\nmakes such untangling of dependencies too costly.\n\n"},{"id":"483360","messageId":"xmqqwmvlgg71.fsf@gitster.g","threadId":"60368","inReplyTo":"e3987cda75e4db72393f85de4bbb71d2ebaa097b.1697319294.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/8] t7900: setup and tear down clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T20:13:54Z","receivedAt":"2023-10-17T20:14:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> Test `loose-objects task` depends on the two clones setup in `prefetch\n> multiple remotes`.\n>\n> Reuse the two clones setup and tear down the clones afterwards in both\n> tests.\n>\n> Signed-off-by: Kristoffer Haugsbakk \n> ---\n> t/t7900-maintenance.sh | 22 ++++++++++++++++++++++\n> 1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index ca86b2ba687..ebc207f1a58 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -145,6 +145,12 @@ test_expect_success 'run --task=prefetch with no remotes' '\n> '\n> \n> test_expect_success 'prefetch multiple remotes' '\n> +\ttest_when_finished rm -r clone1 &&\n> +\ttest_when_finished rm -r clone2 &&\n> +\ttest_when_finished git remote remove remote1 &&\n> +\ttest_when_finished git remote remove remote2 &&\n> +\ttest_when_finished git tag --delete one &&\n> +\ttest_when_finished git tag --delete two &&\n> \tgit clone . clone1 &&\n> \tgit clone . clone2 &&\n> \tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n\nAs I already said in my response to the cover letter, while I am\nsurprised that the series managed to make each step (and it alone)\nsucceed after the set-up (applaud!), I am not sure if it is really\nworth doing. As the business of test scripts is to test git, and it\nmeans that we should always assume that we are dealing with a\npotentially broken version of git. By running so many git\nsubcommands in test_when_finished, each of them may be from a buggy\nimplementation of git, we cannot be really sure that we are\nresetting the environment to the pristine state. We should strive\nto do as little as possible in test_when_finished.\n\n> @@ -175,6 +181,22 @@ test_expect_success 'prefetch multiple remotes' '\n> '\n> \n> test_expect_success 'loose-objects task' '\n> +\ttest_when_finished rm -r clone1 &&\n> +\ttest_when_finished rm -r clone2 &&\n> +\ttest_when_finished git remote remove remote1 &&\n> +\ttest_when_finished git remote remove remote2 &&\n> +\ttest_when_finished git tag --delete one &&\n> +\ttest_when_finished git tag --delete two &&\n\nDitto.\n\n> +\tgit clone . clone1 &&\n> +\tgit clone . clone2 &&\n> +\tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n> +\tgit remote add remote2 \"file://$(pwd)/clone2\" &&\n> +\tgit -C clone1 switch -c one &&\n> +\tgit -C clone2 switch -c two &&\n> +\ttest_commit -C clone1 one &&\n> +\ttest_commit -C clone2 two &&\n> +\tgit fetch --all &&\n\nThis is even worse; it has to redo much of what the previous test\ndid. Developers cannot be reasonably expected to maintain this\nduplication when we need to change the earlier test.\n\nWhile I am impressed that \"set-up + individual single test\" was made\nto work, I am not convinced that the changes that took us to get\nthere are reasonable. The end result looks much less maintainable\nand more wasteful with duplicated steps.\n\nThanks.\n"},{"id":"483361","messageId":"8cd788dc-7d16-4cfd-9f70-7889dcaa7199@app.fastmail.com","threadId":"60368","inReplyTo":"xmqqbkcxhvf9.fsf@gitster.g","subject":"Re: [PATCH 0/8] t7900: untangle test dependencies","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-17T20:14:03Z","receivedAt":"2023-10-17T20:14:28Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"\nOn Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:\n> It is kind-of surprising that with only 8 patches you can reach such\n> a state, but ...\n>\n>> # The tests that used to depend on each other should still pass\n>> # when run together\n>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&\n>\n> ... this puzzles me. What does it mean for tests to \"depend on each\n> other\"? Does this mean running #31 with or without running #30 runs\n> under different condition and potentially run different things?\n\nWhat I mean is that some preceding test has a side-effect that a test\ndepends on. Or that the test depends on some test *not* having done\nsomething; patch 9/8 changes `maintenance.auto config option` to delete\nand init the repository since it depends on the preceding tests *not*\nhaving run `git maintenance register`, since that turns off the default\n`true` value of `maintenance.auto`.\n\n(Maybe those last meta-tests with combining tests like number 30 and 31\nwas a bit silly.)\n\n> One might argue that, in an ideal world, our tests should work when\n> any non-setup tests are omitted (so, instead of $i above, you'll\n> have an arbitrary subsequence of 1..42 and your tests still pass),\n> and it may be a worthy goal, but at the same time, it may be a bit\n> impractical, as setting things up is costly, but what you can do in\n> the common \"setup\" will be very small. Or you'll have so much\n> \"recovering from damage\" in test_when_finished for each test that\n> makes such untangling of dependencies too costly.\n\nI don't know what the policy is. :) My motivation was that I was working\non something else which seemed to break the suite, then I tried to reduce\nthe tests that were run to get rid of the noise (`--verbose`), but then it\ngot confusing because I didn't know if I had really broken some tests\nmyself or if more tests would start failing by only running a subset of\nthem.\n\nThat last patch 9/8 deals with what I discovered when I added two tests\nbefore `maintenance.auto config option`; the test started failing, which\nmade me think that my changes might have some side-effect on what the test\nis testing. But it was just an invisible dependency on `git maintenance\nregister` *not* having been run in the whole suite up until that point.\n\nCheers\n"},{"id":"483362","messageId":"655ca147-c214-41be-919d-023c1b27b311@app.fastmail.com","threadId":"60368","inReplyTo":"xmqqwmvlgg71.fsf@gitster.g","subject":"Re: [PATCH 2/8] t7900: setup and tear down clones","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-17T20:20:12Z","receivedAt":"2023-10-17T20:20:35Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Oct 17, 2023, at 22:13, Junio C Hamano wrote:\n> As I already said in my response to the cover letter, while I am\n> surprised that the series managed to make each step (and it alone)\n> succeed after the set-up (applaud!), I am not sure if it is really\n> worth doing. As the business of test scripts is to test git, and it\n> means that we should always assume that we are dealing with a\n> potentially broken version of git. By running so many git\n> subcommands in test_when_finished, each of them may be from a buggy\n> implementation of git, we cannot be really sure that we are\n> resetting the environment to the pristine state. We should strive\n> to do as little as possible in test_when_finished.\n\nI'll have to think more about this part in order to understand the\nramifications. Thanks for the feedback.\n\n> This is even worse; it has to redo much of what the previous test\n> did. Developers cannot be reasonably expected to maintain this\n> duplication when we need to change the earlier test.\n>\n> While I am impressed that \"set-up + individual single test\" was made\n> to work, I am not convinced that the changes that took us to get\n> there are reasonable. The end result looks much less maintainable\n> and more wasteful with duplicated steps.\n>\n> Thanks.\n\nI can rewrite this one—as well as others—to use the `setup` keyword in the\noriginal test instead.\n\nBut dropping the series is also fine. I am still very new to this test\nsuite.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n"},{"id":"483366","messageId":"xmqqv8b5ezz1.fsf@gitster.g","threadId":"60368","inReplyTo":"8cd788dc-7d16-4cfd-9f70-7889dcaa7199@app.fastmail.com","subject":"Re: [PATCH 0/8] t7900: untangle test dependencies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T20:49:38Z","receivedAt":"2023-10-17T20:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:\n>> It is kind-of surprising that with only 8 patches you can reach such\n>> a state, but ...\n>>\n>>> # The tests that used to depend on each other should still pass\n>>> # when run together\n>>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&\n>>\n>> ... this puzzles me. What does it mean for tests to \"depend on each\n>> other\"? Does this mean running #31 with or without running #30 runs\n>> under different condition and potentially run different things?\n>\n> What I mean is that some preceding test has a side-effect that a test\n> depends on.\n\nI see. And 31 used to depend on the side effect of having ran 30,\nbut in the updated test, the precondition 31 depends on is created\nby itself without relying on what 30 did (and in fact, perhaps in\nthe updated test, 30 may rewind what it did as part of the clean-up\nprocess using test_when_finished). That makes sense.\n\n> I don't know what the policy is. :) My motivation was that I was working\n> on something else which seemed to break the suite, then I tried to reduce\n> the tests that were run to get rid of the noise (`--verbose`), but then it\n> got confusing because I didn't know if I had really broken some tests\n> myself or if more tests would start failing by only running a subset of\n> them.\n\nYeah, it is a laudable goal, but I am not sure how practical it is\nto expect developers to maintain that propertly. Unless there is\nsome automated test to enforce the independence of the tests, that\nis.\n\nThanks.\n"}]}