git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v4 2/2] merge: add --quit
Date: Mon, 20 May 2019 20:05:48 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1905201904240.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190518113043.18389-3-pclouds@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]

Hi Duy,

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

> 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

  reply	other threads:[~2019-05-20 18:06 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-01 13:11 [PATCH 0/2] Add "git merge --quit" Nguyễn Thái Ngọc Duy
2019-05-01 13:11 ` [PATCH 1/2] merge: remove drop_save() in favor of remove_merge_branch_state() Nguyễn Thái Ngọc Duy
2019-05-01 13:11 ` [PATCH 2/2] merge: add --quit Nguyễn Thái Ngọc Duy
2019-05-02 21:49   ` Emily Shaffer
2019-05-02 10:13 ` [PATCH 0/2] Add "git merge --quit" Phillip Wood
2019-05-09 10:10 ` [PATCH v2 0/2] nd/merge-quit update Nguyễn Thái Ngọc Duy
2019-05-09 10:10   ` [PATCH v2 1/2] merge: remove drop_save() in favor of remove_merge_branch_state() Nguyễn Thái Ngọc Duy
2019-05-09 10:10   ` [PATCH v2 2/2] merge: add --quit Nguyễn Thái Ngọc Duy
2019-05-14  9:13   ` [PATCH v3 0/2] nd/merge-quit updates Nguyễn Thái Ngọc Duy
2019-05-14  9:13     ` [PATCH v3 1/2] merge: remove drop_save() in favor of remove_merge_branch_state() Nguyễn Thái Ngọc Duy
2019-05-14  9:13     ` [PATCH v3 2/2] merge: add --quit Nguyễn Thái Ngọc Duy
2019-05-14 13:44       ` Johannes Schindelin
2019-05-15  2:58         ` Junio C Hamano
2019-05-15 15:00           ` Johannes Schindelin
2019-05-15  2:52       ` Junio C Hamano
2019-05-18 11:30     ` [PATCH v4 0/2] nd/merge-quit updates Nguyễn Thái Ngọc Duy
2019-05-18 11:30       ` [PATCH v4 1/2] merge: remove drop_save() in favor of remove_merge_branch_state() Nguyễn Thái Ngọc Duy
2019-05-18 11:30       ` [PATCH v4 2/2] merge: add --quit Nguyễn Thái Ngọc Duy
2019-05-20 18:05         ` Johannes Schindelin [this message]
2019-05-20 17:01       ` [PATCH v4 0/2] nd/merge-quit updates Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1905201904240.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).