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

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

Brian Henderson [off-list ref] writes:

<snip>

quoted

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

quoted

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

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

quoted

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

quoted

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

quoted

+	diff diff.exp diff.act

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

quoted

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

quoted

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