Re: $PATH pollution and t9902-completion.sh
On Thu, Dec 20, 2012 at 09:53:06PM +0100, Torsten Bögershausen wrote:
> (Good to learn about the comm command, thanks )
> What do we think about this:
>
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..82eeba7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -62,12 +62,16 @@ test_completion ()
> {
> if test $# -gt 1
> then
> - printf '%s\n' "$2" >expected
> + printf '%s\n' "$2" | sort >expected.sorted
> else
> - sed -e 's/Z$//' >expected
> + sed -e 's/Z$//' | sort >expected.sorted
> fi &&
> run_completion "$1" &&
> - test_cmp expected out
> + sort actual.sorted &&
> + >empty &&
> + comm -23 expected.sorted actual.sorted >actual &&
> + test_cmp empty actual &&
> + rm empty actual
I like test_* helpers that tell you what happened on error, but this
output will be kind of a weird diff (it is a diff showing things you
expected to have but did not get as additions, and no mention of things
you expected and got).
The output is human readable, so I think it is perfectly OK to print a
message about what is going on (we do this in test_expect_code, for
example). Something like:
test_fully_contains() {
sort "$1" >expect.sorted &&
sort "$2" >actual.sorted &&
comm -23 expect.sorted actual.sorted >missing
test -f missing -a ! -s missing &&
rm -f expect.sorted actual.sorted missing &&
return 0
{
echo "test_fully_contains: some lines were missing"
echo "expected:"
sed 's/^/ /' <"$1"
echo "actual:"
sed 's/^/ /' <"$2"
echo "missing:"
sed 's/^/ /' &2
return 1
}
Though come to think of it, just showing the diff between expect.sorted
and actual.sorted would probably be enough. It would show the missing
elements as deletions, the found elements as common lines, and the
"extra" elements as additions (which are not an error, but when you are
debugging, might be useful to know about).
-Peff
--
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: $PATH pollution and t9902-completion.sh
Torsten Bögershausen writes:
> On 20.12.12 21:01, Jeff King wrote:
>> +test_fully_contains () {
>>> + sort "$1" >expect.sorted &&
>>> + sort "$2" >actual.sorted &&
>>> + test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
>>> +}
>
> (Good to learn about the comm command, thanks )
> What do we think about this:
Three points to consider.
* Do _all_ tests that use test_completion not care about the actual
order of choices that come out of the completion machinery?
* Do _all_ tests that use test_completion not care about having
extra choice, or is the command name completion the only odd
case?
* Is this still as easy to extend as the original to conditionally
verify that all extra output share the same prefix?
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..82eeba7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -62,12 +62,16 @@ test_completion ()
> {
> if test $# -gt 1
> then
> - printf '%s\n' "$2" >expected
> + printf '%s\n' "$2" | sort >expected.sorted
> else
> - sed -e 's/Z$//' >expected
> + sed -e 's/Z$//' | sort >expected.sorted
> fi &&
> run_completion "$1" &&
> - test_cmp expected out
> + sort actual.sorted &&
> + >empty &&
> + comm -23 expected.sorted actual.sorted >actual &&
> + test_cmp empty actual &&
> + rm empty actual
> }
>
> # Test __gitcomp.
--
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: $PATH pollution and t9902-completion.sh
On 20.12.12 21:01, Jeff King wrote:
> +test_fully_contains () {
>> +sort "$1" >expect.sorted &&
>> +sort "$2" >actual.sorted &&
>> +test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
>> +}
(Good to learn about the comm command, thanks )
What do we think about this:
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..82eeba7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -62,12 +62,16 @@ test_completion ()
{
if test $# -gt 1
then
- printf '%s\n' "$2" >expected
+ printf '%s\n' "$2" | sort >expected.sorted
else
- sed -e 's/Z$//' >expected
+ sed -e 's/Z$//' | sort >expected.sorted
fi &&
run_completion "$1" &&
- test_cmp expected out
+ sort actual.sorted &&
+ >empty &&
+ comm -23 expected.sorted actual.sorted >actual &&
+ test_cmp empty actual &&
+ rm empty actual
}
# Test __gitcomp.
--
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: $PATH pollution and t9902-completion.sh
On Thu, Dec 20, 2012 at 11:55:45AM -0800, Junio C Hamano wrote:
> The beginning of such a change may look like the attached patch.
> [...]
> +test_fully_contains () {
> + sort "$1" >expect.sorted &&
> + sort "$2" >actual.sorted &&
> + test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
> +}
I like the direction. I suspect test_fully_contains could be used in a
lot of other places, too.
-Peff
--
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: $PATH pollution and t9902-completion.sh
Junio C Hamano writes:
> Jeff King writes:
>
>> 2. Loosen the test to look for the presence of "checkout", but not
>> fail when other items are present. Bonus points if it makes sure
>> that everything returned starts with "check".
>>
>> I think (2) is the ideal solution in terms of behavior, but writing it
>> may be more of a pain.
>
> Yeah, I think (2) is the way to go.
The beginning of such a change may look like the attached patch.
If we want to go for the bonus points, we would either add another
parameter "prefix" to the test_completion function, or introduce the
test_complete_command function that takes that prefix parameter, and
in addition to making sure lines from "expect" is fully contained in
the "actual", make sure that output from
comm -13 expect.sorted actual.sorted
all begin with that "prefix" string, perhaps with
grep -v "^$prefix"
or something. The test_fully_contains function needs to be renamed
if somebody goes that additional step.
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..5fab389 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,10 +54,16 @@ run_completion ()
__git_wrap__git_main && print_comp
}
+test_fully_contains () {
+ sort "$1" >expect.sorted &&
+ sort "$2" >actual.sorted &&
+ test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
+}
+
# Test high-level completion
# Arguments are:
# 1: typed text so far (cur)
-# 2: expected completion
+# 2: expected completion (if missing, this is read from stdin)
test_completion ()
{
if test $# -gt 1
@@ -67,7 +73,7 @@ test_completion ()
sed -e 's/Z$//' >expected
fi &&
run_completion "$1" &&
- test_cmp expected out
+ test_fully_contains expected out
}
# Test __gitcomp.
--
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: $PATH pollution and t9902-completion.sh
Jeff King writes: > 2. Loosen the test to look for the presence of "checkout", but not > fail when other items are present. Bonus points if it makes sure > that everything returned starts with "check". > > I think (2) is the ideal solution in terms of behavior, but writing it > may be more of a pain. Yeah, I think (2) is the way to go. -- 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: $PATH pollution and t9902-completion.sh
On 20.12.12 16:13, Adam Spiers wrote: > On Thu, Dec 20, 2012 at 2:55 PM, Jeff King wrote: >> On Mon, Dec 17, 2012 at 01:05:38AM +, Adam Spiers wrote: >>> t/t9902-completion.sh is currently failing for me because I happen to >>> have a custom shell-script called git-check-email in ~/bin, which is >>> on my $PATH. This is different to a similar-looking case reported >>> recently, which was due to an unclean working tree: >>> >>> http://thread.gmane.org/gmane.comp.version-control.git/208085 >>> >>> It's not unthinkable that in the future other tests could break for >>> similar reasons. Therefore it would be good to sanitize $PATH in the >>> test framework so that it cannot destabilize tests, although I am >>> struggling to think of a good way of doing this. Naively stripping >>> directories under $HOME would not protect against git "plugins" such >>> as the above being installed into places like /usr/bin. Thoughts? >> >> I've run into this, too. I think sanitizing $PATH is the wrong approach. >> The real problem is that the test is overly picky. Right now it is >> failing because you happen to have "check-email" in your $PATH, but it >> will also need to be adjusted when a true "check-email" command is added >> to git. >> >> I can think of two other options: >> >> 1. Make the test input more specific (e.g., looking for "checkou"). >> This doesn't eliminate the problem, but makes it less likely >> to occur. >> >> 2. Loosen the test to look for the presence of "checkout", but not >> fail when other items are present. Bonus points if it makes sure >> that everything returned starts with "check". >> >> I think (2) is the ideal solution in terms of behavior, but writing it >> may be more of a pain. > > I agree with all your points. Thanks for the suggestions. I volonteer for 1) (and we got for 2) later) -- 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: $PATH pollution and t9902-completion.sh
On Thu, Dec 20, 2012 at 2:55 PM, Jeff King wrote: > On Mon, Dec 17, 2012 at 01:05:38AM +, Adam Spiers wrote: >> t/t9902-completion.sh is currently failing for me because I happen to >> have a custom shell-script called git-check-email in ~/bin, which is >> on my $PATH. This is different to a similar-looking case reported >> recently, which was due to an unclean working tree: >> >> http://thread.gmane.org/gmane.comp.version-control.git/208085 >> >> It's not unthinkable that in the future other tests could break for >> similar reasons. Therefore it would be good to sanitize $PATH in the >> test framework so that it cannot destabilize tests, although I am >> struggling to think of a good way of doing this. Naively stripping >> directories under $HOME would not protect against git "plugins" such >> as the above being installed into places like /usr/bin. Thoughts? > > I've run into this, too. I think sanitizing $PATH is the wrong approach. > The real problem is that the test is overly picky. Right now it is > failing because you happen to have "check-email" in your $PATH, but it > will also need to be adjusted when a true "check-email" command is added > to git. > > I can think of two other options: > > 1. Make the test input more specific (e.g., looking for "checkou"). > This doesn't eliminate the problem, but makes it less likely > to occur. > > 2. Loosen the test to look for the presence of "checkout", but not > fail when other items are present. Bonus points if it makes sure > that everything returned starts with "check". > > I think (2) is the ideal solution in terms of behavior, but writing it > may be more of a pain. I agree with all your points. Thanks for the suggestions. -- 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: $PATH pollution and t9902-completion.sh
On Mon, Dec 17, 2012 at 01:05:38AM +, Adam Spiers wrote: > t/t9902-completion.sh is currently failing for me because I happen to > have a custom shell-script called git-check-email in ~/bin, which is > on my $PATH. This is different to a similar-looking case reported > recently, which was due to an unclean working tree: > > http://thread.gmane.org/gmane.comp.version-control.git/208085 > > It's not unthinkable that in the future other tests could break for > similar reasons. Therefore it would be good to sanitize $PATH in the > test framework so that it cannot destabilize tests, although I am > struggling to think of a good way of doing this. Naively stripping > directories under $HOME would not protect against git "plugins" such > as the above being installed into places like /usr/bin. Thoughts? I've run into this, too. I think sanitizing $PATH is the wrong approach. The real problem is that the test is overly picky. Right now it is failing because you happen to have "check-email" in your $PATH, but it will also need to be adjusted when a true "check-email" command is added to git. I can think of two other options: 1. Make the test input more specific (e.g., looking for "checkou"). This doesn't eliminate the problem, but makes it less likely to occur. 2. Loosen the test to look for the presence of "checkout", but not fail when other items are present. Bonus points if it makes sure that everything returned starts with "check". I think (2) is the ideal solution in terms of behavior, but writing it may be more of a pain. -Peff -- 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
$PATH pollution and t9902-completion.sh
t/t9902-completion.sh is currently failing for me because I happen to have a custom shell-script called git-check-email in ~/bin, which is on my $PATH. This is different to a similar-looking case reported recently, which was due to an unclean working tree: http://thread.gmane.org/gmane.comp.version-control.git/208085 It's not unthinkable that in the future other tests could break for similar reasons. Therefore it would be good to sanitize $PATH in the test framework so that it cannot destabilize tests, although I am struggling to think of a good way of doing this. Naively stripping directories under $HOME would not protect against git "plugins" such as the above being installed into places like /usr/bin. Thoughts? -- 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
