[PATCH] tests: create an interactive gdb session with the 'debug' helper

[PATCH] tests: create an interactive gdb session with the 'debug' helper

[SZEDER Gábor]

The 'debug' test helper is supposed to facilitate debugging by running
a command of the test suite under gdb.  Unfortunately, its usefulness
is severely limited, because that gdb session is not interactive,
since the test's, and thus gdb's standard input is attached to
/dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
of tests, 2011-12-15)).

Re-attach the original standard input in the debug helper, thus
creating an interactive gdb session, which is much much more useful.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index bd357704c..9567dc986 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -154,7 +154,7 @@ test_pause () {
 #
 # Example: "debug git checkout master".
 debug () {
-	 GIT_TEST_GDB=1 "$@"
+	 GIT_TEST_GDB=1 "$@" <&6
 }
 
 # Call test_commit with the arguments
-- 
2.12.0.375.g7a2ebd56b

Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper

[Jeff King]

On Fri, Mar 17, 2017 at 03:46:46PM +0100, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is attached to
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Re-attach the original standard input in the debug helper, thus
> creating an interactive gdb session, which is much much more useful.

Yeah, I think I've had to do a similar hack manually. This is an obvious
improvement. Though...

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index bd357704c..9567dc986 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -154,7 +154,7 @@ test_pause () {
>  #
>  # Example: "debug git checkout master".
>  debug () {
> -	 GIT_TEST_GDB=1 "$@"
> +	 GIT_TEST_GDB=1 "$@" <&6
>  }

Your stderr and stdout may be redirected, too. That's usually not a big
problem because you'll be using "-v", but perhaps this should add:

  >&3 2>&4

to be on the safe side. That covers running without "-v", as well as
cases where the test script is redirecting output (I _actually_ wish gdb
behaved like perl's debugger and unconditionally used the terminal,
retaining existing redirections, but I AFAIK there's no easy way to get
it to do that).

-Peff

Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper

[SZEDER Gábor]

On Fri, Mar 17, 2017 at 3:55 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 17, 2017 at 03:46:46PM +0100, SZEDER Gábor wrote:
>
>> The 'debug' test helper is supposed to facilitate debugging by running
>> a command of the test suite under gdb.  Unfortunately, its usefulness
>> is severely limited, because that gdb session is not interactive,
>> since the test's, and thus gdb's standard input is attached to
>> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
>> of tests, 2011-12-15)).
>>
>> Re-attach the original standard input in the debug helper, thus
>> creating an interactive gdb session, which is much much more useful.
>
> Yeah, I think I've had to do a similar hack manually. This is an obvious
> improvement. Though...
>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index bd357704c..9567dc986 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -154,7 +154,7 @@ test_pause () {
>>  #
>>  # Example: "debug git checkout master".
>>  debug () {
>> -      GIT_TEST_GDB=1 "$@"
>> +      GIT_TEST_GDB=1 "$@" <&6
>>  }
>
> Your stderr and stdout may be redirected, too. That's usually not a big
> problem because you'll be using "-v",

Yeah, I didn't consider using this helper in non-verbose mode at all,
and the resulting behaviour is not quite pleasant, indeed: gdb starts
in interactive mode, but as its stdout goes straight to /dev/null the
user has no idea, and gdb does not quit on Ctrl-C.  A possible
solution would be to forbid using the 'debug' helper without '-v',
like in the 'test_pause' helper just above.

However...

> but perhaps this should add:
>
>   >&3 2>&4
>
> to be on the safe side. That covers running without "-v", as well as
> cases where the test script is redirecting output

That wouldn't buy us much.  First, file descriptors 3 and 4 are the
test's stdout and stderr, i.e. any process started within the test
would be connected to those fds anyway without those explicit
redirections.  Second, fds 3 and 4 are redirected to /dev/null in
non-verbose mode, so it would still not work without '-v'.

Perhaps you meant '>&5 2>&7' here (and in the bash example in the
commit message of 781f76b15 (test-lib: redirect stdin of tests,
2011-12-15), for that matter)?  Those redirections do in fact make
'debug' work even without '-v'.  Furthermore, those redirections do
make 'test_pause' work without '-v', too.

So this is what v2 will do, then.

Gábor

[PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper

[SZEDER Gábor]

The 'debug' test helper is supposed to facilitate debugging by running
a command of the test suite under gdb.  Unfortunately, its usefulness
is severely limited, because that gdb session is not interactive,
since the test's, and thus gdb's standard input is redirected from
/dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
of tests, 2011-12-15)).

