[v2,2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

Commit Message

Philippe Blain Aug. 28, 2021, 12:47 a.m. UTC

From: Philippe Blain <[e-mail]>

The 'test_pause' function, which is designed to help interactive
debugging and exploration of tests, currently inherits the value of HOME
and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It
also invokes the shell defined by TEST_SHELL_PATH, which defaults to
/bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a
developers' shell startup files and any Git aliases defined in their
global Git configuration file are not available in the shell invoked by
'test_pause'.

Changing the value of TERM to 'dumb' means that colored output
is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since
some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and
this shell is usually compiled without readline support, which makes for
a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by
'test_pause' more pleasant, save the values of HOME and TERM in
USER_HOME and USER_TERM before changing them in test-lib.sh, and add
options to 'test_pause' to optionally use these variables to invoke the
shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so
that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since
these three variables can break test reproducibility. Moreover, using the
original HOME means tests could overwrite files in a user's home
directory. Be explicit about these caveats in the new 'Usage' section in
test-lib-functions.sh.

Signed-off-by: Philippe Blain <[e-mail]>
---
 t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 40 insertions(+), 3 deletions(-)

Comments

Elijah Newren Aug. 28, 2021, 7:27 a.m. UTC | #1

On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget
<[e-mail]> wrote:
>
> From: Philippe Blain <[e-mail]>
>
> The 'test_pause' function, which is designed to help interactive
> debugging and exploration of tests, currently inherits the value of HOME
> and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It
> also invokes the shell defined by TEST_SHELL_PATH, which defaults to
> /bin/sh (through SHELL_PATH).
>
> Changing the value of HOME means that any customization configured in a
> developers' shell startup files and any Git aliases defined in their
> global Git configuration file are not available in the shell invoked by
> 'test_pause'.
>
> Changing the value of TERM to 'dumb' means that colored output
> is disabled for all commands in that shell.
>
> Using /bin/sh as the shell invoked by 'test_pause' is not ideal since
> some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and
> this shell is usually compiled without readline support, which makes for
> a poor interactive command line experience.
>
> To make the interactive command line experience in the shell invoked by
> 'test_pause' more pleasant, save the values of HOME and TERM in
> USER_HOME and USER_TERM before changing them in test-lib.sh, and add
> options to 'test_pause' to optionally use these variables to invoke the
> shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so
> that developer's interactive shell is used.
>
> We use options instead of changing the behaviour unconditionally since
> these three variables can break test reproducibility. Moreover, using the
> original HOME means tests could overwrite files in a user's home
> directory. Be explicit about these caveats in the new 'Usage' section in
> test-lib-functions.sh.
>
> Signed-off-by: Philippe Blain <[e-mail]>
> ---
>  t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-
>  t/test-lib.sh           |  6 ++++--
>  2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 1884177e293..1b775343adc 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -137,9 +137,44 @@ test_tick () {
>  # Stop execution and start a shell. This is useful for debugging tests.
>  #
>  # Be sure to remove all invocations of this command before submitting.
> +#
> +# Usage: test_pause [options]
> +#   -t
> +#      Use your original TERM instead of test-lib.sh's "dumb".
> +#      This usually restores color output in the invoked shell.
> +#      WARNING: this can break test reproducibility.
> +#   -s
> +#      Invoke $SHELL instead of $TEST_SHELL_PATH
> +#      WARNING: this can break test reproducibility.
> +#   -h
> +#      Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
> +#      This allows you to use your regular shell environment and Git aliases.
> +#      WARNING: this can break test reproducibility.
> +#      CAUTION: this can overwrite files in your HOME.

Do you want to add an option that implies the other three, so that
folks can do something like `test_pause -a` (or some other single
letter) rather than `test_pause -t -s -h`?

>
>  test_pause () {
> -       "$TEST_SHELL_PATH" <&6 >&5 2>&7
> +       PAUSE_TERM=$TERM &&
> +       PAUSE_SHELL=$TEST_SHELL_PATH &&
> +       PAUSE_HOME=$HOME &&
> +       while test $# != 0
> +       do
> +               case "$1" in
> +               -t)
> +                       PAUSE_TERM="$USER_TERM"
> +                       ;;
> +               -s)
> +                       PAUSE_SHELL="$SHELL"
> +                       ;;
> +               -h)
> +                       PAUSE_HOME="$USER_HOME"
> +                       ;;
> +               *)
> +                       break
> +                       ;;
> +               esac
> +               shift
> +       done &&
> +       TERM="$PAUSE_TERM" HOME="$PAUSE_HOME" "$PAUSE_SHELL" <&6 >&5 2>&7
>  }
>
>  # Wrap git with a debugger. Adding this to a command can make it easier
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index abcfbed6d61..132618991e2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -585,8 +585,9 @@ else
>         }
>  fi
>
> +USER_TERM="$TERM"
>  TERM=dumb
> -export TERM
> +export TERM USER_TERM
>
>  error () {
>         say_color error "error: $*"
> @@ -1380,9 +1381,10 @@ then
>  fi
>
>  # Last-minute variable setup
> +USER_HOME="$HOME"
>  HOME="$TRASH_DIRECTORY"
>  GNUPGHOME="$HOME/gnupg-home-not-used"
> -export HOME GNUPGHOME
> +export HOME GNUPGHOME USER_HOME
>
>  # Test repository
>  rm -fr "$TRASH_DIRECTORY" || {
> --
> gitgitgadget

Philippe Blain Aug. 28, 2021, 2:50 p.m. UTC | #2

Hi Elijah,

Le 2021-08-28 à 03:27, Elijah Newren a écrit :
> On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget
> <[e-mail]> wrote:
>> +#
>> +# Usage: test_pause [options]
>> +#   -t
>> +#      Use your original TERM instead of test-lib.sh's "dumb".
>> +#      This usually restores color output in the invoked shell.
>> +#      WARNING: this can break test reproducibility.
>> +#   -s
>> +#      Invoke $SHELL instead of $TEST_SHELL_PATH
>> +#      WARNING: this can break test reproducibility.
>> +#   -h
>> +#      Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
>> +#      This allows you to use your regular shell environment and Git aliases.
>> +#      WARNING: this can break test reproducibility.
>> +#      CAUTION: this can overwrite files in your HOME.
> 
> Do you want to add an option that implies the other three, so that
> folks can do something like `test_pause -a` (or some other single
> letter) rather than `test_pause -t -s -h`?
> 
>>
Yes, that would be even better. I'll add that for next version.

Thanks,

Philippe.

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..1b775343adc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -137,9 +137,44 @@  test_tick () {
 # Stop execution and start a shell. This is useful for debugging tests.
 #
 # Be sure to remove all invocations of this command before submitting.
+#
+# Usage: test_pause [options]
+#   -t
+#	Use your original TERM instead of test-lib.sh's "dumb".
+#	This usually restores color output in the invoked shell.
+#	WARNING: this can break test reproducibility.
+#   -s
+#	Invoke $SHELL instead of $TEST_SHELL_PATH
+#	WARNING: this can break test reproducibility.
+#   -h
+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+#	This allows you to use your regular shell environment and Git aliases.
+#	WARNING: this can break test reproducibility.
+#	CAUTION: this can overwrite files in your HOME.
 
 test_pause () {
-	"$TEST_SHELL_PATH" <&6 >&5 2>&7
+	PAUSE_TERM=$TERM &&
+	PAUSE_SHELL=$TEST_SHELL_PATH &&
+	PAUSE_HOME=$HOME &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			PAUSE_TERM="$USER_TERM"
+			;;
+		-s)
+			PAUSE_SHELL="$SHELL"
+			;;
+		-h)
+			PAUSE_HOME="$USER_HOME"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	TERM="$PAUSE_TERM" HOME="$PAUSE_HOME" "$PAUSE_SHELL" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier
diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -585,8 +585,9 @@  else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"
@@ -1380,9 +1381,10 @@  then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {