[5/8] scalar-clone: add test coverage

Commit Message

Victoria Dye Aug. 31, 2022, 4:02 p.m. UTC

From: Victoria Dye <[e-mail]>

Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
behavior of the 'scalar clone' command.

Signed-off-by: Victoria Dye <[e-mail]>
---
 t/t9211-scalar-clone.sh | 135 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100755 t/t9211-scalar-clone.sh

Comments

Johannes Schindelin Sept. 1, 2022, 9:32 a.m. UTC | #1

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <[e-mail]>
>
> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
> behavior of the 'scalar clone' command.

Great catch!

I have one suggestion, given my experience with debugging test failures:

> +test_expect_success 'creates content in enlistment root' '
> +	test_when_finished cleanup_clone cloned &&
> +
> +	scalar clone "file://$(pwd)/to-clone" cloned &&

This pattern of cloning into `cloned` and removing the directory when the
test case is done is repeated throughout this test script.

In instances where all test cases succeed, that poses no problem.

When running the test script with `-i`, also no problem.

But when we run into test failures in CI, those directories will be
removed before the workflow run can tar them up and upload them for later
inspection.

May I suggest an alternative strategy?

If we drop all those `test_when_finished cleanup_clone cloned` calls and
instead `scalar clone` into different directories (whose names reflect the
test cases' intended purpose), I could imagine having a much easier time
not only diagnosing but also reproducing and fixing test failures in the
future.

When discussing code review practices [*1*], we did not come up with any
standard terminology to describe what I am offering here, and I am unsure
how to label this in a catchy way. I want to present this suggestion for
you to consider, and I would be delighted if you take it, at the same time
I will happily let it go should you decide against it.

Ciao,
Dscho

Footnote *1*:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l48

Victoria Dye Sept. 1, 2022, 11:49 p.m. UTC | #2

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <[e-mail]>
>>
>> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
>> behavior of the 'scalar clone' command.
> 
> Great catch!
> 
> I have one suggestion, given my experience with debugging test failures:
> 
>> +test_expect_success 'creates content in enlistment root' '
>> +	test_when_finished cleanup_clone cloned &&
>> +
>> +	scalar clone "file://$(pwd)/to-clone" cloned &&
> 
> This pattern of cloning into `cloned` and removing the directory when the
> test case is done is repeated throughout this test script.
> 
> In instances where all test cases succeed, that poses no problem.
> 
> When running the test script with `-i`, also no problem.
> 
> But when we run into test failures in CI, those directories will be
> removed before the workflow run can tar them up and upload them for later
> inspection.
> 
> May I suggest an alternative strategy?
> 
> If we drop all those `test_when_finished cleanup_clone cloned` calls and
> instead `scalar clone` into different directories (whose names reflect the
> test cases' intended purpose), I could imagine having a much easier time
> not only diagnosing but also reproducing and fixing test failures in the
> future.

While I like the simplicity of using 'test_when_finished', I hadn't
considered the value of having the failed tests' artifacts in the CI
results. If you think that would be helpful to developers, I'll update
accordingly (although I'd still clean up the clones whose tests pass to
avoid archiving more data than we need).

That being said, even if I update 't9211', my experience with Git's test
suite is that very few tests preserve test repos this way. Do you expect
these artifacts to be especially helpful for 'scalar clone' in particular,
or is this more of a "gently nudge contributors to make this standard
practice" sort of recommendation? 

> 
> When discussing code review practices [*1*], we did not come up with any
> standard terminology to describe what I am offering here, and I am unsure
> how to label this in a catchy way. I want to present this suggestion for
> you to consider, and I would be delighted if you take it, at the same time
> I will happily let it go should you decide against it.
> 
> Ciao,
> Dscho
> 
> Footnote *1*:
> https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l48

Johannes Schindelin Sept. 2, 2022, 9:07 a.m. UTC | #3

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin wrote:
>
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >
> >> From: Victoria Dye <[e-mail]>
> >>
> >> Create a new test file ('t9211-scalar-clone.sh') to exercise the options and
> >> behavior of the 'scalar clone' command.
> >
> > Great catch!
> >
> > I have one suggestion, given my experience with debugging test failures:
> >
> >> +test_expect_success 'creates content in enlistment root' '
> >> +	test_when_finished cleanup_clone cloned &&
> >> +
> >> +	scalar clone "file://$(pwd)/to-clone" cloned &&
> >
> > This pattern of cloning into `cloned` and removing the directory when the
> > test case is done is repeated throughout this test script.
> >
> > In instances where all test cases succeed, that poses no problem.
> >
> > When running the test script with `-i`, also no problem.
> >
> > But when we run into test failures in CI, those directories will be
> > removed before the workflow run can tar them up and upload them for later
> > inspection.
> >
> > May I suggest an alternative strategy?
> >
> > If we drop all those `test_when_finished cleanup_clone cloned` calls and
> > instead `scalar clone` into different directories (whose names reflect the
> > test cases' intended purpose), I could imagine having a much easier time
> > not only diagnosing but also reproducing and fixing test failures in the
> > future.
>
> While I like the simplicity of using 'test_when_finished', I hadn't
> considered the value of having the failed tests' artifacts in the CI
> results. If you think that would be helpful to developers, I'll update
> accordingly (although I'd still clean up the clones whose tests pass to
> avoid archiving more data than we need).

Thank you!

> That being said, even if I update 't9211', my experience with Git's test
> suite is that very few tests preserve test repos this way. Do you expect
> these artifacts to be especially helpful for 'scalar clone' in particular,
> or is this more of a "gently nudge contributors to make this standard
> practice" sort of recommendation?

Thank you for this question, which helps me clarify even to myself what my
intention is.

After considering this, yes, I would like this to be a gentle nudge to
take a tiny step toward improving Git's test suite by recommending a new
standard practice.

Ciao,
Dscho

>
> >
> > When discussing code review practices [*1*], we did not come up with any
> > standard terminology to describe what I am offering here, and I am unsure
> > how to label this in a catchy way. I want to present this suggestion for
> > you to consider, and I would be delighted if you take it, at the same time
> > I will happily let it go should you decide against it.
> >
> > Ciao,
> > Dscho
> >
> > Footnote *1*:
> > https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l48
>
>

Junio C Hamano Sept. 2, 2022, 4:52 p.m. UTC | #4

Johannes Schindelin <[e-mail]> writes:

>> That being said, even if I update 't9211', my experience with Git's test
>> suite is that very few tests preserve test repos this way. Do you expect
>> these artifacts to be especially helpful for 'scalar clone' in particular,
>> or is this more of a "gently nudge contributors to make this standard
>> practice" sort of recommendation?
>
> Thank you for this question, which helps me clarify even to myself what my
> intention is.
>
> After considering this, yes, I would like this to be a gentle nudge to
> take a tiny step toward improving Git's test suite by recommending a new
> standard practice.

I agree that it would in general be a good idea to leave more cruft
for those who want to postmortem.  It will nudge a sequence of tests
to use distinct test directories and output files, in order to avoid
letting the later ones overwrite the earlier ones, which would be a
deviation from what we have done historically.

For those who diagnose breakage manually after seeing "tXXXX.sh -i"
fail, cruft left by earlier successful tests are quite distracting
nuisance and that certainly was why we tried to remove them after
each test piece succeeds.  These days, we seem to be relying much
more on unattended bulk tests at CI than before, which changes the
equation.

Patch

diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
new file mode 100755
index 00000000000..9fbbc4de2c0
--- /dev/null
+++ b/t/t9211-scalar-clone.sh
@@ -0,0 +1,135 @@ 
+#!/bin/sh
+
+test_description='test the `scalar clone` subcommand'
+
+. ./test-lib.sh
+
+GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab cron.txt,launchctl:true,schtasks:true"
+export GIT_TEST_MAINT_SCHEDULER
+
+test_expect_success 'set up repository to clone' '
+	rm -rf .git &&
+	git init to-clone &&
+	(
+		cd to-clone &&
+		git branch -m base &&
+
+		test_commit first &&
+		test_commit second &&
+		test_commit third &&
+
+		git switch -c parallel first &&
+		mkdir -p 1/2 &&
+		test_commit 1/2/3 &&
+
+		git switch base &&
+
+		# By default, permit
+		git config uploadpack.allowfilter true &&
+		git config uploadpack.allowanysha1inwant true
+	)
+'
+
+cleanup_clone () {
+	rm -rf "$1"
+}
+
+test_expect_success 'creates content in enlistment root' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+	ls -A cloned >enlistment-root &&
+	test_line_count = 1 enlistment-root &&
+	test_path_is_dir cloned/src &&
+	test_path_is_dir cloned/src/.git
+'
+
+test_expect_success 'with spaces' '
+	test_when_finished cleanup_clone "cloned with space" &&
+
+	scalar clone "file://$(pwd)/to-clone" "cloned with space" &&
+	test_path_is_dir "cloned with space" &&
+	test_path_is_dir "cloned with space/src" &&
+	test_path_is_dir "cloned with space/src/.git"
+'
+
+test_expect_success 'partial clone if supported by server' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+
+	(
+		cd cloned/src &&
+
+		# Two promisor packs: one for refs, the other for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 2 promisorlist
+	)
+'
+
+test_expect_success 'fall back on full clone if partial unsupported' '
+	test_when_finished cleanup_clone cloned &&
+
+	test_config -C to-clone uploadpack.allowfilter false &&
+	test_config -C to-clone uploadpack.allowanysha1inwant false &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned 2>err &&
+	grep "filtering not recognized by server, ignoring" err &&
+
+	(
+		cd cloned/src &&
+
+		# Still get a refs promisor file, but none for blobs
+		ls .git/objects/pack/pack-*.promisor >promisorlist &&
+		test_line_count = 1 promisorlist
+	)
+'
+
+test_expect_success 'initializes sparse-checkout by default' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		test_cmp_config true core.sparseCheckout &&
+		test_cmp_config true core.sparseCheckoutCone
+	)
+'
+
+test_expect_success '--full-clone does not create sparse-checkout' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --full-clone "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		test_cmp_config "" --default "" core.sparseCheckout &&
+		test_cmp_config "" --default "" core.sparseCheckoutCone
+	)
+'
+
+test_expect_success '--single-branch clones HEAD only' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --single-branch "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 1 out &&
+		grep "refs/remotes/origin/base" out
+	)
+'
+
+test_expect_success '--no-single-branch clones all branches' '
+	test_when_finished cleanup_clone cloned &&
+
+	scalar clone --no-single-branch "file://$(pwd)/to-clone" cloned &&
+	(
+		cd cloned/src &&
+		git for-each-ref refs/remotes/origin >out &&
+		test_line_count = 2 out &&
+		grep "refs/remotes/origin/base" out &&
+		grep "refs/remotes/origin/parallel" out
+	)
+'
+
+test_done