| Message ID | 20221121134040.12260-1-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 288fcb1c942d582b915a4c824c6b87a94ae875a7 |
| Headers | show |
| Series | t5516: fail to run in verbose mode | expand |
> 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 subcommand > "git pack-objects" will contain an argument "--progress" > instead of "-q". Adding a specific option "--quiet" to "git push" will get a stable > result for t5516. > That's true. Thanks for noticing and fixing. I didn't consider this usage of test case, and it works after your patch. Thanks, Kyle
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.
On Tue, Nov 22, 2022 at 01:19:39PM +0900, Junio C Hamano wrote: > 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
Jeff King <peff@peff.net> writes: > 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. Yes, this is tempting (and I almost brought it up in my response), and a pipe to "|cat" may be hopefully closer than tester's tty to the redirection to "/dev/null". I didn't know how much closer, though, and the differences may still matter (we could teach "git grep" or "git diff --exit-code" to notice that the output is sent to /dev/null and stop at the first hit, for example), but still "|cat" is closer than ">/dev/tty". > None of which needs to hold up this patch, but I wonder if we'd want to > pursue the larger fix in (1). Yeah, I agree. Thanks.
On Wed, Nov 23, 2022 at 10:17:29AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > 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. > > Yes, this is tempting (and I almost brought it up in my response), > and a pipe to "|cat" may be hopefully closer than tester's tty to > the redirection to "/dev/null". I didn't know how much closer, > though, and the differences may still matter (we could teach "git > grep" or "git diff --exit-code" to notice that the output is sent to > /dev/null and stop at the first hit, for example), but still "|cat" > is closer than ">/dev/tty". One thing I'd worry about is buffering. One of the nice things about "-v" is that there is nothing between you and the running programs, so you are much less likely to be fooled about the order of events in the output. Or wondering why nothing is happening because real-time output seems to have stalled. But piping through "cat" may end up with weird pauses while it fills up a 4k buffer. Using stdbuf could help, but that's far from portable. -Peff
Jeff King <peff@peff.net> writes: > One thing I'd worry about is buffering. One of the nice things about > "-v" is that there is nothing between you and the running programs, so > you are much less likely to be fooled about the order of events in the > output. Or wondering why nothing is happening because real-time output > seems to have stalled. But piping through "cat" may end up with weird > pauses while it fills up a 4k buffer. Using stdbuf could help, but > that's far from portable. We could pipe to "dd bs=1 conv=fsync" (tongue-in-cheek---I think conv=fsync is a GNU thing).
On Fri, Nov 25, 2022 at 01:58:55PM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > One thing I'd worry about is buffering. One of the nice things about > > "-v" is that there is nothing between you and the running programs, so > > you are much less likely to be fooled about the order of events in the > > output. Or wondering why nothing is happening because real-time output > > seems to have stalled. But piping through "cat" may end up with weird > > pauses while it fills up a 4k buffer. Using stdbuf could help, but > > that's far from portable. > > We could pipe to "dd bs=1 conv=fsync" (tongue-in-cheek---I think > conv=fsync is a GNU thing). We don't need the fsync; we are just worried about in-process buffering, not kernel-level flushing. So bs=1 is sufficient, in that it would use syscalls to read/write single bytes. It would just be horribly inefficient. We really just want immediate partial read()/write() but with sensible buffer sizes. It would be easy to write a 5-line C program that did this if we really wanted to. I'm not entirely convinced it's worth worrying too much about, though. -Peff
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 4f2bfaf005..98a27a2948 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1858,19 +1858,19 @@ test_expect_success 'push with config push.useBitmaps' ' git checkout main && test_unconfig push.useBitmaps && GIT_TRACE2_EVENT="$PWD/default" \ - git push testrepo main:test && + git push --quiet testrepo main:test && test_subcommand git pack-objects --all-progress-implied --revs --stdout \ --thin --delta-base-offset -q <default && test_config push.useBitmaps true && GIT_TRACE2_EVENT="$PWD/true" \ - git push testrepo main:test2 && + git push --quiet testrepo main:test2 && test_subcommand git pack-objects --all-progress-implied --revs --stdout \ --thin --delta-base-offset -q <true && test_config push.useBitmaps false && GIT_TRACE2_EVENT="$PWD/false" \ - git push testrepo main:test3 && + git push --quiet testrepo main:test3 && test_subcommand git pack-objects --all-progress-implied --revs --stdout \ --thin --delta-base-offset -q --no-use-bitmap-index <false '