On Wed, Oct 09, 2019 at 07:25:51PM +0200, SZEDER Gábor wrote: > > Probably they'd be easy enough to fix (and they're out of tree anyway), > > so I'm not really arguing against the escape hatch exactly. Mostly I'm > > just surprised that if I introduced 3 cases (out of probably a dozen > > scripts), I'm surprised that more contributors aren't accidentally doing > > so upstream. > > I see it a bit differently. Over a decade we gathered about > twenty-something such tests cases: that's about two cases per year. > You added three such cases in about a year and a half: that's two > cases per year. The numbers add up perfectly, you singlehandedly took > care of everything ;) Those cases are actually much older than that. I just didn't bother to clean them up until recently. So my rate is even lower :) > - Some shells do include file descriptor redirections in the trace > output, and it varies between implementations to which fd the > trace of the redirection goes. > > - 'ksh/ksh93' and NetBSD's /bin/sh send the trace of > redirections to the "wrong" fd, in the sense that e.g. the > trace of commands invoked in 'test_must_fail' goes to the > function's standard error, and checking its stderr with > 'test_cmp' would then fail. > > (But 'ksh/ksh93' doesn't really matter, because they don't > support the 'local' keyword, so they fail a bunch of tests > even without '-x' anyway.) > > I don't think we can do anything about these shells. Yeah, unless somebody is complaining, I don't know that it's worth worrying about too much. The test suite is certainly useful without being able to use "-x" on every single test run (you can still run it without "-x" obviously, or selectively use "-x" to debug a single test or script). So if it is only unreliable on a few tests on a few obscure shells, we can probably live with it until somebody demonstrates a real-world problem (e.g., that they're running automated CI on an obscure platform that is stuck with an old shell, and really want "-x --verbose-log" to get more verbose failures). > - We do call 'test_have_prereq' from within test cases as well, > notably from the 'test_i18ngrep', 'test_i18ncmp' and > 'test_ln_s_add' helper functions. In those cases all trace output > from 'test_have_prereq' is included in the test case's trace > output, which means that during the first invocation: > > - there is lots of distracting and confusing trace output, as > the script evaluating the prereq is passed around to a bunch > of functions. Yeah, I think this is probably an issue even with bash. > As far as 'test_i18ngrep' is concerned, which accounts for the > majority of 'test_have_prereq' invocations within test cases, I > don't understand why it uses 'test_have_prereq' in the first place > instead of checking the GIT_TEST_GETTEXT_POISON environment > variable; and 6cdccfce1e (i18n: make GETTEXT_POISON a runtime > option, 2018-11-08) doesn't give me any insight. I think it's just that checking the environment variable is non-trivial: we invoke env--helper to handle bool interpretation. So we'd prefer to cache the result (and not to run it at all if a test script doesn't use i18ngrep, though it's perhaps ubiquitous enough that we should just run it up front for every script). > I recall that some months ago we discussed the idea of how to > disable trace output from within test helper functions; that would > help with this 'test_have_prereq' issue as well, at least in case > of the more "common" shells. That might be worth doing, though IIRC it got kind of hairy. :) -Peff
