Re: [PATCH 5/8] scalar-clone: add test coverage
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 2, 2022, 09:07 UTC
- Message-ID
- <9n46p076-q126-o813-8s39-936s6667o7o1@tzk.qr>
- In-Reply-To
- <23e8901f-b543-dbb5-c7e1-fa653fdf3d57@github.com>
Hi Victoria,
On Thu, 1 Sep 2022, Victoria Dye wrote:
Show 42 quoted lines
> 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!
Show 5 quoted lines
> 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
Show 15 quoted lines
> > > > > 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 > >