[PATCH] test-lib: add the test_bash convenience function

[PATCH] test-lib: add the test_bash convenience function

[Jens Lehmann]

Since 781f76b15 (test-lib: redirect stdin of tests) you can't simply put a
"bash &&" into a test for debugging purposes anymore. Instead you'll have
to use "bash <&6 >&3 2>&4".

As that invocation is not that easy to remember add the test_bash
convenience function. This function also checks if the -v flag is given
and will complain if that is not the case instead of letting the test
hang until ^D is pressed.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I was tempted to call that method "run_bash" but after looking around
in test-lib.sh "test_bash" seemed like a better name.

 t/test-lib.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65dfc7..f9061e0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -329,6 +329,17 @@ test_tick () {
 	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }

+# Stop execution and start a bash shell. This is useful for debugging tests
+# and only makes sense together with "-v".
+
+test_bash () {
+	if test "$verbose" = t; then
+		bash <&6 >&3 2>&4
+	else
+		say >&5 "skipping test_bash as it makes no sense without -v"
+	fi
+}
+
 # Call test_commit with the arguments "<message> [<file> [<contents>]]"
 #
 # This will commit a file with the given contents and the given commit
-- 
1.7.9.rc1.1.g46aa0.dirty

Re: [PATCH] test-lib: add the test_bash convenience function

[Jeff King]

On Sun, Jan 15, 2012 at 09:00:41PM +0100, Jens Lehmann wrote:

> Since 781f76b15 (test-lib: redirect stdin of tests) you can't simply put a
> "bash &&" into a test for debugging purposes anymore. Instead you'll have
> to use "bash <&6 >&3 2>&4".

Yeah, an unfortunate side effect.

If you're not relying on particular state in the middle of a chain of
commands, you can just put the "bash" outside of the test_expect_*. But
sometimes you do care about having it in the middle.

> As that invocation is not that easy to remember add the test_bash
> convenience function. This function also checks if the -v flag is given
> and will complain if that is not the case instead of letting the test
> hang until ^D is pressed.

Nice. Many times I have added such a "bash" or "gdb" invocation then
forgotten "-v", only to scratch my head at why the test seemed to be
hanging.

Two minor nits on the patch itself:

> +# Stop execution and start a bash shell. This is useful for debugging tests
> +# and only makes sense together with "-v".
> +
> +test_bash () {
> +	if test "$verbose" = t; then
> +		bash <&6 >&3 2>&4
> +	else
> +		say >&5 "skipping test_bash as it makes no sense without -v"
> +	fi
> +}

1. It may be worth putting a warning in the comment that this is never
   to be used in a real test, but only temporarily inserted.

2. I do this not just with bash, but with "gdb". I wonder if it is worth
   making this "test_foo bash", for some value of "foo" (the ones that
   occur to me are "debug" and "run", but of course they are taken).

   Actually, I wonder if the existing test_debug could handle this
   already (though you do have to remember to add "--debug" to your
   command line, then).

-Peff

[PATCH] test_interactive: interactive debugging in test scripts

[Pete Wyckoff]

For test debugging, pause test execution and spawn a shell to
allow examination of internal test state.  The shell has access
to all exported variables from the test.  Exit the shell to
continue the test.  Calls to this function should never be
included in committed code.  The "--verbose" option is required.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
peff@peff.net wrote on Sun, 15 Jan 2012 18:24 -0500:
> On Sun, Jan 15, 2012 at 09:00:41PM +0100, Jens Lehmann wrote:
> > As that invocation is not that easy to remember add the test_bash
> > convenience function. This function also checks if the -v flag is given
> > and will complain if that is not the case instead of letting the test
> > hang until ^D is pressed.
> 
> Nice. Many times I have added such a "bash" or "gdb" invocation then
> forgotten "-v", only to scratch my head at why the test seemed to be
> hanging.

Yes, good catch noticing the need for "-v".

Here's something similar that I've been playing around with
locally.  I export HOME and TERM in the debug shell to make
sure all the features around dotfiles and color/editing work
nicely.  Also just use SHELL directly, not SHELL_PATH or bash,
to cater to other shell users.

