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

Commit Message

Philippe Blain Sept. 1, 2021, 1:31 p.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 | 44 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 47 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Sept. 1, 2021, 8:26 p.m. UTC | #1

"Philippe Blain via GitGitGadget" <[e-mail]> writes:

> +# 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.
> +#   -a
> +#	Shortcut for -t -s -h

As this is not end-user facing but facing our developer, I do not
deeply care, but I find the warnings in this help text problematic.

Because a new process instance of $PAUSE_SHELL is run, the options
you add when inserting test_pause does not affect what happens in
the tests after you exit the $PAUSE_SHELL [*], right?

Of course, you can modify the repository or the working tree files
used in the test in the $PAUSE_SHELL, and that can break "test
reproducibility"---if you run "git ls-files -s" and take its output
in a temporary file, a step later in the test that runs "git status"
will see an extra untracked file, for example, and such a change may
(or may not) unnecessarily break the tests.

But it is not anything new introduced by these options.  It is
inherent to test_pause itself.

If we want to add a warning to the help text here, I think it should
be written in such a way that it is clear that the warning applies
to any use of the test_pause helper, not just to the form with the
options.

Thanks.


[Footnote]

* If we had an alternative implementation of test_pause that does
  not use a separate $PAUSE_SHELL process, but instead like
  inserting a read-eval-print loop, that would be far more powerful
  and useful debugging aid.  You can not just stop the execution of
  the test, and observe the files in the test repository and the
  environment variables---you can also access shell variables and
  functions.

  Such a test_pause from another world would deserve a "if you touch
  anything, the damage is permanent" warning even more than the
  current one, because you can modify even a shell variable to
  affect the execution of the test after you leave the paused state.

Elijah Newren Sept. 1, 2021, 9:52 p.m. UTC | #2

On Wed, Sep 1, 2021 at 1:26 PM Junio C Hamano <[e-mail]> wrote:
>
> "Philippe Blain via GitGitGadget" <[e-mail]> writes:
>
> > +# 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.
> > +#   -a
> > +#    Shortcut for -t -s -h
>
> As this is not end-user facing but facing our developer, I do not
> deeply care, but I find the warnings in this help text problematic.
>
> Because a new process instance of $PAUSE_SHELL is run, the options
> you add when inserting test_pause does not affect what happens in
> the tests after you exit the $PAUSE_SHELL [*], right?

I think the warning was less about what happens after test_pause is
complete and the testcase resumes, and more intended to convey that if
the user tries to manually copy & paste snippets of code from the test
into $PAUSE_SHELL, then the use of these flags can cause the result of
those pasted commands to differ.  If so, a small rewording might be in
order, e.g. "WARNING: this can cause commands run in the paused shell
to give different results".  I'd also say the CAUTION would perhaps
benefit from some rewording (since the option itself doesn't cause
files to be overwritten), e.g. "CAUTION: using this option and copying
commands from the test script into the paused shell might result in
files in your HOME being overwritten".

> Of course, you can modify the repository or the working tree files
> used in the test in the $PAUSE_SHELL, and that can break "test
> reproducibility"---if you run "git ls-files -s" and take its output
> in a temporary file, a step later in the test that runs "git status"
> will see an extra untracked file, for example, and such a change may
> (or may not) unnecessarily break the tests.
>
> But it is not anything new introduced by these options.  It is
> inherent to test_pause itself.
>
> If we want to add a warning to the help text here, I think it should
> be written in such a way that it is clear that the warning applies
> to any use of the test_pause helper, not just to the form with the
> options.
>
> Thanks.
>
>
> [Footnote]
>
> * If we had an alternative implementation of test_pause that does
>   not use a separate $PAUSE_SHELL process, but instead like
>   inserting a read-eval-print loop, that would be far more powerful
>   and useful debugging aid.  You can not just stop the execution of
>   the test, and observe the files in the test repository and the
>   environment variables---you can also access shell variables and
>   functions.
>
>   Such a test_pause from another world would deserve a "if you touch
>   anything, the damage is permanent" warning even more than the
>   current one, because you can modify even a shell variable to
>   affect the execution of the test after you leave the paused state.

Junio C Hamano Sept. 1, 2021, 11:09 p.m. UTC | #3

Elijah Newren <[e-mail]> writes:

>> Because a new process instance of $PAUSE_SHELL is run, the options
>> you add when inserting test_pause does not affect what happens in
>> the tests after you exit the $PAUSE_SHELL [*], right?
>
> I think the warning was less about what happens after test_pause is
> complete and the testcase resumes, and more intended to convey that if
> the user tries to manually copy & paste snippets of code from the test
> into $PAUSE_SHELL, then the use of these flags can cause the result of
> those pasted commands to differ.

But the difference of TERM and SHELL are much smaller issue when one
cuts and pastes the lines from the test script.  Even if you do not
use -s or -t options, shell variables and helpers won't be available
to the interactive session given by test_pause, and that is a much
bigger difference between the "real" shell environment that is
running the tests and the user's shell environment test_pause gives
us.  That is why I think the WARNING should be attached _directly_
to the description of test_pause and not to individual "this uses
different X environment compared to the real shell environment
running the tests" options.

The updated $HOME does deserve a special mention, as .gitconfig from
it would further affect the tests and hide the bug you are hunting
for, and the CAUTION is a good thing to have, even if you do not
plan to modify anything in the test_pause sight-seeing session.

Philippe Blain Sept. 2, 2021, 1:10 p.m. UTC | #4

Hi Elijah and Junio,

Le 2021-09-01 à 19:09, Junio C Hamano a écrit :
> Elijah Newren <[e-mail]> writes:
> 
>>> Because a new process instance of $PAUSE_SHELL is run, the options
>>> you add when inserting test_pause does not affect what happens in
>>> the tests after you exit the $PAUSE_SHELL [*], right?
>>
>> I think the warning was less about what happens after test_pause is
>> complete and the testcase resumes, and more intended to convey that if
>> the user tries to manually copy & paste snippets of code from the test
>> into $PAUSE_SHELL, then the use of these flags can cause the result of
>> those pasted commands to differ.

Yes, I added the warnings based on the reticence I got in v1 to change the
behaviour without adding flags.

> 
> But the difference of TERM and SHELL are much smaller issue when one
> cuts and pastes the lines from the test script.  Even if you do not
> use -s or -t options, shell variables and helpers won't be available
> to the interactive session given by test_pause, and that is a much
> bigger difference between the "real" shell environment that is
> running the tests and the user's shell environment test_pause gives
> us.  That is why I think the WARNING should be attached _directly_
> to the description of test_pause and not to individual "this uses
> different X environment compared to the real shell environment
> running the tests" options.

OK, I can make that tweak.


Le 2021-09-01 à 17:52, Elijah Newren a écrit :
-- snip --
> those pasted commands to differ.  If so, a small rewording might be in
> order, e.g. "WARNING: this can cause commands run in the paused shell
> to give different results".  I'd also say the CAUTION would perhaps
> benefit from some rewording (since the option itself doesn't cause
> files to be overwritten), e.g. "CAUTION: using this option and copying
> commands from the test script into the paused shell might result in
> files in your HOME being overwritten".

Thanks for these suggestions, that wording does seem clearer.

Philippe.

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..4b667dc7e76 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -137,9 +137,51 @@  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.
+#   -a
+#	Shortcut for -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"
+			;;
+		-a)
+			PAUSE_TERM="$USER_TERM"
+			PAUSE_SHELL="$SHELL"
+			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" || {