Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano <gitster@pobox.com>]

Samuel GROOT <samuel.groot@grenoble-inp.org> writes:

> @@ -97,7 +104,7 @@ test_expect_success $PREREQ 'setup expect' '
>  '
>  
>  test_expect_success $PREREQ 'Verify commandline' '
> -	test_cmp expected commandline1
> +	test_cmp_noorder expected commandline1
>  '
>  
>  test_expect_success $PREREQ 'Send patches with --envelope-sender' '

By the way, don't you find it irritating to review this patch that
has three hunks, all of which look like the above?  You cannot
easily tell which 3 among 27 instances of test_cmp are modified,
because the hunks do not give useful context.

This is not at all your fault, but because the existing tests are
structured poorly.  It separates one logical step into three pieces
without a good reason.

Here is an illustration of an organization that I think would be
easier to read, and would result in a more readable patch when
modification is made on top.  The first two hunks collapse the
overall "setup" steps that appear as three separate tests into a
single "setup" test.  The last hunk that begin at -83/+79 collapses
a logically-single test that is split across three into one, and
makes the order of things done in the test to (1) set an
expectation, (2) execute the command and (3) compare the result with
the expectation.

I am not going to commit this myself, because I do not want to
create conflicts with the change your topic is trying to do, and
besides, almost all the remainder of the tests follow "one logical
test split into three" pattern and need to be corrected before this
"illustration" can become a real patch.

I do not mind if you take it and complete it as a preliminary
clean-up step in your series; or you can "keep it in mind, but
ignore it for now", in which case this can be a "low hanging fruit"
somebody else, hopefully somebody new to the development community,
can use to dip their toes ;-)



 t/t9001-send-email.sh | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index b3355d2..858bdbe 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -6,14 +6,16 @@ test_description='git send-email'
 # May be altered later in the test
 PREREQ="PERL"
 
-test_expect_success $PREREQ 'prepare reference tree' '
+clean_fake_sendmail () {
+	rm -f commandline* msgtxt*
+}
+
+test_expect_success $PREREQ 'setup' '
 	echo "1A quick brown fox jumps over the" >file &&
 	echo "lazy dog" >>file &&
 	git add file &&
-	GIT_AUTHOR_NAME="A" git commit -a -m "Initial."
-'
+	GIT_AUTHOR_NAME="A" git commit -a -m "Initial." &&
 
-test_expect_success $PREREQ 'Setup helper tool' '
 	write_script fake.sendmail <<-\EOF &&
 	shift
 	output=1
@@ -28,14 +30,8 @@ test_expect_success $PREREQ 'Setup helper tool' '
 	cat >"msgtxt$output"
 	EOF
 	git add fake.sendmail &&
-	GIT_AUTHOR_NAME="A" git commit -a -m "Second."
-'
-
-clean_fake_sendmail () {
-	rm -f commandline* msgtxt*
-}
+	GIT_AUTHOR_NAME="A" git commit -a -m "Second." &&
 
-test_expect_success $PREREQ 'Extract patches' '
 	patches=$(git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1)
 '
 
@@ -83,20 +79,19 @@ test_expect_success $PREREQ 'No confirm with sendemail.confirm=never' '
 	check_no_confirm
 '
 
-test_expect_success $PREREQ 'Send patches' '
-	git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
-'
-
-test_expect_success $PREREQ 'setup expect' '
-	cat >expected <<-\EOF
+test_expect_success $PREREQ 'with --suppress-cc=sob --from and --to' '
+	cat >expected <<-\EOF &&
 	!nobody@example.com!
 	!author@example.com!
 	!one@example.com!
 	!two@example.com!
 	EOF
-'
 
-test_expect_success $PREREQ 'Verify commandline' '
+	git send-email --suppress-cc=sob \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" $patches 2>errors &&
+
 	test_cmp expected commandline1
 '