Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@xxxxxxxxx> wrote:
> 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.

> 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...

> 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@xxxxxxxxx>

More below...

> ---
> 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.

> +       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.

> +       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"

> +       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 &&
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]