git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Apr 10, 2021, 08:30 UTC
Message-ID
<87a6q6h78y.fsf@evledraar.gmail.com>
In-Reply-To
<YHDVAxxKDzfTlq3h@coredump.intra.peff.net>
On Sat, Apr 10 2021, Jeff King wrote:
Show 16 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 14 quoted lines
>   - 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 :)

Show 23 quoted lines
>   - 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>
> ---
Previous: Jeff KingNext: Jeff King
Message 7 of 10 in “here-doc test bodies”
  1. 0/2 here-doc test bodiesJeff King, Apr 9, 2021
  2. 1/2 test-lib: allow test snippets as here-docsJeff King, Apr 9, 2021
  3. Jeff KingApr 9, 2021
  4. Junio C HamanoApr 9, 2021
  5. Junio C HamanoApr 10, 2021
  6. Jeff KingApr 10, 2021
  7. Ævar Arnfjörð BjarmasonApr 10, 2021
  8. 2/2 t1404: convert to here-doc test bodiesJeff King, Apr 9, 2021
  9. Derrick StoleeApr 10, 2021
  10. Jeff KingApr 10, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.