Re: [PATCH v4 2/2] merge: add --quit

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 20, 2019, 18:05 UTC
Message-ID: <nycvar.QRO.7.76.6.1905201904240.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20190518113043.18389-3-pclouds@gmail.com>

Hi Duy,

On Sat, 18 May 2019, Nguyễn Thái Ngọc Duy wrote:

Show 33 quoted lines

> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..625a24a980 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -822,4 +822,30 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
>  	verify_parents $c0 $c1
>  '
>
> +test_expect_success 'merge --quit' '
> +	git init merge-quit &&
> +	(
> +		cd merge-quit &&
> +		test_commit base &&
> +		echo one >>base.t &&
> +		git commit -am one &&
> +		git branch one &&
> +		git checkout base &&
> +		echo two >>base.t &&
> +		git commit -am two &&
> +		test_must_fail git -c rerere.enabled=true merge one &&
> +		test_path_is_file .git/MERGE_HEAD &&
> +		test_path_is_file .git/MERGE_MODE &&
> +		test_path_is_file .git/MERGE_MSG &&
> +		git rerere status >rerere.before &&
> +		git merge --quit &&
> +		test_path_is_missing .git/MERGE_HEAD &&
> +		test_path_is_missing .git/MERGE_MODE &&
> +		test_path_is_missing .git/MERGE_MSG &&
> +		git rerere status >rerere.after &&
> +		test_must_be_empty rerere.after &&
> +		! test_cmp rerere.after rerere.before
> +	)
> +'

Good test cases do not *need* to be excessively long. Something like this should be conciser, and more importantly, less inviting to typos or other bugs:

	test_commit quit-test &&
	test_commit quit-one quit-test.t one &&
	git reset --hard HEAD^ &&
	test_commit quit-two quit-test.t two &&

	test_must_fail git -c rerere.enabled=true merge one &&
	test_path_is_file .git/MERGE_HEAD &&
	git rerere status >rerere &&
	test -s rerere &&

	git merge --quit &&
	test_path_is_missing .git/MERGE_HEAD &&
	git rerere status >rerere &&
	test_must_be_empty rerere

Note that this does not do an exhaustive test for all the .git/MERGE_* files: this test regression promises to verify that `git merge --quit` works, it does not promise to verify that a failed `git merge` leaves all of those files! Using just `MERGE_HEAD` as a tell-tale for that is plenty sufficient.

Likewise, this test case does not verify that the output of `git rerere status` is different before and after the `git merge --quit`. That is not the point of the test, to make sure that they are different. The point is to make sure that it is empty afterwards, but not empty beforehand.

Technically, your version of the test case verifies the same (if a file's contents differ from an empty file's, then necessarily it is not empty, but it requires some gymnastics to come to that conclusion). There is no need for convoluted thinking in regression test cases. In fact, the easier it is to understand the *intent* of a test case, the quicker the investigation of any future bug, and consequently the faster the bug fix.

Also, the less you execute, the quicker the test case runs. That might not sound like much, but we have over 20,000 test cases in our test suite. That multiplication is really easy to compute in your head. If all of them were written succinctly, I bet you would find it much less taxing on your patience to actually run the full test suite from time to time ;-)

And this illustrates a very real cost of a slow test suite in addition to the time: developers will run it less often, causing more regressions, wasting even more time in the long run.

Ciao, Johannes