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?
@@ -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_successPERL'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.
+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.
+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.
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.
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?
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.
From: Brian Henderson <hidden> Date: 2016-08-19 14:42:53
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?
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.
From: Jeff King <hidden> Date: 2016-08-19 14:52:17
On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:
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.
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
+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
+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
From: Brian Henderson <hidden> Date: 2016-08-19 15:15:03
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
@@ -85,10 +85,50 @@ dh_commit_test () {test_cmpcommit.expcommit.act}+# dh_test_setup_history takes a series (3) of changes and generates a contrived graph+# of commits and merges+dh_test_setup_history(){+a="$1"b="$2"c="$3"++printf"$a">file&&+gitaddfile&&+gitcommit-m"Add a file"&&++printf"$b">file&&+gitcommit-am"Update a file"&&++gitcheckout-bbranch&&+printf"$c">file&&+gitcommit-am"Update a file on branch"&&++gitcheckoutmaster&&+printf"$a">file&&+gitcommit-am"Update a file again"&&++gitcheckoutbranch&&+printf"$b">file&&+gitcommit-am"Update a file similar to master"&&++gitmergemaster&&+gitcheckoutmaster&&+gitmergebranch--no-ff+}++ test_chomp_eof(){"$PERL_PATH"-pe'chomp if eof'}+left_trim(){+"$PERL_PATH"-pe's/^\s+//'+}++trim_graph_el(){+# graphs start with * or |+# followed by a space or / or \+"$PERL_PATH"-pe's@^((\*|\|)( |/|\\))+@@'+}+ test_expect_success'diff-highlight highlights the beginning of a line''dh_test\"aaa\nbbb\nccc\n"\
@@ -145,6 +185,20 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '# TODO add multi-byte test+test_expect_failure'diff-highlight works with the --graph option''+dh_test_setup_history\+"aaa\nbbb\nccc\n"\+"aaa\n0bb\nccc\n"\+"aaa\nb0b\nccc\n"&&++# topo-order so that the order of the commits is the same as with --graph+# trim graph elements so we can do a diff+# trim leading space because our trim_graph_el is not perfect+gitlog-p--topo-order|"$DIFF_HIGHLIGHT"|left_trim>graph.exp&&+gitlog-p--graph|"$DIFF_HIGHLIGHT"|trim_graph_el|left_trim>graph.act&&+test_cmpgraph.expgraph.act+'+ test_done# vim: set noet
@@ -0,0 +1,150 @@+#!/bin/sh++test_description='Test diff-highlight'++CURR_DIR=$(pwd)+TEST_OUTPUT_DIRECTORY=$(pwd)+TEST_DIRECTORY="$CURR_DIR"/../../../t+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight++CW="\033[7m"# white+CR="\033[27m"# reset++."$TEST_DIRECTORY"/test-lib.sh++if!test_have_prereqPERL+then+skip_all='skipping diff-highlight tests; perl not available'+test_done+fi++# 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.+dh_test(){+dh_diff_test"$@"&&+dh_commit_test"$@"+}++# see dh_test for usage+dh_diff_test(){+a="$1"b="$2"++printf"$a">file+gitaddfile++printf"$b">file+gitdifffile>diff.raw++iftest$#-ge3+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-n5diff.raw|test_chomp_eof>diff.exp+printf"$3">>diff.exp+else+catdiff.raw>diff.exp+fi++"$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-sdiff.act&&+test_cmpdiff.expdiff.act+}++# see dh_test for usage+dh_commit_test(){+a="$1"b="$2"++printf"$a">file+gitaddfile+gitcommit-m"Add a file">/dev/null++printf"$b">file+gitcommit-am"Update a file">/dev/null++gitshow>commit.raw++iftest$#-ge3+then+# same as dh_diff_test+head-n11commit.raw|test_chomp_eof>commit.exp+printf"$3">>commit.exp+else+catcommit.raw>commit.exp+fi++"$DIFF_HIGHLIGHT"<commit.raw>commit.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-scommit.act&&+test_cmpcommit.expcommit.act+}++test_chomp_eof(){+"$PERL_PATH"-pe'chomp if eof'+}++test_expect_success'diff-highlight highlights the beginning of a line''+dh_test\+"aaa\nbbb\nccc\n"\+"aaa\n0bb\nccc\n"\+"+aaa+-${CW}b${CR}bb++${CW}0${CR}bb+ccc+"+'++test_expect_success'diff-highlight highlights the end of a line''+dh_test\+"aaa\nbbb\nccc\n"\+"aaa\nbb0\nccc\n"\+"+aaa+-bb${CW}b${CR}++bb${CW}0${CR}+ccc+"+'++test_expect_success'diff-highlight highlights the middle of a line''+dh_test\+"aaa\nbbb\nccc\n"\+"aaa\nb0b\nccc\n"\+"+aaa+-b${CW}b${CR}b++b${CW}0${CR}b+ccc+"+'++test_expect_success'diff-highlight does not highlight whole line''+dh_test\+"aaa\nbbb\nccc\n"\+"aaa\n000\nccc\n"+'++test_expect_failure'diff-highlight highlights mismatched hunk size''+dh_test\+"aaa\nbbb\n"\+"aaa\nb0b\nccc\n"\+"+aaa+-b${CW}b${CR}b++b${CW}0${CR}b++ccc+"+'++# TODO add multi-byte test++test_done++# vim: set noet
@@ -21,6 +21,10 @@ my $RESET = "\x1b[m"; my $COLOR = qr/\x1b\[[0-9;]*m/; my $BORING = qr/$COLOR|\s/;+# The patch portion of git log -p --graph should only ever have preceding | and+# not / or \ as merge history only shows up on the commit line.+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;+ my @removed; my @added; my $in_hunk;
@@ -46,7 +50,7 @@ while (<>) { @added = (); print;- $in_hunk = /^$COLOR*[\@ ]/;+ $in_hunk = /^$GRAPH*$COLOR*[\@ ]/; } # Most of the time there is enough output to keep things streaming,
@@ -163,6 +167,9 @@ sub highlight_pair { } }+# we split either by $COLOR or by character. This has the side effect of+# leaving in graph cruft. It works because the graph cruft does not contain "-"+# or "+" sub split_line { local $_ = shift; return utf8::decode($_) ?
@@ -185,7 +185,7 @@ test_expect_failure 'diff-highlight highlights mismatched hunk size' '# TODO add multi-byte test-test_expect_failure'diff-highlight works with the --graph option''+test_expect_success'diff-highlight works with the --graph option''dh_test_setup_history\"aaa\nbbb\nccc\n"\"aaa\n0bb\nccc\n"\