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

On Wed, Sep 1, 2021 at 1:26 PM Junio C Hamano [off-list ref] wrote:

"Philippe Blain via GitGitGadget" [off-list ref] writes:

quoted

+# 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.