Jeff King [off-list ref] writes:
On Tue, Oct 07, 2014 at 01:35:15PM -0700, Junio C Hamano wrote:
quoted
Yeah, if we are going to reduce it down to the above implementation,
intereseting things like "test -f $frotz" will become possible and
"cmp-str" stops making sense. It really is about "We run test and
expect it to yield true. Report the failure a bit more prominently
under the '-v' option to help us debug".
We already have test_path_is_file to do the same thing just for "-f". We
could in theory switch all of those to this new, more generic wrapper. I
don't know if it is worth doing a mass-conversion, but we could
discourage test_path_is_file in new tests. We could also implement
test_path_is_{dir,file} on top of this.
Oh, I wasn't going in that direction when I mentioned "-f"; I just
wanted to say that 'test "$@"' is clearly about 'test' (/bin/test or
shell built-in) and less about 'compare string'. I do not think it
is necessarily a good direction to go in to replace test-path-is-file
with the test_cond thing; after all, type specific tests have chance
to report breakage of expectation in type specifc ways, e.g.
test_path_is_file () {
test -f "$1" && return 0
echo >&2 "expected '$1' to be file"
if test -e "$1"
then
echo >&2 "but it is missing"
else
echo >&2 "but it is a non-file"
ls >&2 -ld "$1"
fi
return 1
}
But that is also just in theory ;-).
quoted
So among the ones you listed, test_verbose may be the least silly, I
would think.
Somehow test_verbose seems to me like checking the "verbose" option of
the test suite. I prefer "test_cond", but I do not feel too strongly, if
you want to override me.
Hmph, your 'test' in that name is a generic verb "we check that...",
which I think aligns better with the other test_foo functions. When
I suggested 'test_verbose', 'test' in that name was specifically
meant to refer to the 'test' command.
Still "test_cond" feels somewhat funny, as "we check that..." always
validates some condition, but I don't think of anything better X-<.
On 10/07/2014 11:53 PM, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
On Tue, Oct 07, 2014 at 01:35:15PM -0700, Junio C Hamano wrote:
quoted
Yeah, if we are going to reduce it down to the above implementation,
intereseting things like "test -f $frotz" will become possible and
"cmp-str" stops making sense. It really is about "We run test and
expect it to yield true. Report the failure a bit more prominently
under the '-v' option to help us debug".
We already have test_path_is_file to do the same thing just for "-f". We
could in theory switch all of those to this new, more generic wrapper. I
don't know if it is worth doing a mass-conversion, but we could
discourage test_path_is_file in new tests. We could also implement
test_path_is_{dir,file} on top of this.
Oh, I wasn't going in that direction when I mentioned "-f"; I just
wanted to say that 'test "$@"' is clearly about 'test' (/bin/test or
shell built-in) and less about 'compare string'. I do not think it
is necessarily a good direction to go in to replace test-path-is-file
with the test_cond thing; after all, type specific tests have chance
to report breakage of expectation in type specifc ways, e.g.
test_path_is_file () {
test -f "$1" && return 0
echo >&2 "expected '$1' to be file"
if test -e "$1"
then
echo >&2 "but it is missing"
else
echo >&2 "but it is a non-file"
ls >&2 -ld "$1"
fi
return 1
}
But that is also just in theory ;-).
quoted
quoted
So among the ones you listed, test_verbose may be the least silly, I
would think.
Somehow test_verbose seems to me like checking the "verbose" option of
the test suite. I prefer "test_cond", but I do not feel too strongly, if
you want to override me.
Hmph, your 'test' in that name is a generic verb "we check that...",
which I think aligns better with the other test_foo functions. When
I suggested 'test_verbose', 'test' in that name was specifically
meant to refer to the 'test' command.
Still "test_cond" feels somewhat funny, as "we check that..." always
validates some condition, but I don't think of anything better X-<.
I like "verbose_test $foo = $bar" because it puts the word "test" next
to the condition, where the built-in command "test" would otherwise be.
We could even define a command
verbose () {
"$@" && return 0
echo >&2 "command failed: $*"
return 1
}
and use it like
verbose test $foo = $bar
Somehow I feel like I'm reinventing something that must already exist...
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
On Wed, Oct 08, 2014 at 12:17:07AM +0200, Michael Haggerty wrote:
On 10/07/2014 11:53 PM, Junio C Hamano wrote:
quoted
Hmph, your 'test' in that name is a generic verb "we check that...",
which I think aligns better with the other test_foo functions. When
I suggested 'test_verbose', 'test' in that name was specifically
meant to refer to the 'test' command.
I actually meant "test" as a namespace to indicate it is part of the
test suite (just like "test_seq" is not testing anything). I think that
is why the names are so silly. We are using the "test" command in our
"test" suite to "test" some conditions.
I like "verbose_test $foo = $bar" because it puts the word "test" next
to the condition, where the built-in command "test" would otherwise be.
We could even define a command
verbose () {
"$@" && return 0
echo >&2 "command failed: $*"
return 1
}
and use it like
verbose test $foo = $bar
I kind of like this. It is easy to see which shell command is being
invoked, and it would extend naturally to other silent commands.
Somehow I feel like I'm reinventing something that must already exist...
Yes, we're basically reinventing "set -x" here, with the caveat that we
only _really_ care about showing failed commands. The problem with "set
-x" is that it also wants to apply itself to the test harness itself, so
you end up with a lot of cruft.
Below is my best attempt at keeping the cruft to a minimum. Here's
sample output using "-v" (the commands are all supplied by dummy
aliases):
expecting success:
do_some_thing &&
test "$(inspect_some_thing)" = "expected" &&
do_some_other_thing
+ do_some_thing
+ echo doing some thing...
doing some thing...
+ inspect_some_thing
+ echo foo
+ test foo = expected
+ eval_ret=1
+ set +x
not ok 1 - experiment with set -x
It's not _too_ bad, because we turn on "set -x" for just the test eval
(mostly). But the rough edges are:
1. We are stuck with the "eval_ret = 1; set +x" cleanup at the end. I
don't think there's any way around that without a subshell, and
many tests will not work in a subshell (they set environment
variables they expect to persist).
2. There's nothing highlighting the failed code. You just have to know
that it was the last thing before the eval_ret call. We can set PS4
to show $?, or even do something complicated like:
PS4='+ $(test $? = 0 || say_color error "^^^ failure; ")'
though note that running commands via PS4 works in bash, but
causes dash to go into an infinite loop. :)
But even with that, the output is still not great.
3. The "-x" continues into any shell functions, which are hard to
read. For example, notice above that we walked into the
"do_some_thing" function and showed its implementation. Now imagine
doing that for complicated test_* helpers.
So it's not great. The upside is that it Just Works everywhere without
even having to modify the tests themselves. I admit I have sometimes
used "sh -x" to debug a test script, but it is usually a giant pain due
to the verbosity of the harness code. The patch below cuts out _most_ of
that because at least we just use it during the eval, but I'm still not
sure it's a good default for "-v" (we could add it as "-vv" or
something, though, if others find it useful).
---diff --git a/t/test-lib.sh b/t/test-lib.sh
index 82095e3..af51868 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -517,10 +517,30 @@ maybe_setup_valgrind () {
fi
}
+# This is a separate function because some tests use
+# "return" to end a test_expect_success block early
+# (and we want to make sure we do our "set +x" cleanup).
+test_eval_inner_2 () {
+ # We do the "set -x" inside the eval because we
+ # want to keep it as close to the actual test
+ # commands as possible to avoid harness cruft.
+ eval "set -x; $*"
+}
+
+# All of the "set +x" cleanup has to happen inside
+# here, because the output is redirected (otherwise
+# we leak "set -x" lines to stderr in non-verbose mode.
+test_eval_inner_1 () {
+ test_eval_inner_2 "$@"
+ eval_ret=$?
+ set +x
+ return $eval_ret
+}
+
+# This wrapper exists just to keep the I/O redirect
+# factored out into a single place.
test_eval_ () {
- # This is a separate function because some tests use
- # "return" to end a test_expect_success block early.
- eval </dev/null >&3 2>&4 "$*"
+ test_eval_inner_1 "$@" </dev/null >&3 2>&4
}
test_run_ () {@@ -531,7 +551,10 @@ test_run_ () {
eval_ret=$?
teardown_malloc_check
- if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
+ # We avoid running a straight ":" because it is a noop, and it
+ # pollutes our "set -x" output.
+ if test -z "$immediate" || test $eval_ret = 0 ||
+ test -n "$expecting_failure" && test "$test_cleanup" != ":"
then
setup_malloc_check
test_eval_ "$test_cleanup"