Re: [PATCH v2 1/3] diff-highlight: add some tests.

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2016, 19:09 UTC
Message-ID: <xmqqziob8b6i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160817153124.7770-2-henderson.bj@gmail.com>

Brian Henderson <henderson.bj@gmail.com> writes:

Show 11 quoted lines

> Signed-off-by: Brian Henderson <henderson.bj@gmail.com>
> ---
>  contrib/diff-highlight/Makefile                  |  5 ++
>  contrib/diff-highlight/t/Makefile                | 22 ++++++++
>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 62 +++++++++++++++++++++
>  contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++
>  4 files changed, 158 insertions(+)
>  create mode 100644 contrib/diff-highlight/Makefile
>  create mode 100644 contrib/diff-highlight/t/Makefile
>  create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh
>  create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh

I am not sure test-diff-highlight.sh should be there; the function definitions would still be useful but move them to the beginning of t9400-diff-highlight.sh perhaps?

Show 8 quoted lines

> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
> new file mode 100644
> index 0000000..b866259
> --- /dev/null
> +++ b/contrib/diff-highlight/Makefile
> @@ -0,0 +1,5 @@
> +# nothing to build
> +all:;

Drop ';'.

Show 16 quoted lines

> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> new file mode 100755
> index 0000000..8eff178
> --- /dev/null
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -0,0 +1,62 @@
> +#!/bin/sh
> +
> +test_description='Test diff-highlight'
> +
> +. ./test-diff-highlight.sh
> +. "$TEST_DIRECTORY"/test-lib.sh
> +
> +# PERL is required, but assumed to be present, although not necessarily modern
> +# some tests require 5.8
> +test_expect_success PERL 'name' 'true'

If the platform lacks PERL prerequisite, this will simply be skipped, and if the platform has it, it will always succeed.

I am not sure what you are trying to achieve by having this line here.

Show 5 quoted lines

> +test_expect_success 'diff-highlight does not highlight whole line' '
> +	dh_test \
> +		"aaa\nbbb\nccc\n" \
> +		"aaa\n000\nccc\n"
> +'

Hmm, does this express the desired outcome, or just document the current (possibly broken--I dunno) behaviour? The same question for the next one.

Show 5 quoted lines

> +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> +	dh_test \
> +		"aaa\nbbb\n" \
> +		"aaa\nb0b\nccc\n"
> +'

> +dh_test() {

Style: "dh_test () {"

The other functions in this file share the same.

Show 17 quoted lines

> +	dh_diff_test "$@" &&
> +	dh_commit_test "$@"
> +}
> +
> +dh_diff_test() {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +
> +	printf "$b" >file
> +	git diff file >diff.raw
> +
> +	if test $# -eq 3
> +	then
> +		# remove last newline
> +		head -n5 diff.raw | test_chomp_eof >diff.exp

A reader can see "remove last newline" by seeing test_chomp_eof and what it does without a comment, but it is totally unclear why you need to remove. The comment that says what it does without saying why it does it is useless.

Show 6 quoted lines

> +		printf "$3" >>diff.exp
> +	else
> +		cat diff.raw >diff.exp
> +	fi
> +
> +	<diff.raw "$DIFF_HIGHLIGHT" >diff.act &&

Even though this is technically kosher, I do not see a merit of deviating from the common practice of starting a line with the command, i.e.

	"$DIFF_HIGHLIGHT" <diff.raw >diff.actual

would be much easier to read.

> +	test -s diff.act &&

Why? If you always have the expected output that you are going to compare with, wouldn't that sufficient to do that test without this? Besides, having "test -s" means that you can never make sure that a certain pair of input does not show any changes. Perhaps drop it?

> +	diff diff.exp diff.act

Use test_cmp unless there is a strong reason why you shouldn't?

Show 8 quoted lines

> +}
> +
> +dh_commit_test() {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +	git commit -m"Add a file" >/dev/null

Avoid sticking a short-option to its argument, i.e.

    git commit -m "Add a file"

> +
> +	printf "$b" >file
> +	git commit -am"Update a file" >/dev/null

Likewise.

    git commit -a -m "Update a file"

The remainder of the file invites the same set of questions and comments you see for dh_diff_test() above, so I won't repeat them.

Thanks.