From: Junio C Hamano <gitster@pobox.com>
To: Stephan Beyer <s-beyer@gmx.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Add test cases for git-am
Date: Fri, 30 May 2008 15:12:41 -0700 [thread overview]
Message-ID: <7vy75rh25i.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <20080530140447.GB10514@leksak.fem-net> (Stephan Beyer's message of "Fri, 30 May 2008 16:04:47 +0200")
Stephan Beyer <s-beyer@gmx.net> writes:
> the reason I added this test is because I'm working on git-sequencer (for
> GSoC'08), which functions as a common backend for git-am, git-rebase, ...
> And I think this test will make my life easier ;-)
Absolutely. Starting by writing a verifiable specification of what you
will be building (aka "tests") is a very good way.
> As this is my first test, I hope to get some constructive feedback.
Date: Fri, 30 May 2008 16:04:47 +0200
From: Stephan Beyer <s-beyer@gmx.net>
To: git@vger.kernel.org
Cc: gitster@pobox.com
Subject: [PATCH] Add test cases for git-am
Message-ID: <20080530140447.GB10514@leksak.fem-net>
Mail-Followup-To: git@vger.kernel.org, gitster@pobox.com
Please don't do this "Mail-Followup-To:"; it is rude to others, and it is
rude to me.
* It is rude to others. When they want to give feedback to _you_
privately and say "Reply", it will put me (and the list) on "To:"
header. They have to be careful and edit their "To:" header. You
stole time from them, which otherwise would have been spent on much
better things, such as actually giving constructive feedbacks.
* It is rude to me (or whoever you place on your M-F-T). I filter and
sort my incoming mails to give precedence to ones that have me on "To:"
over the ones that have me on "Cc:". When people want to give feedback
to _you_ publicly and say "Reply All", it will again put me (and
perhaps the list) on "To:" header, and I may not be interested in
suggestions they are giving to _you_. You stole time from me, which
otherwise would have been spent on something better, like sitting back
and sipping my Caipirinha ;-).
> Perhaps it is also useful to swap t4150 and t4151 or to merge them.
Pehaps. A single test t4150-am.sh might be enough.
> diff --git a/t/t4151-am.sh b/t/t4151-am.sh
> new file mode 100755
> index 0000000..df4fb5a
> --- /dev/null
> +++ b/t/t4151-am.sh
> @@ -0,0 +1,223 @@
> +#!/bin/sh
> +
> +test_description='git am running'
> +
> +. ./test-lib.sh
> +
> +cat >msg <<EOF
> +second
> +
> +Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, sed diam nonumy
> +...
> +feugait nulla facilisi.
> +EOF
> ...
> +test_expect_success setup '
> + echo hello >file &&
> + git add file &&
> + test_tick &&
> + git commit -m first &&
> + git tag first &&
> + echo world >>file &&
> + git add file &&
> + test_tick &&
> + git commit -s -F msg &&
> + git tag second &&
> + git format-patch --stdout first >patch1 &&
> + tail -n +3 msg >file &&
"tail -n 3" you mean? Same for "head -n <n>" in other places.
> ...
> +test_expect_success 'am changes committer and keeps author' '
> + test_tick &&
> + git checkout first &&
> + git am patch2 &&
> + ! test -d .dotest &&
> + test "$(git rev-parse master)" != "$(git rev-parse HEAD)" &&
> + test "$(git rev-parse master^)" != "$(git rev-parse HEAD^)" &&
> + test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
> + test -z "$(git diff master..HEAD)" &&
> + test -z "$(git diff master^..HEAD^)" &&
> + compare author master HEAD &&
> + compare author master^ HEAD^ &&
> + ! compare committer master HEAD &&
> + ! compare committer master^ HEAD^
> +'
Hmmm. Checking for inequality does not feel so robust. You will allow
"am" to record garbage and will not be able to detect a breakage.
> +test_expect_success 'am --signoff adds Signed-off-by: line' '
> + git checkout -b master2 first &&
> + git am --signoff <patch2 &&
> + test "$(git cat-file commit HEAD | grep -c "^Signed-off-by:")" -eq 1 &&
> + test "$(git cat-file commit HEAD^ | grep -c "^Signed-off-by:")" -eq 2
> +'
Again, don't you want to check not just "It added something", but "It
added what we expected it to add"?
> ...
> +test_expect_success 'am --keep really keeps the subject' '
> + git checkout HEAD^ &&
> + git am --keep patch4 &&
> + ! test -d .dotest &&
> + git-cat-file commit HEAD |
> + grep -q -F "Re: Re: Re: [PATCH 1/5 v2] third"
> +'
... in other words, like this one that checks "It left what we expected it
to in the result".
Other than that, I did not think anything obviously wrong with the patch.
next prev parent reply other threads:[~2008-05-30 22:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-05-30 14:04 [PATCH] Add test cases for git-am Stephan Beyer
2008-05-30 22:12 ` Junio C Hamano [this message]
2008-05-31 2:40 ` Stephan Beyer
2008-05-31 6:26 ` Mutt and Mail-Followup-To Teemu Likonen
2008-05-31 19:22 ` [PATCH] Add test cases for git-am Junio C Hamano
2008-05-31 22:07 ` Stephan Beyer
2008-05-31 22:11 ` [PATCH v2 1/2] " Stephan Beyer
2008-05-31 22:11 ` [PATCH v2 2/2] Merge t4150-am-subdir.sh and t4151-am.sh into t4150-am.sh Stephan Beyer
2008-05-31 22:41 ` [PATCH] Add test cases for git-am Junio C Hamano
2008-06-02 17:53 ` Andreas Ericsson
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=7vy75rh25i.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=s-beyer@gmx.net \
/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).