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
SZEDER Gábor sze...@ira.uka.de 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 Mon, Jun 10, 2013 at 10:56 AM, Junio C Hamano gits...@pobox.com 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
On Mon, Jun 10, 2013 at 08:56:58AM -0700, Junio C Hamano wrote: SZEDER Gábor sze...@ira.uka.de 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
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
SZEDER Gábor sze...@ira.uka.de 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
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
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
Duy Nguyen pclo...@gmail.com writes:
On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
felipe.contre...@gmail.com 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 gits...@pobox.com
---
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.template expect
test-parse-options $* output 2output.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.template expect
test-parse-options $* output 2output.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 2output.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 2output.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
test_cmp typo.err output.err
'
@@ -266,7 +266,7
Re: [PATCH 2/3] test: improve rebase -q test
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com 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
Felipe Contreras felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com 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 2:20 PM, Junio C Hamano gits...@pobox.com wrote:
Felipe Contreras felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com 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 felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano gits...@pobox.com wrote:
Felipe Contreras felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com 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 3:35 PM, Junio C Hamano gits...@pobox.com wrote:
Felipe Contreras felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano gits...@pobox.com wrote:
Felipe Contreras felipe.contre...@gmail.com writes:
On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com 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
Philip Oakley philipoak...@iee.org 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
On Fri, Jun 7, 2013 at 9:44 PM, Duy Nguyen pclo...@gmail.com wrote: On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras felipe.contre...@gmail.com 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
felipe.contre...@gmail.com 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 felipe.contre...@gmail.com
---
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 21
+ 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
