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

[Junio C Hamano <gitster@pobox.com>]

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

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