Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@peff.net>
> ---