Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'
From: Jeff King <hidden>
Date: 2018-01-26 18:42:12
On Fri, Jan 26, 2018 at 01:37:06PM +0100, SZEDER Gábor wrote:
When checking a git command's output with 'test_i18ngrep', it's
tempting to conveniently pipe the git command's standard output into
'test_i18ngrep'. Unfortunately, this is an anti-pattern, because it
hides the git command's exit code, and the test could continue even if
the command exited with error.
Add a bit of linting to 'test_i18ngrep' to detect when data is fed to
its standard input and to error out with a "bug in the test script"
message.
Note that this change will also forbid cases where 'test_i18ngrep'
would legitimately read its standard input, e.g.
- when its standard input is redirected from a file, or
- when a git command's standard output is first written to an
intermediate file, which is then preprocessed by a non-git command
before the results are piped into 'test_i18ngrep'.
See two of the previous patches for the only such cases we had in our
test suite. However, reliably preventing this antipattern is arguably
more important than supporting these cases, which can be worked around
by only minor inconveniences.The idea seems reasonable to me. Let's think about what the escape hatch looks like to work around it if you need to. I guess you've got: cat >file && test_i18ngrep ... file which is not too bad. You've also got: test_i18ngrep ... - though that relies on the underlying grep understanding "-" (which is in POSIX, though with a rather vague "if the implementations supports it"). And it wouldn't work with the "read" test in this patch.
quoted hunk ↗ jump to hunk
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 92ed02937..e381d50d0 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh@@ -719,6 +719,10 @@ test_i18ncmp () { # under GETTEXT_POISON this pretends that the command produced expected # results. test_i18ngrep () { + ( read line ) && + error "bug in the test script: data on test_i18ngrep's stdin;" \ + "perhaps a git command's output is piped into it?" +
This seems kind of hacky compared to just seeing if there is a file argument. But I suppose that is hard to do, since we just pass through the arguments to grep. Though looking at our test_18ngrep invocations, they are simple enough that would just ask "are there two non-option arguments at the end of the command line". The exception is "-e", but IMHO we could just drop that. It serves no purpose unless you're trying to hide a "-" at the start of your pattern, and in fact we used to ban it since sysv grep didn't understand it (e.g., aadbe44f883). -Peff