Re: [PATCH v3 1/3] t7501: add merge conflict tests for dry run

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 17, 2018, 17:45 UTC
Message-ID: <xmqqd0vlpxhy.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqq1sc1rdvz.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 17 quoted lines

> But by splitting these into separate tests, the patch makes such a
> potential failure with "git commit --short" break the later steps.
>
> Not very nice.
>
> It may be a better change to just do in the original one
>
> 	git add test-file &&
> 	git commit --dry-run &&
> +	git commit --short &&
> +	git commit --long &&
> +	git commit --porcelain &&
> 	git commit -m "conflicts fixed from merge."
>
> without adding these new and separate tests, and then mark that one
> to expect a failure (because it would pass up to the --dry-run
> commit, but the --short commit would fail) at this step, perhaps?

Of course, if you want to be more thorough, anticipating that other people in their future updates may break --short but not --long or --porcelain, testing each option in separate test_expect_success is a necessary way to do so, but then you'd need to actually be more thorough, by not merely running each of them in separate test_expect_success block but also arranging that each of them start in an expected state to try the thing we want it to try. That is

	for opt in --dry-run --short --long --porcelain
	do
		test_expect_success "commit $opt" '
			set up the conflicted state after merge &&
			git commit $opt
		'
	done

where the "set up the state" part makes sure it can tolerate potential mistakes of previous run of "git commit $opt" (e.g. it by mistake made a commit, making the index identical to HEAD and taking us out of "merge in progress" state).

But from your 1/3 I did not get the impression that you particularly want to be more thorough, and from your 3/3 I did not get the impression that you anticipate --short/--long/--porcelain may get broken independently. And if that is the case, then chaining all of them together like the above is a more honest way to express that we are only doing a minimum set of testing.

Thanks.