Re: [PATCH 1/3] diff-highlight: add some tests.
> On 16 Aug 2016, at 22:48, Junio C Hamano wrote: > > Lars Schneider writes: > >>> On 30 Jul 2016, at 17:11, Brian Henderson wrote: >>> >>> --- >>> contrib/diff-highlight/Makefile | 5 ++ >>> contrib/diff-highlight/t/Makefile| 19 +++ >>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++ >>> contrib/diff-highlight/t/test-diff-highlight.sh | 69 >>> >>> 4 files changed, 156 insertions(+) >>> create mode 100644 contrib/diff-highlight/Makefile >>> create mode 100644 contrib/diff-highlight/t/Makefile >>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh >>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh >>> >>> diff --git a/contrib/diff-highlight/Makefile >>> b/contrib/diff-highlight/Makefile >> >> Would it make sense to add the contrib tests to the Travis-CI build, too? >> https://travis-ci.org/git/git/branches > > The more the merrier ;-)? I do not think of a downside, especially > if you are adding it as a separate thing that comes after the main > test, or for even better isolation, an entirely separate job. OK, if I will post a patch (might take a while). > By the way, how flaky are existing tests? Are people actively > following breakage reports? Good question - I was curious about that, too. That's why I made a little website (only tested on Chrome, requires JS): https://larsxschneider.github.io/git-ci-stats/ There you can see all builds for all branches including their failure reason. In general I would say flakiness is no issue anymore. I don't know who else is looking at the breakage reports but I do. That's the reason for my "next" breakage reports in the past: http://public-inbox.org/git/a6131c47-3230-4ec4-b3f6-b2507c937...@gmail.com/ http://public-inbox.org/git/4877318e-3cbf-4c87-b24d-aae35c427...@gmail.com/ http://public-inbox.org/git/fb76544f-16f7-45ca-9649-fd62ee44b...@gmail.com/ ... Cheers, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] diff-highlight: add some tests.
Lars Schneider writes: >> On 30 Jul 2016, at 17:11, Brian Henderson wrote: >> >> --- >> contrib/diff-highlight/Makefile | 5 ++ >> contrib/diff-highlight/t/Makefile| 19 +++ >> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++ >> contrib/diff-highlight/t/test-diff-highlight.sh | 69 >> >> 4 files changed, 156 insertions(+) >> create mode 100644 contrib/diff-highlight/Makefile >> create mode 100644 contrib/diff-highlight/t/Makefile >> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh >> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh >> >> diff --git a/contrib/diff-highlight/Makefile >> b/contrib/diff-highlight/Makefile > > Would it make sense to add the contrib tests to the Travis-CI build, too? > https://travis-ci.org/git/git/branches The more the merrier ;-)? I do not think of a downside, especially if you are adding it as a separate thing that comes after the main test, or for even better isolation, an entirely separate job. By the way, how flaky are existing tests? Are people actively following breakage reports? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] diff-highlight: add some tests.
> On 30 Jul 2016, at 17:11, Brian Henderson wrote: > > --- > contrib/diff-highlight/Makefile | 5 ++ > contrib/diff-highlight/t/Makefile| 19 +++ > contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++ > contrib/diff-highlight/t/test-diff-highlight.sh | 69 > 4 files changed, 156 insertions(+) > create mode 100644 contrib/diff-highlight/Makefile > create mode 100644 contrib/diff-highlight/t/Makefile > create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh > create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh > > diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile Would it make sense to add the contrib tests to the Travis-CI build, too? https://travis-ci.org/git/git/branches - Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] diff-highlight: add some tests.
On Wed, Aug 10, 2016 at 08:56:35AM +, Eric Wong wrote: > Typically, we expect a reroll in a few days, and I guess there's > no rush (so you can squash your comment patch in addressing > Junio's concern into 3/3). > > Thanks. thanks, (slowly) working on an update. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] diff-highlight: add some tests.
Brian Henderson writes: > On Wed, Aug 10, 2016 at 08:56:35AM +, Eric Wong wrote: > > > >> Typically, we expect a reroll in a few days, and I guess there's >> no rush (so you can squash your comment patch in addressing >> Junio's concern into 3/3). >> >> Thanks. > > thanks, (slowly) working on an update. Thanks, both, for keeping the ball rolling ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] diff-highlight: add some tests.
Brian Henderson wrote:
Hi Brian,
A few minor portability/style nits below, but contrib/ probably
(still?) has laxer rules than the rest of git...
I think we still require Signed-off-by lines in contrib,
though...
> +++ b/contrib/diff-highlight/t/Makefile
> @@ -0,0 +1,19 @@
> +-include ../../../config.mak.autogen
> +-include ../../../config.mak
> +
> +T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
> +
> +all: test
> +test: $(T)
> +
> +.PHONY: help clean all test $(T)
> +
> +help:
> + @echo 'Run "$(MAKE) test" to launch test scripts'
> + @echo 'Run "$(MAKE) clean" to remove trash folders'
> +
> +$(T):
> + @echo "*** $@ ***"; sh $@ $(GIT_TEST_OPTS)
Probably:
@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
like we do in t/Makefile in case we need to override 'sh'.
> +
> +clean:
> + $(RM) -r 'trash directory'.*
> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> new file mode 100644
> index 000..ca7605f
> --- /dev/null
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2016
IANAL, but I think your name (or who you represent) needs to be
in the copyright.
> +
> +test_description='Test diff-highlight'
> +
> +. ./test-diff-highlight.sh
> +. $TEST_DIRECTORY/test-lib.sh
TEST_DIRECTORY ought to be quoted since it could contain
shell-unfriendly chars (we intentionally name 'trash directory'
this way to trigger errors).
> +
> +# PERL is required, but assumed to be present, although not necessarily
> modern
> +# some tests require 5.8
> +
> +test_expect_success 'diff-highlight highlightes the beginning of a line' '
You can declare a prereq for PERL::
test_expect_success PERL 'name' 'true'
And spelling: "highlights" (there's other instances of this, too)
> + dh_test \
> +"aaa\nbbb\nccc\n" \
> +"aaa\n0bb\nccc\n" \
> +"
We use tabs for shell indentation.
> diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh
> b/contrib/diff-highlight/t/test-diff-highlight.sh
> new file mode 100644
> index 000..9b26271
> --- /dev/null
> +++ b/contrib/diff-highlight/t/test-diff-highlight.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2016
> +
> +CURR_DIR=$(pwd)
> +TEST_OUTPUT_DIRECTORY=$(pwd)
> +TEST_DIRECTORY="$CURR_DIR"/../../../t
> +cmd=diff-highlight
> +CMD="$CURR_DIR"/../$cmd
Same comments as above on quoting pathnames and empty copyright.
> +
> +CW="\033[7m"
> +CR="\033[27m"
> +
> +export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR
> +
> +dh_test() {
> + dh_diff_test "$@"
> + dh_commit_test "$@"
> +}
> +
> +dh_diff_test() {
> + local a="$1" b="$2"
"local" is not a portable construct. It's common for
Debian/Ubuntu systems to use dash as /bin/sh instead of bash;
(dash is faster, and mostly sticks to POSIX)
The "devscripts" package in Debian/Ubuntu-based systems has a
handy "checkbashisms" tool for checking portability of shell
scripts.
> +
> + printf "$a" > file
> + git add file
> +
> + printf "$b" > file
> +
> + git diff file > diff.raw
Commands should be '&&'-chained here since any of them could fail
("git add"/printf/etc could run out of space or fail on disk errors)
Also, our redirect style is:command >file
without a space between '>' and destination.
See Documentation/CodingGuidelines for more details.
Unfortunately, the reasoning is not explained for '>NOSPACE'
and I'm not sure exactly why, either...
> + if test "$#" = 3
Better to use -eq for numeric comparisons: test $# -eq 3
Quoting $# doesn't seem necessary unless your shell is
hopelessly buggy :)
> + then
> +# remove last newline
> +head -n5 diff.raw | head -c -1 > diff.act
"head -c" isn't portable, fortunately Jeff hoisted it out for
use as test_copy_bytes in commit 48860819e8026
https://public-inbox.org/git/20160630090753.ga17...@sigill.intra.peff.net/
> +printf "$3" >> diff.act
> + else
> +cat diff.raw > diff.act
> + fi
> +
> + < diff.raw $CMD > diff.exp
$CMD probably needs to be quoted. However, by the time I got
here I had to ask myself: What is $CMD again?
A: Oh, look up at the top!
*shrug* My attention span is tiny and my fonts are gigantic.
Perhaps using:
DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
Would be more-readable?
> +
> + diff diff.exp diff.act
Maybe use test_cmp to avoid depending external diff.
(or "git diff -b --no-index" in your later test)
Same comments for the rest of the series, I think.
Typically, we expect a reroll in a few days, and I guess there's
no rush (so you can squash your comment patch in addressing
Junio's concern into 3/3).
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
