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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 1, 2022, 09:32 UTC
Message-ID: <o429p430-sp18-rr20-p2ro-4r60qs2n83o5@tzk.qr>
In-Reply-To: <a3b7cb0a3bd1f56172db8420b9e80a26be1fda5d.1661961746.git.gitgitgadget@gmail.com>

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.

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