Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 28, 2015, 13:34 UTC
- Message-ID
- <CAPig+cRHB3Qzm-e1_KROu2RQoW2rftLH=uKrWQBsnW0EYkcLPw@mail.gmail.com>
- In-Reply-To
- <1432620908-16071-2-git-send-email-patryk.obara@gmail.com>
On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
Show 5 quoted lines
> Currently messages are compared with --pretty=format:%s%b which does > not retain raw format of commit message. In result it's not clear what > part of expected commit msg is subject and what part is body. Also, it's > impossible to test if messages with multiple lines are handled > correctly, which may be significant when using nondefault --cleanup.
Makes sense.
> Change "commit_msg_is" function to use raw message format in log and > interpret escaped sequences in expected message. This way it's possible > to test exactly what commit message text was saved.
These changes would be less daunting to review if split into multiple patches; one per logical change. So, for instance, patch 1 would make this change and adjust tests accordingly.
Show 9 quoted lines
> Add test to verify, that no additional content is appended to template > message, which uncovers tiny "bug" in --status handling - new line is > always appended before status message. If template file ended with > newline (which is default for many popular text editors, e.g. vim) > then blank line appears before status (which is very annoying when > template ends with line starting with '#'). On the other hand, this > newline needs to be appended if template file didn't end with newline > (which is default for e.g. emacs) - otherwise first line of status > message may be not cleaned up.
This could be patch 2.
> Add explicit test to verify if \n is kept unexpanded in commit message - > this used to be part of unrelated template test.
And patch 3, and so on...
Show 6 quoted lines
> Modify add-content-and-comment fake editor to include both comments and > whitespace, so --cleanup=whitespace is now actually tested. > > Modify expected value of test "cleanup commit messages" (t7502), which > shouldn't be passing, because template and logfiles are unnecessarily > stripped before placing them into editor.
Your cover letter correctly states that with this patch is applied, a number of tests fail. Tests which are expected to fail should be declared test_expect_failure rather than test_expect_success. The patch which fixes the failures should flip them to test_expect_success.
> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
More below...
Show 22 quoted lines
> ---
> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
> index 116885a..fd1bf71 100755
> --- a/t/t7500-commit.sh
> +++ b/t/t7500-commit.sh
> @@ -13,8 +13,8 @@ commit_msg_is () {
> expect=commit_msg_is.expect
> actual=commit_msg_is.actual
>
> - printf "%s" "$(git log --pretty=format:%s%b -1)" >"$actual" &&
> - printf "%s" "$1" >"$expect" &&
> + git log --pretty=format:%B -1 >"$actual" &&
> + printf "%b" "$1" >"$expect" &&
> test_i18ncmp "$expect" "$actual"
> }
>
> @@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using --fixup' '
> test_must_fail git commit --fixup HEAD~1 -F log
> '
>
> +test_expect_success 'no blank lines appended after template with --status' '
> + echo "template line" > "$TEMPLATE" &&Style: Modern code omits the space after the redirection operator (>"$TEMPLATE"), however, it's also important to match existing style. Unfortunately, this file has an equal mixture of both '>blap' and '> blap', so it's difficult to know which style to match. As this is new code, it'd probably be best to omit the space.
Show 8 quoted lines
> + echo changes >>foo && > + git add foo && > + test_set_editor "$TEST_DIRECTORY"/t7500/add-content && > + git commit -e -t "$TEMPLATE" --status && > + commit_msg_is "template line\ncommit message\n" > +' > + > +test_expect_success 'template without newline before eof should work with --status' '
It's not clear what "should work" means. I suppose you mean that the end result should have exactly one newline after the template. Perhaps the test title could indicate the intent more clearly.
Show 9 quoted lines
> + printf "%s" "template line" > "$TEMPLATE" && > + echo changes >>foo && > + git add foo && > + test_set_editor "$TEST_DIRECTORY"/t7500/add-content && > + git commit -e -t "$TEMPLATE" --status && > + commit_msg_is "template line\ncommit message\n" > +' > + > +test_expect_success 'logfile without newline before eof should work with --status' '
Ditto: Unclear "should work"
Show 25 quoted lines
> + printf "%s" "log line" >log-file &&
> + echo changes >>foo &&
> + git add foo &&
> + test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> + git commit -e -F log-file --status &&
> + commit_msg_is "log line\ncommit message\n"
> +'
> test_done
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 051489e..d2203ed 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -8,11 +8,12 @@ commit_msg_is () {
> expect=commit_msg_is.expect
> actual=commit_msg_is.actual
>
> - printf "%s" "$(git log --pretty=format:%s%b -1)" >$actual &&
> - printf "%s" "$1" >$expect &&
> - test_i18ncmp $expect $actual
> + git log --pretty=format:%B -1 >"$actual" &&
> + printf "%b" "$1" >"$expect" &&
> + test_i18ncmp "$expect" "$actual"
> }
>
> +Sneaking in unnecessary whitespace change.
> # Arguments: [<prefix] [<commit message>] [<commit options>]
> check_summary_oneline() {
> test_tick &&