diff mbox series

[6/7] t/t3437: update the tests

Message ID 20210207181439.1178-7-charvi077@gmail.com (mailing list archive)
State New, archived
Headers show
Series Improve the 'fixup [-C | -c]' in interactive rebase | expand

Commit Message

Charvi Mendiratta Feb. 7, 2021, 6:14 p.m. UTC 
Let's do the changes listed below to make tests more easier to follow :

-Remove the dependency of 'expected-message' file from earlier tests to
make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.

-Add author timestamp to check that the author date of fixed up commit
is unchanged.

-Simplify the test_commit_message() and add comments before the
function.

-Clarify the working of 'fixup -c' with "amend!" in the test-description.

-Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 82 ++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 37 deletions(-)

--
2.29.0.rc1

Comments

Eric Sunshine Feb. 7, 2021, 6:43 p.m. UTC | #1 
On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Let's do the changes listed below to make tests more easier to follow :
>
> -Remove the dependency of 'expected-message' file from earlier tests to
> make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.
>
> -Add author timestamp to check that the author date of fixed up commit
> is unchanged.
>
> -Simplify the test_commit_message() and add comments before the
> function.
>
> -Clarify the working of 'fixup -c' with "amend!" in the test-description.
>
> -Remove unnecessary curly braces and use the named commits in the
> tests so that they will still refer to the same commit if the setup
> gets changed in the future whereas 'branch~2' will change which commit
> it points to.

Typically, if you find yourself enumerating a list of distinct changes
like this in a commit message, it's a good indication that it should
be split into multiple patches, each taking care of one item from the
list. A good reason for splitting it up like this is that it's
difficult for reviewers to keep the entire list in mind while
reviewing the patch, however, it's easy to keep in mind a single
stated goal while reading the changes.

Having said that, I'm not sure it's worth a re-roll or the extra work
of actually splitting it up since you've already been dragged deeper
into this than planned, and these are relatively minor issues.

(Returning to this after reading the remainder of the patch, I did
find it reasonably confusing trying to figure out which changes
related to each other and to items from the list above. It would have
been easier to reason about the changes had they been done in separate
patches. Still, though, I'm not sure it's worth the time and effort to
split them up -- but I wouldn't complain if you did.)

More below...

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> @@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
>  This test checks the "fixup [-C|-c]" command of rebase interactive.
>  In addition to amending the contents of the commit, "fixup -C"
>  replaces the original commit message with the message of the fixup
> -commit. "fixup -c" also replaces the original message, but opens the
> -editor to allow the user to edit the message before committing.
> +commit and similar to "fixup" command that works with "fixup!", "fixup -C"
> +works with "amend!" upon --autosquash. "fixup -c" also replaces the original
> +message, but opens the editor to allow the user to edit the message before
> +committing.
>  '

I had trouble digesting this run-on sentence due, I think, to the
mixing of thoughts. It might be easier to understand if you first talk
only about the options to `fixup` (-c/-C), and then, as a separate
sentence, talk about how `amend!` is transformed into `fixup -C` (like
`fixup!` is transformed into `fixup`). However, as this is just minor
descriptive text in a test file, not user-facing documentation, I'm
not sure it matters enough to warrant a re-roll.

> @@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.
> +# test_commit_message <rev> -m <msg>
> +# test_commit_message <rev> <path>
> +# Verify that the commit message of <rev> matches
> +# <msg> or the content of <path>.

Good.

>  test_commit_message () {
> +       git show --no-patch --pretty=format:%B "$1" >actual &&
> +    case "$2" in
> +    -m) echo "$3" >expect &&
> +           test_cmp expect actual ;;
> +    *) test_cmp "$2" actual ;;
> +    esac
>  }

The funky indentation here is due to a mix of tabs and spaces. It
should use tabs exclusively.
Charvi Mendiratta Feb. 8, 2021, 4:30 a.m. UTC | #2 
Hi Eric,

On Mon, 8 Feb 2021 at 00:13, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> Typically, if you find yourself enumerating a list of distinct changes
> like this in a commit message, it's a good indication that it should
> be split into multiple patches, each taking care of one item from the
> list. A good reason for splitting it up like this is that it's
> difficult for reviewers to keep the entire list in mind while
> reviewing the patch, however, it's easy to keep in mind a single
> stated goal while reading the changes.
>
> Having said that, I'm not sure it's worth a re-roll or the extra work
> of actually splitting it up since you've already been dragged deeper
> into this than planned, and these are relatively minor issues.