And it is necessary to export any test variables you want to use
in the debug shell.  I often cut-n-paste lines containing
TEST_DIRECTORY and TRASH_DIRECTORY; there could be others,
in test scripts and helper libraries too.

> 2. I do this not just with bash, but with "gdb". I wonder if it is worth
>    making this "test_foo bash", for some value of "foo" (the ones that
>    occur to me are "debug" and "run", but of course they are taken).
> 
>    Actually, I wonder if the existing test_debug could handle this
>    already (though you do have to remember to add "--debug" to your
>    command line, then).

While it would be nice to use:

    test_interactive gdb --args git ...

the path is setup to invoke the script in bin-wrappers/git,
requiring either --with-dashes or something like

    test_interactive gdb --args "$GIT_EXEC_PATH"/git ...

both of which I'm sure to forget.  I have a "test_gdb_git" that
can be used in place of "git" in a script for exactly this
purpose, but feel it's not general enough to warrant inclusion.
It's easy to start a shell then invoke gdb by hand.

Between mine and Jens' there is hopefully something widely useful
here.

		-- Pete

 t/README      |    9 +++++++++
 t/test-lib.sh |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index c85abaf..da4521f 100644
--- a/t/README
+++ b/t/README
@@ -548,6 +548,15 @@ library for your script to use.
 		...
 	'
 
+ - test_interactive
+
+   For test debugging, pause test execution and spawn a shell
+   to allow examination of internal test state.  The shell has
+   access to all exported variables from the test.  Exit the shell
+   to continue the test.  Calls to this function should never be
+   included in committed code.  This function requires the "-v"
+   ("--verbose") option.
+
 Prerequisites
 -------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65dfc7..a834602 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -32,7 +32,9 @@ done,*)
 esac
 
 # Keep the original TERM for say_color
+# and the original HOME for interactive debugging
 ORIGINAL_TERM=$TERM
+ORIGINAL_HOME="$HOME"
 
 # For repeatability, reset the environment to known value.
 LANG=C
