On Sat, Apr 10 2021, Jeff King wrote:
> Most test snippets are wrapped in single quotes, like:
>
> test_expect_success 'some description' '
> do_something
> '
>
> This sometimes makes the snippets awkward to write, because you can't
> easily use single quotes. We sometimes work around this with $SQ, or by
> loosening regexes to use "." instead of a literal quote, or by using
> double quotes when we'd prefer to use single-quotes (and just adding
> extra backslash-escapes to avoid interpolation).
>
> This commit adds another option: feeding the snippet on the function's
> stdin. This doesn't conflict with anything the snippet would want to do,
> because we always redirect its stdin from /dev/null anyway (which we'll
> continue to do).
I like this, and not having to write $SQ, '"'"' etc.
> A few notes on the implementation:
>
> - it would be nice to push this down into test_run_, but we can't, as
> test_expect_success and test_expect_failure want to see the actual
> script content to report it for verbose-mode. A helper function
> limits the amount of duplication in those callers here.
I've got an unsubmitted series (a bigger part of the -V rewrite) which
conflicted with this one, because I'd moved that message into those
helper functions.
But in that case I end up having to have this in
test_expect_{success,failure} anyway, because the change I had was to
move it into test_{ok,failure}_, i.e. to color the emitted body under
verbose differently depending on test ok/failure (which means deferring
the "this is our test body" until after the run).
It got slightly awkward because before I could pass "$@" to those (they
pass "$1" now), but with your change there's a "-" left on the argument
list, so we need to pass "$1" and "$test_body".
Anyway, it's no problem, just musings on having re-arranged this code
you're pointing out needs/could be re-arranged.
Maybe it would be easier to pass test_run arguments saying whether we
expect failure or not, and then move the whole if/else after it into its
own body. It already takes the "expecting_failure" parameter, so this on
top of master:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d733..9e20bd607d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -611,8 +611,7 @@ test_expect_failure () {
export test_prereq
if ! test_skip "$@"
then
- say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $2"
- if test_run_ "$2" expecting_failure
+ if test_run_ "$1" "$2" expecting_failure
then
test_known_broken_ok_ "$1"
else
@@ -631,8 +630,7 @@ test_expect_success () {
export test_prereq
if ! test_skip "$@"
then
- say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $2"
- if test_run_ "$2"
+ if test_run_ "$1" "$2"
then
test_ok_ "$1"
else
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d3f6af6a65..5a1192e80c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -935,9 +935,20 @@ test_eval_ () {
}
test_run_ () {
+ local description
+ description="$1"
+ shift
+
test_cleanup=:
expecting_failure=$2
+ if test -n "$expecting_failure"
+ then
+ say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$description': $1"
+ else
+ say >&3 "expecting success of $TEST_NUMBER.$test_count '$description': $1"
+ fi
+
if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then
# turn off tracing for this test-eval, as it simply creates
# confusing noise in the "-x" output
... or maybe not, but in any case, if the verbose mode was what was
stopping you from moving this down to "test_run_" just that seems like
an easy change.
I like your current implementation better, i.e. to have the stdin
consumption happen ASAP and have the others be low-level utility
functions, but I don't care much, but if you wanted it the other way
maybe the above diff helps.
> - The helper function is a little awkward to call, as you feed it the
> name of the variable you want to set. The more natural thing in
> shell would be command substitution like:
>
> body=$(body_or_stdin "$2")
>
> but that loses trailing whitespace. There are tricks around this,
> like:
>
> body=$(body_or_stdin "$2"; printf '.)
> body=${body%.}
>
> but we'd prefer to keep such tricks in the helper, not in each
> caller.
I see why you did this, and for a narrow change it's a good thing.
FWIW having spent some more time on making the TAP format more pruttah
in a parallel WIP series I think this is ultimately a losing
game. You're inserting the extra LF because you don't want to have the
"checking..." and the first line of the test body on the same line;
But we have all of:
test_expect_success 'foo' 'true'
test_expect_success 'foo' '
true
'
And now:
test_expect_success 'foo' - <<\EOT
true
'
So if (definitely not needed for your change) wanted to always make this
pretty/indented we'd need to push that logic down to the formatter,
which would insert a leading LF and/or indentation as appropriate.
I just declared that if you use the first form you don't get
indentation :)
> - I implemented the helper using a sequence of "read" calls. Together
> with "-r" and unsetting the IFS, this preserves incoming whitespace.
> An alternative is to use "cat" (which then requires the gross "."
> trick above). But this saves us a process, which is probably a good
> thing. The "read" builtin does use more read() syscalls than
> necessary (one per byte), but that is almost certainly a win over a
> separate process.
>
> Both are probably slower than passing a single-quoted string, but
> the difference is lost in the noise for a script that I converted as
> an experiment.
>
> - I handle test_expect_success and test_expect_failure here. If we
> like this style, we could easily extend it to other spots (e.g.,
> lazy_prereq bodies) on top of this patch.
>
> - even though we are using "local", we have to be careful about our
> variable names. Within test_expect_success, any variable we declare
> with local will be seen by the test snippets themselves (so it won't
> persist between tests like normal variables would).
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