> (Returning to this after reading the remainder of the patch, I did
> find it reasonably confusing trying to figure out which changes
> related to each other and to items from the list above. It would have
> been easier to reason about the changes had they been done in separate
> patches. Still, though, I'm not sure it's worth the time and effort to
> split them up -- but I wouldn't complain if you did.)
>

Agree, I will split this patch.

> More below...
>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> > ---
> > diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> > @@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
> >  This test checks the "fixup [-C|-c]" command of rebase interactive.
> >  In addition to amending the contents of the commit, "fixup -C"
> >  replaces the original commit message with the message of the fixup
> > -commit. "fixup -c" also replaces the original message, but opens the
> > -editor to allow the user to edit the message before committing.
> > +commit and similar to "fixup" command that works with "fixup!", "fixup -C"
> > +works with "amend!" upon --autosquash. "fixup -c" also replaces the original
> > +message, but opens the editor to allow the user to edit the message before
> > +committing.
> >  '
>
> I had trouble digesting this run-on sentence due, I think, to the
> mixing of thoughts. It might be easier to understand if you first talk
> only about the options to `fixup` (-c/-C), and then, as a separate
> sentence, talk about how `amend!` is transformed into `fixup -C` (like
> `fixup!` is transformed into `fixup`). However, as this is just minor
> descriptive text in a test file, not user-facing documentation, I'm
> not sure it matters enough to warrant a re-roll.
>

Okay, will change it.

> >  test_commit_message () {
> > +       git show --no-patch --pretty=format:%B "$1" >actual &&
> > +    case "$2" in
> > +    -m) echo "$3" >expect &&
> > +           test_cmp expect actual ;;
> > +    *) test_cmp "$2" actual ;;
> > +    esac
> >  }
>
> The funky indentation here is due to a mix of tabs and spaces. It
> should use tabs exclusively.

Oh, thanks I will correct it.
diff mbox series

Patch

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 3de899f68a..96f3a94831 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -8,8 +8,10 @@  test_description='git rebase interactive fixup options
 This test checks the "fixup [-C|-c]" command of rebase interactive.
 In addition to amending the contents of the commit, "fixup -C"
 replaces the original commit message with the message of the fixup
-commit. "fixup -c" also replaces the original message, but opens the
-editor to allow the user to edit the message before committing.
+commit and similar to "fixup" command that works with "fixup!", "fixup -C"
+works with "amend!" upon --autosquash. "fixup -c" also replaces the original
+message, but opens the editor to allow the user to edit the message before
+committing.
 '

 . ./test-lib.sh
@@ -18,36 +20,34 @@  editor to allow the user to edit the message before committing.

 EMPTY=""

+# test_commit_message <rev> -m <msg>
+# test_commit_message <rev> <path>
+# Verify that the commit message of <rev> matches
+# <msg> or the content of <path>.
 test_commit_message () {
-	rev="$1" && # commit or tag we want to test
-	file="$2" && # test against the content of a file
-	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
-	if test "$2" = -m
-	then
-		str="$3" && # test against a string
-		printf "%s\n" "$str" >tmp-expected-message &&
-		file="tmp-expected-message"
-	fi
-	test_cmp "$file" actual-message
+	git show --no-patch --pretty=format:%B "$1" >actual &&
+    case "$2" in
+    -m) echo "$3" >expect &&
+	    test_cmp expect actual ;;
+    *) test_cmp "$2" actual ;;
+    esac
 }

 get_author () {
 	rev="$1" &&
-	git log -1 --pretty=format:"%an %ae" "$rev"
+	git log -1 --pretty=format:"%an %ae %at" "$rev"
 }

 test_expect_success 'setup' '
 	cat >message <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	new subject
-	${EMPTY}
+	$EMPTY
 	new
 	body
 	EOF

-	sed "1,2d" message >expected-message &&
-
 	test_commit A A &&
 	test_commit B B &&
 	get_author HEAD >expected-author &&
@@ -68,40 +68,43 @@  test_expect_success 'setup' '
 	echo B1 >B &&
 	test_tick &&
 	git commit --fixup=HEAD -a &&
+	git tag B1 &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
 	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
 	EOF
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
+	git tag B2 &&
 	echo B3 >B &&
 	test_tick &&
 	git commit -a -F - <<-EOF &&
 	amend! amend! amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
-	${EMPTY}
+	$EMPTY
 	edited 3
 	EOF
+	git tag B3 &&

 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
@@ -134,6 +137,7 @@  test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
@@ -145,13 +149,14 @@  test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	test_write_lines "" "edited" >>expected-message &&
 	test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_cmp_rev HEAD^ conflicts &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
-	test_write_lines "" edited >>expected-message &&
 	test_commit_message HEAD expected-message &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author
@@ -167,12 +172,12 @@  test_expect_success 'skipping fixup -C after fixup gives correct message' '
 '

 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
@@ -180,7 +185,7 @@  test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '

 test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
 	test_when_finished "test_might_fail git rebase --abort" &&
-	git checkout branch && git checkout --detach branch~2 &&
+	git checkout --detach B2~ &&
 	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
@@ -190,13 +195,16 @@  test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	base=$(git rev-parse HEAD~4) &&
-	FAKE_COMMIT_MESSAGE="Modified-A3" \
+	git log -1 --pretty=format:%B >expected-message &&
+	test_write_lines "" "Modified-A3" >>expected-message &&
+	FAKE_COMMIT_AMEND="Modified-A3" \
 		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
-		git rebase -i $base &&
-	test_cmp_rev $base HEAD^ &&
-	test 1 = $(git show | grep Modified-A3 | wc -l)
+		git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author &&
+	test_commit_message HEAD expected-message
 '

 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
@@ -211,12 +219,12 @@  test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 '

 test_expect_success 'fixup -C works upon --autosquash with amend!' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --autosquash \
 						--signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message