Re: [PATCH] t5516: fail to run in verbose mode

From: Jeff King <peff@peff.net>
Date: Nov 22, 2022, 19:08 UTC
Message-ID: <Y30eK/mNYeLNtOi2@coredump.intra.peff.net>
In-Reply-To: <xmqqo7szsjs4.fsf@gitster.g>

On Tue, Nov 22, 2022 at 01:19:39PM +0900, Junio C Hamano wrote:

Show 20 quoted lines

> Jiang Xin <worldhello.net@gmail.com> writes:
> 
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > The test case "push with config push.useBitmap" of t5516 was introduced
> > in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,
> > 2022-06-17). It won't work in verbose mode, e.g.:
> >
> >     $ sh t5516-fetch-push.sh --run='1,115' -v
> >
> > This is because "git-push" will run in a tty in this case, and the
> 
> Right.  "-v" involves redirecting the stdout/stderr of the commands
> being run in the test to stdout/stderr in the environment the tests
> are run, so
> 
>     $ sh t5516-fetch-push.sh --run='1,115' -v >log 2>&1
> 
> would have succeeded correctly.  Forcing the behaviour with the
> "--quiet" option is certainly a good way to gain stability.

I agree this is a good fix for now, but I wonder philosophically what it means. That is, I could see two arguments:

  1. Our tests sometimes run with stderr connected to a tty and
     sometimes not. This means the test environment isn't consistent,
     and perhaps we should be piping all "-v" tests through "cat" or
     something so that the environment is stable.

  2. Having "-v" run through a tty is yet another source of variation
     that may help us find bugs (though of course sometimes it finds
     false positives). I'm a little uncomfortable with this just because
     it's not providing us variation in any kind of planned or cohesive
     way. This failure lasted for months before somebody noticed it was
     broken under "-v".

     But if we take that approach, then tests are responsible for
     handling both cases. Passing "--quiet" here works, though it feels
     like making test_subcommand more flexible is the right fix. Looks
     like we had an "inexact" variant at one point, but it was dropped
     (because it was buggy) in 16dcec218b (test-lib-functions: remove
     test_subcommand_inexact, 2022-03-25).

None of which needs to hold up this patch, but I wonder if we'd want to pursue the larger fix in (1).

-Peff