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

On Fri, Aug 19, 2016 at 10:51:23AM -0400, Jeff King wrote:

On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:

quoted

quoted

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.

Yeah, I'd agree this test would want the PERL prereq. It is not just
using perl for one-liners in support of the script; it is testing major
perl functionality that should be skipped if we do not have a modern
perl available.

quoted

Eric recommended adding this line, what do you think?

would `test_set_prereq PERL` be better?

test_set_prereq is for telling the test scripts that we _have_ perl, but
what I think this script wants to do is test "do we have perl?" and
abort otherwise. The way to do that is:

  if ! test_have_prereq PERL
  then
	skip_all='skipping diff-highlight tests; perl not available'
	test_done
  fi

quoted

quoted

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`

Yeah, that is an intentional behavior, and makes sense to test.

quoted

quoted

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?

Undesired behavior should generally not be tested for. It just makes
life harder for somebody when they make a change that violates it, and
they have to figure out "oh, but it's _good_ that I changed that, the
tests were wrong" (or more likely "I didn't fix it, but it's just broken
in a different way, and neither is preferable").

If you want to document known shortcomings, the best thing to do is show
what you'd _like_ to have happen, and mark it as test_expect_failure;
the test scripts show this as a known-breakage, and somebody later who
fixes it can flip the "failure" to "success".

-Peff

all very helpful, thanks!