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

From: Brian Henderson <henderson.bj@gmail.com>
Date: Aug 19, 2016, 14:42 UTC
Message-ID: <20160819144235.GA29458@tci.corp.yp.com>
In-Reply-To: <xmqqziob8b6i.fsf@gitster.mtv.corp.google.com>

On Wed, Aug 17, 2016 at 12:09:25PM -0700, Junio C Hamano wrote:

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

<snip>

Show 11 quoted lines

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

I originally didn't have this line, and my comment was referring to the t/README which says

   Even without the PERL prerequisite, tests can assume there is a
   usable perl interpreter at $PERL_PATH, though it need not be
   particularly modern.

There is current functionality in diff-highlight which requires at least perl 5.8 (the utf8 functions). I was going to add a test for this as well, but I'm not super comfy with multibyte chars.

Eric recommended adding this line, what do you think?

would `test_set_prereq PERL` be better?

Show 6 quoted lines

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

This (at least to me) is desired. See comment for `sub is_pair_interesting`

Show 10 quoted lines

> 
> Hmm, does this express the desired outcome, or just document the
> current (possibly broken--I dunno) behaviour?  The same question for
> the next one.
> 
> > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '
> > +	dh_test \
> > +		"aaa\nbbb\n" \
> > +		"aaa\nb0b\nccc\n"
> > +'

This is undesired behavior, but currently implemented for simplicity, see `sub show_hunk`

Do they need comments or something?

<snip>

Show 7 quoted lines

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

I was trying to address Eric's concern for `printf` or `git commit` et al failing. Also, this file will always be a diff, it just might not having any highlighting (so not empty?).

I'll take another stab.

Show 30 quoted lines

> 
> > +	diff diff.exp diff.act
> 
> Use test_cmp unless there is a strong reason why you shouldn't?
> 
> > +}
> > +
> > +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.

thanks for the feedback.