From: Junio C Hamano <gitster@pobox.com>
To: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: git@vger.kernel.org, stolee@gmail.com
Subject: Re: [PATCH 2/8] t7900: setup and tear down clones
Date: Tue, 17 Oct 2023 13:13:54 -0700 [thread overview]
Message-ID: <xmqqwmvlgg71.fsf@gitster.g> (raw)
In-Reply-To: <e3987cda75e4db72393f85de4bbb71d2ebaa097b.1697319294.git.code@khaugsbakk.name> (Kristoffer Haugsbakk's message of "Sat, 14 Oct 2023 23:45:53 +0200")
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.
next prev parent reply other threads:[~2023-10-17 20:14 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-14 21:45 [PATCH 0/8] t7900: untangle test dependencies Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 1/8] t7900: remove register dependency Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 2/8] t7900: setup and tear down clones Kristoffer Haugsbakk
2023-10-17 20:13 ` Junio C Hamano [this message]
2023-10-17 20:20 ` Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 3/8] t7900: create commit so that branch is born Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 4/8] t7900: factor out inheritance test dependency Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 5/8] t7900: factor out common schedule setup Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 6/8] t7900: fix `pfx` dependency Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 7/8] t7900: fix `print-args` dependency Kristoffer Haugsbakk
2023-10-14 21:45 ` [PATCH 8/8] t7900: factor out packfile dependency Kristoffer Haugsbakk
2023-10-14 23:00 ` [PATCH 9/8] t7900: fix register dependency Kristoffer Haugsbakk
2023-10-15 3:04 ` [PATCH 0/8] t7900: untangle test dependencies Jeff King
2023-10-17 19:59 ` Junio C Hamano
2023-10-17 20:14 ` Kristoffer Haugsbakk
2023-10-17 20:49 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwmvlgg71.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).