Re: [PATCH 2/3] test: improve rebase -q test
On Sun, Jun 09, 2013 at 11:30:01AM -0700, Junio C Hamano wrote: > -- >8 -- > Subject: [PATCH] test: test_output_must_be_empty helper > > There are quite a lot places where an output file is expected to be > empty, and we fail the test when it is not. The output from running > the test script with -i -v can be helped if we showed the unexpected > contents at that point. > > We could of course do > > >expected.empty && test_cmp expected.empty actual > > but this is commmon enough to be done with a dedicated helper. Thanks, I think this improves the readability of the test suite (and its output when there are failures). You can also do: test_cmp /dev/null actual for the same effect, but I guess the diff is not all that interesting (by definition, it would consist only of added lines, and you are already showing them, so it would not be an improvement). -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: [PATCH 2/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 09:07:06PM +0200, Johannes Sixt wrote: > Am 10.06.2013 19:27, schrieb SZEDER Gábor: > > My main motivation is that, like in your example, in the bash prompt > > tests I only have to check a single line of output, but because of > > debuggability I always did: > > > > echo "(master)" >expected > > __git_ps1 >actual > > test_cmp expected actual > > Chained by &&, I presume. Sure. > > With such a helper function this could be reduced to a single line: > > > > test_string_equal "(master)" "$(__git_ps1)" > > > > without loss of functionality > > Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It > probably doesn't matter here, but it certainly does if the command is > $(git rev-parse foo) or similar.) Ouch, indeed. Yeah, the exit code doesn't matter for the prompt tests (I mean for __git_ps1() tests, but maybe it does matter for some __gitdir() tests), but I suppose it does matter everywhere else where the same construct is used. We could still do actual="$(git foo)" && test_string_equal "good" "$actual" to preserve and check the exit code, and this is still one line shorter, but overall not that appealing anymore. However. The git command's exit code is lost the same way in 'test good = $(git foo)' constructs, too, and plenty of such constructs are all over the test suite. Shouldn't we avoid using such constucts then? Gábor -- 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 2/3] test: improve rebase -q test
SZEDER Gábor writes: > With such a helper function this could be reduced to a single line: > > test_string_equal "(master)" "$(__git_ps1)" > > without loss of functionality or debuggability, because in case of a > failure it would output something like this (bikesheddable, of > course): > > Error: > expected: "(master)" > got: "((deadbeef...))" Yeah, that looks sensible. -- 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 2/3] test: improve rebase -q test
Am 10.06.2013 19:27, schrieb SZEDER Gábor: > My main motivation is that, like in your example, in the bash prompt > tests I only have to check a single line of output, but because of > debuggability I always did: > > echo "(master)" >expected > __git_ps1 >actual > test_cmp expected actual Chained by &&, I presume. > With such a helper function this could be reduced to a single line: > > test_string_equal "(master)" "$(__git_ps1)" > > without loss of functionality Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It probably doesn't matter here, but it certainly does if the command is $(git rev-parse foo) or similar.) -- Hannes -- 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 2/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 08:56:58AM -0700, Junio C Hamano wrote:
> SZEDER Gábor writes:
>
> > On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
> >> There
> >> will not be a need for test_string_must_be_empty() just like there's
> >> no need for test_string_cmp().
> >
> > Actually, if there were a test_string_cmp(), that would be the test
> > helper function I used most often.
>
> Hmm, there indeed are quite a many "At this point, the variable's
> value must be this" in the test scripts. With things like this:
>
> t/t0002-gitfile.sh: test "$SHA" = "$(git rev-list HEAD)"
>
> we can go to the trash directory upon seeing a failure to run the
> command used on the RHS, but the value in $SHA is cumbersome to find
> out (either running it under sh -x or insert an extra echo before
> it), so such a helper function may be useful.
>
> Do you really need a general comparison ("does A sort before B") or
> just equality? If the latter, test_string_equal (or even
> string_equal) might be a better name for it.
Yeah, I need only equality. Or at least it would be nice to have.
My main motivation is that, like in your example, in the bash prompt
tests I only have to check a single line of output, but because of
debuggability I always did:
echo "(master)" >expected
__git_ps1 >actual
test_cmp expected actual
With such a helper function this could be reduced to a single line:
test_string_equal "(master)" "$(__git_ps1)"
without loss of functionality or debuggability, because in case of a
failure it would output something like this (bikesheddable, of
course):
Error:
expected: "(master)"
got: "((deadbeef...))"
And perhaps with a description as an optional third argument to help
identify the failed check if multiple such checks are done in a single
test, e.g. the test_rev_parse() helper in t/t1501-worktree.sh, 'setup:
helper for testing rev-parse', which could be shortened as:
test_string_equal "$1" "$(git rev-parse --is-bare-repository)" "bare"
test_string_equal "$2" "$(git rev-parse --is-inside-git-dir)" "gitdir"
test_string_equal "$3" "$(git rev-parse --is-inside-work-tree)" "worktree"
and if something goes wrong we'd get:
Error: worktree
expected: "true"
got: "false"
Perhaps I could find some time in the days ahead to give it a go.
Gábor
--
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 2/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 10:56 AM, Junio C Hamano wrote: > By the way, test_cmp() is a replacement for the "cmp" command and > that is why it does not have "file" in its name. That's an irrelevant implementation detail. But if you want to be driven the the implementation, call it test_zero(). -- Felipe Contreras -- 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 2/3] test: improve rebase -q test
SZEDER Gábor writes:
> On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
>> There
>> will not be a need for test_string_must_be_empty() just like there's
>> no need for test_string_cmp().
>
> Actually, if there were a test_string_cmp(), that would be the test
> helper function I used most often.
Hmm, there indeed are quite a many "At this point, the variable's
value must be this" in the test scripts. With things like this:
t/t0002-gitfile.sh: test "$SHA" = "$(git rev-list HEAD)"
we can go to the trash directory upon seeing a failure to run the
command used on the RHS, but the value in $SHA is cumbersome to find
out (either running it under sh -x or insert an extra echo before
it), so such a helper function may be useful.
Do you really need a general comparison ("does A sort before B") or
just equality? If the latter, test_string_equal (or even
string_equal) might be a better name for it.
By the way, test_cmp() is a replacement for the "cmp" command and
that is why it does not have "file" in its name.
--
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 2/3] test: improve rebase -q test
On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote: > There > will not be a need for test_string_must_be_empty() just like there's > no need for test_string_cmp(). Actually, if there were a test_string_cmp(), that would be the test helper function I used most often. -- 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 2/3] test: improve rebase -q test
"Philip Oakley" writes: > While folks do use such simplistic names, given that the patch had > many call sites, I do think Filipe's short name would quickly become > the accepted test name and not cause any great difficulties. OK. -- 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 2/3] test: improve rebase -q test
From: "Felipe Contreras"
Sent: Sunday, June 09, 2013 8:33 PM
On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano
wrote:
Felipe Contreras writes:
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano
wrote:
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -606,6 +606,18 @@ test_cmp() {
$GIT_TEST_CMP "$@"
}
+# Check if the file expected to be empty is indeed empty, and
barfs
+# otherwise.
+
+test_output_must_be_empty () {
Why such a big name? test_empty() does the trick.
Primarily in order to avoid that exact name "test_empty" that others
may want to use for a helper to check that the contents of a string
variable is empty.
Which is never going to happen.
While folks do use such simplistic names, given that the patch had many
call sites, I do think Filipe's short name would quickly become the
accepted test name and not cause any great difficulties.
regards
Philip
--
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 2/3] test: improve rebase -q test
On Sun, Jun 9, 2013 at 3:35 PM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano wrote:
>>> Felipe Contreras writes:
>>>
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano wrote:
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -606,6 +606,18 @@ test_cmp() {
> $GIT_TEST_CMP "$@"
> }
>
> +# Check if the file expected to be empty is indeed empty, and barfs
> +# otherwise.
> +
> +test_output_must_be_empty () {
Why such a big name? test_empty() does the trick.
>>>
>>> Primarily in order to avoid that exact name "test_empty" that others
>>> may want to use for a helper to check that the contents of a string
>>> variable is empty.
>>
>> Which is never going to happen.
>
> For anything, a failure from
>
> test -z "$mustbeemptystring"
>
> in the test suite is much harder to diagnose because there is
> nothing left in the trash directory to inspect, as opposed to
>
> test ! -s "$mustbeemptyfile"
>
> where you can just go there and inspect yourself.
Except that it's usually gone. And I challenge you to find a instance
where there's a test -z "$mustbeemptystring" that throws a test
failure. It will take you time to find it (if there's any).
Moreover, by that rationale, we should call test_cmp, test_file_cmp,
but there's no need, because that's rarely needed (if at all). There
will not be a need for test_string_must_be_empty() just like there's
no need for test_string_cmp().
--
Felipe Contreras
--
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 2/3] test: improve rebase -q test
Felipe Contreras writes:
> On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano wrote:
>> Felipe Contreras writes:
>>
>>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano wrote:
>>>
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -606,6 +606,18 @@ test_cmp() {
$GIT_TEST_CMP "$@"
}
+# Check if the file expected to be empty is indeed empty, and barfs
+# otherwise.
+
+test_output_must_be_empty () {
>>>
>>> Why such a big name? test_empty() does the trick.
>>
>> Primarily in order to avoid that exact name "test_empty" that others
>> may want to use for a helper to check that the contents of a string
>> variable is empty.
>
> Which is never going to happen.
For anything, a failure from
test -z "$mustbeemptystring"
in the test suite is much harder to diagnose because there is
nothing left in the trash directory to inspect, as opposed to
test ! -s "$mustbeemptyfile"
where you can just go there and inspect yourself.
--
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 2/3] test: improve rebase -q test
On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano wrote:
>>
>>> --- a/t/test-lib-functions.sh
>>> +++ b/t/test-lib-functions.sh
>>> @@ -606,6 +606,18 @@ test_cmp() {
>>> $GIT_TEST_CMP "$@"
>>> }
>>>
>>> +# Check if the file expected to be empty is indeed empty, and barfs
>>> +# otherwise.
>>> +
>>> +test_output_must_be_empty () {
>>
>> Why such a big name? test_empty() does the trick.
>
> Primarily in order to avoid that exact name "test_empty" that others
> may want to use for a helper to check that the contents of a string
> variable is empty.
Which is never going to happen.
--
Felipe Contreras
--
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 2/3] test: improve rebase -q test
Felipe Contreras writes:
> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano wrote:
>
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -606,6 +606,18 @@ test_cmp() {
>> $GIT_TEST_CMP "$@"
>> }
>>
>> +# Check if the file expected to be empty is indeed empty, and barfs
>> +# otherwise.
>> +
>> +test_output_must_be_empty () {
>
> Why such a big name? test_empty() does the trick.
Primarily in order to avoid that exact name "test_empty" that others
may want to use for a helper to check that the contents of a string
variable is empty. test-file-must-be-empty is a bit shorter and
also good for that purpose; I do not think we want to go any shorter
than that.
--
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 2/3] test: improve rebase -q test
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano wrote:
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -606,6 +606,18 @@ test_cmp() {
> $GIT_TEST_CMP "$@"
> }
>
> +# Check if the file expected to be empty is indeed empty, and barfs
> +# otherwise.
> +
> +test_output_must_be_empty () {
Why such a big name? test_empty() does the trick.
> + if test -s "$1"
> + then
> + echo "Bad: test_output_must_be_empty '$1', but has the
> following."
echo "'$1' is not empty, it contains:"
Cheers.
--
Felipe Contreras
--
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 2/3] test: improve rebase -q test
Duy Nguyen writes:
> On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
> wrote:
>> Let's show the output so it's clear why it failed.
>
> I think you can always look into trash-directory.t3400 and figure why.
> But if you show this, I think you should use test_cmp to make it clear
> what is not wanted. Something like
>
> : >expected &&
> test_cmp expected output.out
There are quite many places that do this "output must be empty" in
the test suite, so it may deserve a special case perhaps like this
one.
-- >8 --
Subject: [PATCH] test: test_output_must_be_empty helper
There are quite a lot places where an output file is expected to be
empty, and we fail the test when it is not. The output from running
the test script with -i -v can be helped if we showed the unexpected
contents at that point.
We could of course do
>expected.empty && test_cmp expected.empty actual
but this is commmon enough to be done with a dedicated helper.
Signed-off-by: Junio C Hamano
---
t/t0040-parse-options.sh | 46 +--
t/t3400-rebase.sh | 2 +-
t/t3903-stash.sh | 10 +-
t/t5521-pull-options.sh | 26
t/t5702-clone-options.sh | 2 +-
t/t7102-reset.sh | 2 +-
t/t7400-submodule-basic.sh| 6 +++---
t/t9402-git-cvsserver-refs.sh | 12 +--
t/test-lib-functions.sh | 12 +++
9 files changed, 65 insertions(+), 53 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 244a43c..e2f5be0 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -50,7 +50,7 @@ EOF
test_expect_success 'test help' '
test_must_fail test-parse-options -h > output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_i18ncmp expect output
'
@@ -75,7 +75,7 @@ check() {
shift &&
sed "s/^$what .*/$what $expect/" expect &&
test-parse-options $* >output 2>output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
}
@@ -86,7 +86,7 @@ check_i18n() {
shift &&
sed "s/^$what .*/$what $expect/" expect &&
test-parse-options $* >output 2>output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_i18ncmp expect output
}
@@ -99,7 +99,7 @@ check_unknown() {
esac &&
cat expect.err >>expect &&
test_must_fail test-parse-options $* >output 2>output.err &&
- test ! -s output &&
+ test_output_must_be_empty output &&
test_cmp expect output.err
}
@@ -112,7 +112,7 @@ check_unknown_i18n() {
esac &&
cat expect.err >>expect &&
test_must_fail test-parse-options $* >output 2>output.err &&
- test ! -s output &&
+ test_output_must_be_empty output &&
test_i18ncmp expect output.err
}
@@ -149,7 +149,7 @@ test_expect_success 'short options' '
test-parse-options -s123 -b -i 1729 -b -vv -n -F my.file \
> output 2> output.err &&
test_cmp expect output &&
- test ! -s output.err
+ test_output_must_be_empty output.err
'
cat > expect << EOF
@@ -168,7 +168,7 @@ test_expect_success 'long options' '
test-parse-options --boolean --integer 1729 --boolean --string2=321 \
--verbose --verbose --no-dry-run --abbrev=10 --file fi.le\
--obsolete > output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
'
@@ -199,7 +199,7 @@ EOF
test_expect_success 'intermingled arguments' '
test-parse-options a1 --string 123 b1 --boolean -j 13 -- --boolean \
> output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
'
@@ -217,13 +217,13 @@ EOF
test_expect_success 'unambiguously abbreviated option' '
test-parse-options --int 2 --boolean --no-bo > output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
'
test_expect_success 'unambiguously abbreviated option with "="' '
test-parse-options --int=2 > output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
'
@@ -246,7 +246,7 @@ EOF
test_expect_success 'non ambiguous option (after two options it abbreviates)' '
test-parse-options --st 123 > output 2> output.err &&
- test ! -s output.err &&
+ test_output_must_be_empty output.err &&
test_cmp expect output
'
@@ -256,7 +256,7 @@ EOF
test_expect_success 'detect possible typos' '
test_must_fail test-parse-options -boolean > output 2> output.err &&
- test ! -s output &&
+ test_output_must_be_empty output &&
te
Re: [PATCH 2/3] test: improve rebase -q test
On Fri, Jun 7, 2013 at 9:44 PM, Duy Nguyen wrote: > On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras > wrote: >> Let's show the output so it's clear why it failed. > > I think you can always look into trash-directory.t3400 and figure why. > But if you show this, I think you should use test_cmp to make it clear > what is not wanted. Something like > > : >expected && > test_cmp expected output.out Feel free to send that patch. I'm done with this one. -- Felipe Contreras -- 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 2/3] test: improve rebase -q test
On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
wrote:
> Let's show the output so it's clear why it failed.
I think you can always look into trash-directory.t3400 and figure why.
But if you show this, I think you should use test_cmp to make it clear
what is not wanted. Something like
: >expected &&
test_cmp expected output.out
>
> Signed-off-by: Felipe Contreras
> ---
> t/t3400-rebase.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index b58fa1a..fb39531 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream
> arg is missing' '
> test_expect_success 'rebase -q is quiet' '
> git checkout -b quiet topic &&
> git rebase -q master >output.out 2>&1 &&
> + cat output.out &&
> test ! -s output.out
> '
>
> --
> 1.8.3.698.g079b096
>
> --
> 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
--
Duy
--
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
[PATCH 2/3] test: improve rebase -q test
Let's show the output so it's clear why it failed.
Signed-off-by: Felipe Contreras
---
t/t3400-rebase.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..fb39531 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream
arg is missing' '
test_expect_success 'rebase -q is quiet' '
git checkout -b quiet topic &&
git rebase -q master >output.out 2>&1 &&
+ cat output.out &&
test ! -s output.out
'
--
1.8.3.698.g079b096
--
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
