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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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

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