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

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

Show 5 quoted lines

> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and passes
> +# that through diff-highlight. The optional 3rd parameter is the expected
> +# output of diff-highlight minus the diff/commit header. Don't include a 3rd
> +# parameter if diff-highlight is stupposed to leave the input unmodified.

Good explanation, but it fails to say one crucial thing. The third parameter is directly fed to printf and not to "printf '%s' $3", hence any '%' you expect to see in diff output need to be doubled to protect it.

Show 20 quoted lines

> +# see dh_test for usage
> +dh_diff_test () {
> +	a="$1" b="$2"
> +
> +	printf "$a" >file
> +	git add file
> +
> +	printf "$b" >file
> +	git diff file >diff.raw
> +
> +	if test $# -ge 3
> +	then
> +		# Add the diff header to the expected file
> +		# we remove the trailing newline to make the test a little more readable
> +		# this means $3 should start with a newline
> +		head -n5 diff.raw | test_chomp_eof >diff.exp
> +		printf "$3" >>diff.exp
> +	else
> +		cat diff.raw >diff.exp
> +	fi

Sorry, but I do not understand why you hardcode -n5 or why you need chomp-eof.

I _think_ you are expecting "git diff file" to start with

    diff --git a/file b/file
    index fffffff..fffffff 100644
    --- a/file
    +++ b/file
    @@ -l,k +m,n @@ comments

and want to grab everything before and including this first hunk header. A more future-proof way to do the "stop at the first occurrence of this" (this comment applies to -n11 we see below) is

	sed -e '/^@@/q' diff.raw

As to chomp-eof, I still do not see the point, especially with the new comment you added: "this means $3 should start with newline".

You are forcing the caller to have an extra empty line at the beginnig ONLY because you do this "chomp" thing. Otherwise the callers do not need to.

Unless you actually mean something else by the new comment, e.g. "I think the callers look prettier if it begins with a newline, so we compensate for that by removing the end of line from what comes before it", that is. If "this means $3 should" is what it sounds like, i.e. imposing an unnatural constraint on the callers of this helper function, then the helper can do this

	printf "\n$3" >>diff.exp

so that the callers do not have to worry about it.

If "I think the caller looks prettier if it begins with a newline" is the true motivation, then "this means $3 should start..." needs to be rephrased. And as to the implementation of the helper, I think

	{
		sed -e '/^@@/q' diff.raw
		printf "$3" | sed -e 1d
	} >diff.expected

may be easier to see what is going on.

> +
> +	"$DIFF_HIGHLIGHT" <diff.raw >diff.act &&

> +	# check that at least one of the files is not empty (any of the above
> +	# commands could have failed resulting in an empty file)
> +	test -s diff.act &&

A more established way in our test suite to ensure that "any of the above commands could have failed" is caught is to &&-chain all the commands, like this:

	a=$1 b=$2 &&
	printf "$a" >file && git add file &&
        printf "$b" >file && git diff file >diff.raw &&
        if test $# = 3
        then
        	...
	else
        	...
	fi &&

> +test_done
> +
> +# vim: set noet

We tend to avoid cluttering the source with editor specific insns like this.

Stepping back a bit. Because you are only interested in making sure the body of the diff match what is expected, it may be a better alternative approach to make the comparison _after_ stripping the headers from the actual output with the expected (which you do not have headers for), rather than comparing the expected (with fake headers added in as necessary) and the actual output with headers, i.e.

	git diff file >diff.raw &&
	if test $# = 3
	then
		printf "$3"
	else
	        sed -e '1,/^@@/d' <diff.raw
	fi >diff.expected &&
	"$DIFF_HIGHLIGHT" <diff.raw >diff.highlight &&
	sed -e '1,/^@@/d' <diff.highlight >diff.actual &&
	test_cmp diff.expected diff.actual

or something like that.

That does not address "is it a good idea to require an empty line at the beginning of $3"? at all, though. If you want to require a blank in front of "$3", the "printf" needs to be tweaked to somehow strip it.