Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

[SZEDER Gábor]

On Sat, Mar 3, 2018 at 8:12 AM, Jeff King  wrote:
> On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:
>
>> The first patch is the most important: with a couple of well-placed file
>> descriptor redirections it ensures that the stderr of the test helper
>> functions running git commands only contain the stderr of the tested
>> command, thereby resolving over 90% of the failures resulting from
>> running the test suite with '-x' and /bin/sh.
>
> I dunno. It seems like this requires a lot of caveats for people using
> subshells and shell functions, and I suspect it's going to be an
> on-going maintenance burden.

After finally figuring out the redirections in the first patch, I was
quite surprised by how few failing tests remained.  We only gathered 28
such tests over all these years; if it continues at this rate, that
probably won't be that much of a burden.  And the second patch provides
an escape hatch, should it ever be needed.

The current situation, however, is a burden much more frequently,
because the idiosyncrasies of TEST_SHELL_PATH and/or '--verbose-log' pop
up whenever trying to run any test script with '-x' that has such a test
in it.

I think this is the right tradeoff.

> That said, I'm not opposed if you want to do the work to try to get the
> whole test-suite clean, and we can see how it goes from there. It
> shouldn't be hurting anything, I don't think, aside from some
> mysterious-looking redirects (but your commit messages seem to explain
> it, so anybody can dig).
>
> Does it make descriptor 7 magical, and something that scripts should
> avoid touching? That would mean we have 2 magical descriptors now.

Tests can still use fd 7 as long as they don't intend to attach it
directly to that particular git command that is run inside one of these
test helper functions.

I settled on fd 7 because that fd is already used as stderr for the
'test_pause' and 'debug' helper functions and it isn't used in any of
our tests.

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

[Jeff King]

On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:

> The first patch is the most important: with a couple of well-placed file
> descriptor redirections it ensures that the stderr of the test helper
> functions running git commands only contain the stderr of the tested
> command, thereby resolving over 90% of the failures resulting from
> running the test suite with '-x' and /bin/sh.

I dunno. It seems like this requires a lot of caveats for people using
subshells and shell functions, and I suspect it's going to be an
on-going maintenance burden.

That said, I'm not opposed if you want to do the work to try to get the
whole test-suite clean, and we can see how it goes from there. It
shouldn't be hurting anything, I don't think, aside from some
mysterious-looking redirects (but your commit messages seem to explain
it, so anybody can dig).

Does it make descriptor 7 magical, and something that scripts should
avoid touching? That would mean we have 2 magical descriptors now.

-Peff

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

[SZEDER Gábor]

On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor  wrote:
> This patch series makes '-x' tracing of tests work reliably even when
> running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be
> unnecessary.
>
>   make GIT_TEST_OPTS='-x --verbose-log' test
>
> passes on my setup and on all Travis CI build jobs (though neither me
> nor Travis CI run all tests, e.g. CVS).

I installed 'cvs' and whatnot to run t94* and t96* tests, and sure
enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.
I think I will be able to get around to send v2 during the weekend.

[PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

[SZEDER Gábor]

This patch series makes '-x' tracing of tests work reliably even when
running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be
unnecessary.

  make GIT_TEST_OPTS='-x --verbose-log' test

passes on my setup and on all Travis CI build jobs (though neither me
nor Travis CI run all tests, e.g. CVS).


The first patch is the most important: with a couple of well-placed file
descriptor redirections it ensures that the stderr of the test helper
functions running git commands only contain the stderr of the tested
command, thereby resolving over 90% of the failures resulting from
running the test suite with '-x' and /bin/sh.

Most of the following patches resolve the remaining failures, one test
script at a time, in most cases by limiting the scope of stderr
redirections from functions and subshells to the tested git commands.
Except the second and ninth patches, which, arguably, could be
considered as cheating...  I admit, my enthusiasm suddenly run out when
I saw t1510 :)

The last two patches are just finishing touches with a bit of
documentation updates and enabling '-x' tracing in Travis CI build jobs.


There is currently nothing in 'pu' that would require additional fixes
to make this patch series work.


SZEDER Gábor (11):
  t: prevent '-x' tracing from interfering with test helpers' stderr
  t: add means to disable '-x' tracing for individual test scripts
  t1507-rev-parse-upstream: don't check the stderr of a shell function
  t3030-merge-recursive: don't check the stderr of a subshell
  t5500-fetch-pack: don't check the stderr of a subshell
  t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file
  t5570-git-daemon: don't check the stderr of a subshell
  t9903-bash-prompt: don't check the stderr of __git_ps1()
  t1510-repo-setup: mark as untraceable with '-x'
  t/README: add a note about don't saving stderr of compound commands
  travis-ci: run tests with '-x' tracing

 ci/lib-travisci.sh|  2 +-
 t/README  | 23 +++---
 t/lib-terminal.sh |  4 ++--
 t/t1507-rev-parse-upstream.sh | 14 +++---
 t/t1510-repo-setup.sh |  4 
 t/t3030-merge-recursive.sh| 36 +++
 t/t5500-fetch-pack.sh | 12 ++--
 t/t5526-fetch-submodules.sh   |  2 +-
 t/t5570-git-daemon.sh |  2 +-
 t/t9903-bash-prompt.sh| 14 ++
 t/test-lib-functions.sh   | 24 +++
 t/test-lib.sh | 19 +-
 12 files changed, 94 insertions(+), 62 deletions(-)

-- 
2.16.2.400.g911b7cc0da