@@ -473,6 +475,36 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+#
+# Add to a test script to spawn a shell during execution.  This
+# spawns a shell allowing inspection of internal test state.  Exit
+# the shell to continue with the test.  Example:
+#
+# 	test_expect_success 'test' '
+# 		git do-something &&
+# 		test_interactive &&
+# 		test_cmp expected current
+#	'
+#
+# Be sure to remove the debug lines before submitting:  it only
+# works with "-v".
+#
+test_interactive () {
+	if test -z "$verbose" ; then
+		error >&5 "test_interactive requires --verbose"
+	fi
+	say >&3 "Interactive debugging"
+	say >&3 "Exit shell to continue test"
+	(
+		# restore important original environment variables
+		exec 0<&6 3>&5 2>&4
+		export HOME="$ORIGINAL_HOME"
+		export TERM="$ORIGINAL_TERM"
+		exec $SHELL
+	)
+	return 0
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -901,6 +933,7 @@ then
 	# itself.
 	TEST_DIRECTORY=$(pwd)
 fi
+export TEST_DIRECTORY
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
 if test -n "$valgrind"
@@ -1038,6 +1071,7 @@ case "$test" in
 /*) TRASH_DIRECTORY="$test" ;;
  *) TRASH_DIRECTORY="$TEST_DIRECTORY/$test" ;;
 esac
+export TRASH_DIRECTORY
 test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
 rm -fr "$test" || {
 	GIT_EXIT_OK=t
-- 
1.7.9.rc0.47.gc9457

Re: [PATCH] test_interactive: interactive debugging in test scripts

[Jens Lehmann]

Am 16.01.2012 16:49, schrieb Pete Wyckoff:
> Between mine and Jens' there is hopefully something widely useful
> here.

Just while I was planning a v2 and thought about renaming "test_bash"
to "test_interactive", let it take a command to run as optional
argument and document it better you came up with this :-)

So I vote for your patch as it takes my initial idea even further. I
really like that HOME, TERM and SHELL are honored in your version
leaving the user with a fully functional shell of his choice.

Re: [PATCH] test_interactive: interactive debugging in test scripts

[Jeff King]

On Mon, Jan 16, 2012 at 09:01:21PM +0100, Jens Lehmann wrote:

> So I vote for your patch as it takes my initial idea even further. I
> really like that HOME, TERM and SHELL are honored in your version
> leaving the user with a fully functional shell of his choice.

I'm actually mildly negative on this feature, as it interferes with the
tests themselves. Probably TERM and SHELL don't matter. But $HOME means
git will read your personal .gitconfig, not any config (or lack thereof)
in the trash directory.

I've personally been left head-scratching in the past by:

  $ ./tXXXX-foo -v -i
  ...
  expecting success: git foo
  not ok - 1 git foo

  $ cd trash\ directory.tXXXX-foo
  $ git foo ;# and it works!

where the culprit was a non-clean environment caused by my personal git
config (or very occasionally, something else in $HOME).

-Peff

Re: [PATCH] test_interactive: interactive debugging in test scripts

[Jens Lehmann]

Am 16.01.2012 21:11, schrieb Jeff King:
> On Mon, Jan 16, 2012 at 09:01:21PM +0100, Jens Lehmann wrote:
> 
>> So I vote for your patch as it takes my initial idea even further. I
>> really like that HOME, TERM and SHELL are honored in your version
>> leaving the user with a fully functional shell of his choice.
> 
> I'm actually mildly negative on this feature, as it interferes with the
> tests themselves. Probably TERM and SHELL don't matter. But $HOME means
> git will read your personal .gitconfig, not any config (or lack thereof)
> in the trash directory.

Good point, I haven't thought of that ... so yes, at least $HOME should
go.

Re: [PATCH] test_interactive: interactive debugging in test scripts

[Pete Wyckoff]

Jens.Lehmann@web.de wrote on Mon, 16 Jan 2012 21:48 +0100:
> Am 16.01.2012 21:11, schrieb Jeff King:
> > On Mon, Jan 16, 2012 at 09:01:21PM +0100, Jens Lehmann wrote:
> > 
> >> So I vote for your patch as it takes my initial idea even further. I
> >> really like that HOME, TERM and SHELL are honored in your version
> >> leaving the user with a fully functional shell of his choice.
> > 
> > I'm actually mildly negative on this feature, as it interferes with the
> > tests themselves. Probably TERM and SHELL don't matter. But $HOME means
> > git will read your personal .gitconfig, not any config (or lack thereof)
> > in the trash directory.
> 
> Good point, I haven't thought of that ... so yes, at least $HOME should
> go.

I, too, have stumbled over differences that are due to picking up
something in $HOME.  It takes a minute to realize what's going
on.

TERM can interfere with at least one test: c2116a1 (test-lib: fix TERM
to dumb for test repeatability, 2008-03-06).  SHELL can cause
issues when it is more feature-ful than SHELL_PATH.

On the other hand, it's frustrating to work in an environment
without my shell aliases, git aliases, readline customizations,
personal path, and assorted environment variables.

I'm comfortable with the (rare?) possibility of confusion.  But
perhaps it is unwise to support a feature with so many caveats,
even if it is only for debugging.

		-- Pete

Re: [PATCH] test_interactive: interactive debugging in test scripts

[Jeff King]

On Mon, Jan 16, 2012 at 10:49:53AM -0500, Pete Wyckoff wrote:

> And it is necessary to export any test variables you want to use
> in the debug shell.  I often cut-n-paste lines containing
> TEST_DIRECTORY and TRASH_DIRECTORY; there could be others,
> in test scripts and helper libraries too.

Yeah, exporting a few common ones would be helpful. I really wish there
was a way to ask a shell to stop running the script and start doing
interactive things in the current shell context, but no such thing
exists AFAIK (you can hack it with a read-eval loop, but you are missing
many of the useful input-handling bits like tab completion. Hmm, I
wonder if bash's "read -e" would be enough, though).

I kind of wonder if that is over-engineering, though. I'd like a perfect
debugging environment, but the fact of the matter is that writing,
maintaining, and making it work in every case that comes up is probably
a lot more work than just fiddling with the scripts now and then. This
code isn't even meant to be run except in such fiddling circumstances.

> While it would be nice to use:
> 
>     test_interactive gdb --args git ...
> 
> the path is setup to invoke the script in bin-wrappers/git,
> requiring either --with-dashes or something like
> 
>     test_interactive gdb --args "$GIT_EXEC_PATH"/git ...

Yeah. I have before patched the bin-wrappers script to accept a
GIT_WRAPPER_PREFIX variable, so you can just set that and have it run
gdb on your invocation. But even that's not enough for externals. I've
been tempted to actually carry around a run-time option to exec
externals via gdb, but I didn't want to pollute the regular code base
(and you can usually get by with just running "gdb git-foo" directly).

-Peff

Re: [PATCH] test-lib: add the test_bash convenience function

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Nice. Many times I have added such a "bash" or "gdb" invocation then
> forgotten "-v", only to scratch my head at why the test seemed to be
> hanging.
>
> Two minor nits on the patch itself:
> ...
> 1. It may be worth putting a warning in the comment that this is never
>    to be used in a real test, but only temporarily inserted.
>
> 2. I do this not just with bash, but with "gdb". I wonder if it is worth
>    making this "test_foo bash", for some value of "foo" (the ones that
>    occur to me are "debug" and "run", but of course they are taken).
>
>    Actually, I wonder if the existing test_debug could handle this
>    already (though you do have to remember to add "--debug" to your
>    command line, then).

I wondered the same thing from a different angle. My first reaction was
"Why is this called 'bash' not 'sh'?" which naturally led to the same
direction as yours "why not an arbitrary command 'test_debug xxx'?"

test_pause perhaps?

[PATCH] test-lib: add the test_pause convenience function

[Jens Lehmann]

Since 781f76b15 (test-lib: redirect stdin of tests) you can't simply put a
"bash &&" into a test for debugging purposes anymore. Instead you'll have
to use "bash <&6 >&3 2>&4".

As that invocation is not that easy to remember add the test_pause
convenience function. This function also checks if the -v flag is given
and will error out if that is not the case instead of letting the test
hang until ^D is pressed.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 16.01.2012 23:51, schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> 
>> Nice. Many times I have added such a "bash" or "gdb" invocation then
>> forgotten "-v", only to scratch my head at why the test seemed to be
>> hanging.
>>
>> Two minor nits on the patch itself:
>> ...
>> 1. It may be worth putting a warning in the comment that this is never
>>    to be used in a real test, but only temporarily inserted.
>>
>> 2. I do this not just with bash, but with "gdb". I wonder if it is worth
>>    making this "test_foo bash", for some value of "foo" (the ones that
>>    occur to me are "debug" and "run", but of course they are taken).
>>
>>    Actually, I wonder if the existing test_debug could handle this
>>    already (though you do have to remember to add "--debug" to your
>>    command line, then).
> 
> I wondered the same thing from a different angle. My first reaction was
> "Why is this called 'bash' not 'sh'?" which naturally led to the same
> direction as yours "why not an arbitrary command 'test_debug xxx'?"
> 
> test_pause perhaps?

I really don't care deeply about the name, so test_pause is absolutely
ok for me. I added some documentation in t/README too and made it an
error when --verbose is not used.

Is it ok to invoke bash here or should sh be used?


 t/README      |   13 +++++++++++++
 t/test-lib.sh |   13 +++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index c85abaf..c09c582 100644
--- a/t/README
+++ b/t/README
@@ -548,6 +548,19 @@ library for your script to use.
 		...
 	'

+ - test_pause
+
+	This command is useful for writing and debugging tests and must be
+	removed before submitting. It halts the execution of the test and
+	spawns a shell in the trash directory. Exit the shell to continue
+	the test. Example:
+
+	test_expect_success 'test' '
+		git do-something >actual &&
+		test_pause &&
+		test_cmp expected actual
+	'
+
 Prerequisites
 -------------

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65dfc7..85084c4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -329,6 +329,19 @@ test_tick () {
 	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }

+# Stop execution and start a shell. This is useful for debugging tests and
+# only makes sense together with "-v".
+#
+# Be sure to remove all invocations of this command before submitting.
+
+test_pause () {
+	if test "$verbose" = t; then
+		bash <&6 >&3 2>&4
+	else
+		error >&5 "test_pause requires --verbose"
+	fi
+}
+
 # Call test_commit with the arguments "<message> [<file> [<contents>]]"
 #
 # This will commit a file with the given contents and the given commit
-- 
1.7.9.rc1.2.g0b847.dirty

Re: [PATCH] test-lib: add the test_pause convenience function

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> I really don't care deeply about the name, so test_pause is absolutely
> ok for me. I added some documentation in t/README too and made it an
> error when --verbose is not used.

I don't care about the name at all, either.

What I cared was more about the hardcoded "bash". Believe it or not, there
are boxes that lack it, and there are people who prefer other shells for
their interactive work. At the very least, invoke "$SHELL_PATH" instead of
bash there, perhaps?

If we wanted to allow an ad-hoc debugging of test scripts to sprinkle
"test_pause $cmd", we might need to do something like:

> +test_pause () {
> +	if test "$verbose" = t; then
> +		bash <&6 >&3 2>&4
		${1-"$SHELL_PATH"} <&6 >&3 2>&4
> +	else
> +		error >&5 "test_pause requires --verbose"
> +	fi
> +}

but I do not think that is worth it. The debugging developer should easily
be able to run gdb or whatever from the interactive shell you are giving
here.

[PATCH v2] test-lib: add the test_pause convenience function

[Jens Lehmann]

Since 781f76b15 (test-lib: redirect stdin of tests) you can't simply put a
"bash &&" into a test for debugging purposes anymore. Instead you'll have
to use "bash <&6 >&3 2>&4".

As that invocation is not that easy to remember add the test_pause
convenience function. It invokes "$SHELL_PATH" to provide a sane shell
for the user.

This function also checks if the -v flag is given and will error out if
that is not the case instead of letting the test hang until ^D is pressed.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 17.01.2012 20:15, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> What I cared was more about the hardcoded "bash". Believe it or not, there
> are boxes that lack it, and there are people who prefer other shells for
> their interactive work. At the very least, invoke "$SHELL_PATH" instead of
> bash there, perhaps?

Sure, I changed that in this version and explained it in the commit
message.

> If we wanted to allow an ad-hoc debugging of test scripts to sprinkle
> "test_pause $cmd", we might need to do something like:
> 
>> +test_pause () {
>> +	if test "$verbose" = t; then
>> +		bash <&6 >&3 2>&4
> 		${1-"$SHELL_PATH"} <&6 >&3 2>&4
>> +	else
>> +		error >&5 "test_pause requires --verbose"
>> +	fi
>> +}
> 
> but I do not think that is worth it. The debugging developer should easily
> be able to run gdb or whatever from the interactive shell you are giving
> here.

That's what I always do, so I'm fine with what this patch provides. And
now the fact that you can temporarily pause a test and explore the trash
directory is documented too ;-)


 t/README      |   13 +++++++++++++
 t/test-lib.sh |   13 +++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index c85abaf..c09c582 100644
--- a/t/README
+++ b/t/README
@@ -548,6 +548,19 @@ library for your script to use.
 		...
 	'

+ - test_pause
+
+	This command is useful for writing and debugging tests and must be
+	removed before submitting. It halts the execution of the test and
+	spawns a shell in the trash directory. Exit the shell to continue
+	the test. Example:
+
+	test_expect_success 'test' '
+		git do-something >actual &&
+		test_pause &&
+		test_cmp expected actual
+	'
+
 Prerequisites
 -------------

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65dfc7..709a300 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -329,6 +329,19 @@ test_tick () {
 	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }

+# Stop execution and start a shell. This is useful for debugging tests and
+# only makes sense together with "-v".
+#
+# Be sure to remove all invocations of this command before submitting.
+
+test_pause () {
+	if test "$verbose" = t; then
+		"$SHELL_PATH" <&6 >&3 2>&4
+	else
+		error >&5 "test_pause requires --verbose"
+	fi
+}
+
 # Call test_commit with the arguments "<message> [<file> [<contents>]]"
 #
 # This will commit a file with the given contents and the given commit
-- 
1.7.9.rc1.1.g8dae2