Redirect gdb's standard file descriptors from/to the test
environment's stdin, stdout and stderr in the 'debug' helper, thus
creating an interactive gdb session (even in non-verbose mode), which
is much, much more useful.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index bd357704c..4a50a6104 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -154,7 +154,7 @@ test_pause () {
 #
 # Example: "debug git checkout master".
 debug () {
-	 GIT_TEST_GDB=1 "$@"
+	 GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
 }
 
 # Call test_commit with the arguments
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16d..23c29bce6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -342,6 +342,7 @@ fi
 
 exec 5>&1
 exec 6<&0
+exec 7>&2
 if test "$verbose_log" = "t"
 then
 	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
-- 
2.12.0.377.g15f6ffe90

[PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode

[SZEDER Gábor]

When the 'test_pause' helper function invokes the shell mid-test, it
explicitly redirects the shell's stdout and stderr to file descriptors
3 and 4, which are the stdout and stderr of the tests (i.e. where they
would be connected anyway without those redirections).  These file
descriptors are only attached to the terminal in verbose mode, hence
the restriction of 'test_pause' to work only with '-v'.

Redirect the shell's stdout and stderr to the test environment's
original stdout and stderr, allowing it to work properly even in
non-verbose mode, and the restriction can be lifted.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4a50a6104..5ee124332 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -136,17 +136,12 @@ 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".
+# Stop execution and start a shell. This is useful for debugging tests.
 #
 # 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
+	"$SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git in gdb. Adding this to a command can make it easier to
-- 
2.12.0.377.g15f6ffe90

Re: [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode

[Jeff King]

On Sat, Mar 18, 2017 at 05:14:00PM +0100, SZEDER Gábor wrote:

> When the 'test_pause' helper function invokes the shell mid-test, it
> explicitly redirects the shell's stdout and stderr to file descriptors
> 3 and 4, which are the stdout and stderr of the tests (i.e. where they
> would be connected anyway without those redirections).  These file
> descriptors are only attached to the terminal in verbose mode, hence
> the restriction of 'test_pause' to work only with '-v'.
> 
> Redirect the shell's stdout and stderr to the test environment's
> original stdout and stderr, allowing it to work properly even in
> non-verbose mode, and the restriction can be lifted.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/test-lib-functions.sh | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

This makes sense, too. Thanks.

-Peff

Re: [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper

[Jeff King]

On Sat, Mar 18, 2017 at 05:13:59PM +0100, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is redirected from
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Redirect gdb's standard file descriptors from/to the test
> environment's stdin, stdout and stderr in the 'debug' helper, thus
> creating an interactive gdb session (even in non-verbose mode), which
> is much, much more useful.
>
> [...]
>
>  debug () {
> -	 GIT_TEST_GDB=1 "$@"
> +	 GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
>  }

Yep, this looks fine to me.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 86d77c16d..23c29bce6 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -342,6 +342,7 @@ fi
>  
>  exec 5>&1
>  exec 6<&0
> +exec 7>&2

And this obviously is required to keep stdout/stderr separate.

-Peff

Re: [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi Gábor,

On Sat, 18 Mar 2017, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is redirected from
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Redirect gdb's standard file descriptors from/to the test
> environment's stdin, stdout and stderr in the 'debug' helper, thus
> creating an interactive gdb session (even in non-verbose mode), which
> is much, much more useful.

Thank you for working on this!

I meant to clean up my local patch that tried to avoid redirection
altogether in the case of `debug`, but I never could make it elegant
enough for my taste. Your approach is much better!

ACK,
Dscho

Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper

[Jeff King]

On Sat, Mar 18, 2017 at 05:10:18PM +0100, SZEDER Gábor wrote:

> > but perhaps this should add:
> >
> >   >&3 2>&4
> >
> > to be on the safe side. That covers running without "-v", as well as
> > cases where the test script is redirecting output
> 
> That wouldn't buy us much.  First, file descriptors 3 and 4 are the
> test's stdout and stderr, i.e. any process started within the test
> would be connected to those fds anyway without those explicit
> redirections.  Second, fds 3 and 4 are redirected to /dev/null in
> non-verbose mode, so it would still not work without '-v'.
> 
> Perhaps you meant '>&5 2>&7' here (and in the bash example in the
> commit message of 781f76b15 (test-lib: redirect stdin of tests,
> 2011-12-15), for that matter)?  Those redirections do in fact make
> 'debug' work even without '-v'.  Furthermore, those redirections do
> make 'test_pause' work without '-v', too.

Yeah, sorry, I got my descriptors mixed up. ">&5 2>&1" is the right
invocation (unless you add a new "7" to save the original stderr).

-Peff