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

[Victoria Dye <vdye@github.com>]

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> 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