Stephan Beyer <s-beyer@xxxxxxx> 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@xxxxxxx>
To: git@xxxxxxxxxxxxxxx
Cc: gitster@xxxxxxxxx
Subject: [PATCH] Add test cases for git-am
Message-ID: <20080530140447.GB10514@xxxxxxxxxxxxxx>
Mail-Followup-To: git@xxxxxxxxxxxxxxx, gitster@xxxxxxxxx
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
