From: SZEDER Gábor <hidden> Date: 2018-02-09 02:43:04
To check that a git command fails with the expected error message, we
usually execute a command like this:
test_must_fail git command --option 2>output.err
Alas, this command doesn't limit the redirection to the git command,
but it redirects the standard error of the 'test_must_fail' helper
function as well, causing various issues discussed in detail in the
second patch. Therefore that patch introduces the 'test_must_fail
stderr=<file>' option to save the executed git command's standard
error to the given file.
The last patch converts one test script to use 'test_must_fail
stderr=<file>' to demonstrate its benefits: thereafter that script
will succeed with '-x'. There are plenty more places to convert:
$ git grep -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l
430
$ git grep --name-only -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l
135
... and this doesn't even count commands spanning more lines, and
there are more in 'pu'.
I didn't convert more test scripts, because it's boring ;) but more
importantly because it could give us 135+ GSoC micro projects.
SZEDER Gábor (3):
t: document 'test_must_fail ok=<signal-name>'
t: teach 'test_must_fail' to save the command's stderr to a file
t1404: use 'test_must_fail stderr=<file>'
t/README | 20 +++++++++++++++++--
t/t1404-update-ref-errors.sh | 46 ++++++++++++++++++++++----------------------
t/test-lib-functions.sh | 45 +++++++++++++++++++++++++++++++++----------
3 files changed, 76 insertions(+), 35 deletions(-)
--
2.16.1.180.g07550b0b1b
From: SZEDER Gábor <hidden> Date: 2018-02-09 02:43:10
Since 'test_might_fail' is implemented as a thin wrapper around
'test_must_fail', it also accepts the same options. Mention this in
the docs as well.
Signed-off-by: SZEDER Gábor <redacted>
---
t/README | 14 ++++++++++++--
t/test-lib-functions.sh | 10 ++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
@@ -655,7 +655,7 @@ library for your script to use. test_expect_code 1 git merge "merge msg" B master '- - test_must_fail <git-command>+ - test_must_fail [<options>] <git-command> Run a git command and ensure it fails in a controlled way. Use this instead of "! <git-command>". When git-command dies due to a
@@ -663,11 +663,21 @@ library for your script to use. treats it as just another expected failure, which would let such a bug go unnoticed.- - test_might_fail <git-command>+ Accepts the following options:++ ok=<signal-name>[,<...>]:+ Don't treat an exit caused by the given signal as error.+ Multiple signals can be specified as a comma separated list.+ Currently recognized signal names are: sigpipe, success.+ (Don't use 'success', use 'test_might_fail' instead.)++ - test_might_fail [<options>] <git-command> Similar to test_must_fail, but tolerate success, too. Use this instead of "<git-command> || :" to catch failures due to segv.+ Accepts the same options as test_must_fail.+ - test_cmp <expected> <actual> Check whether the content of the <actual> file matches the
@@ -610,6 +610,14 @@ list_contains () {## Writing this as "! git checkout ../outerspace" is wrong, because# the failure could be due to a segv. We want a controlled failure.+#+# Accepts the following options:+#+# ok=<signal-name>[,<...>]:+# Don't treat an exit caused by the given signal as error.+# Multiple signals can be specified as a comma separated list.+# Currently recognized signal names are: sigpipe, success.+# (Don't use 'success', use 'test_might_fail' instead.) test_must_fail(){case"$1"in
@@ -656,6 +664,8 @@ test_must_fail () {## Writing "git config --unset all.configuration || :" would be wrong,# because we want to notice if it fails due to segv.+#+# Accepts the same options as test_must_fail. test_might_fail(){test_must_failok=success"$@"
From: SZEDER Gábor <hidden> Date: 2018-02-09 02:43:12
To check that a git command fails and to inspect its error message we
usually execute a command like this throughout our test suite:
test_must_fail git command --option 2>output.err
Note that this command doesn't limit the redirection to the git
command, but it redirects the standard error of the 'test_must_fail'
helper function as well. This is wrong for several reasons:
- If that git command were to succeed or die for an unexpected
reason e.g. signal, then 'test_must_fail's helpful error message
would end up in the given file instead of on the terminal or in
't/test-results/$T.out', when the test is run with '-v' or
'--verbose-log', respectively.
- If the test is run with '-x', the trace of executed commands in
'test_must_fail' will go to stderr as well (except for more recent
Bash versions supporting $BASH_XTRACEFD), and then in turn will
end up in the given file.
- If the git command's error message is checked verbatim with
'test_cmp', then the '-x' trace will cause the test to fail.
- Though rather unlikely, if the git command's error message is
checked with 'test_i18ngrep', then its pattern might match
'test_must_fail's error message (or '-x' trace) instead of the
given git command's, potentially hiding a bug.
Teach the 'test_must_fail' helper the 'stderr=<file>' option to save
the executed git command's standard error to that file, so we can
avoid all the bad side effects of redirecting the whole thing's
standard error.
The same issues apply to the 'test_might_fail' helper function as
well, but since it's implemented as a thin wrapper around
'test_must_fail', no modifications were necessary and 'test_might_fail
stderr=output.err git command' will just work.
Signed-off-by: SZEDER Gábor <redacted>
---
Notes:
I considered adding an analogous 'stdout=<file>' option as well, but in
the end haven't, because:
- 'test_must_fail' doesn't print anything to its standard output, so a
plain '>out' redirection at the end of the line doesn't have any
undesirable side effects.
- We would need four conditions to cover all possible combinations of
'stdout=' and 'stderr='. Considering the above point, I don't think
it's worth it.
t/README | 6 ++++++
t/test-lib-functions.sh | 35 +++++++++++++++++++++++++----------
2 files changed, 31 insertions(+), 10 deletions(-)
@@ -430,6 +430,10 @@ Don't: use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault).+ Don't redirect 'test_must_fail's standard error like+ 'test_must_fail git cmd 2>err'. Instead, run 'test_must_fail+ stderr=err git cmd'.+ On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works.
@@ -670,6 +674,8 @@ library for your script to use. Multiple signals can be specified as a comma separated list. Currently recognized signal names are: sigpipe, success. (Don't use 'success', use 'test_might_fail' instead.)+ stderr=<file>:+ Save <git-command>'s standard error to <file>. - test_might_fail [<options>] <git-command>
@@ -618,18 +618,33 @@ list_contains () {# Multiple signals can be specified as a comma separated list.# Currently recognized signal names are: sigpipe, success.# (Don't use 'success', use 'test_might_fail' instead.)+# stderr=<file>:+# Save the git command's standard error to <file>. test_must_fail(){-case"$1"in-ok=*)-_test_ok=${1#ok=}-shift-;;-*)-_test_ok=-;;-esac-"$@"+_test_ok=_test_stderr=+whiletest$#-gt0+do+case"$1"in+stderr=*)+_test_stderr=${1#stderr=}+shift+;;+ok=*)+_test_ok=${1#ok=}+shift+;;+*)+break+;;+esac+done+iftest-n"$_test_stderr"+then+"$@"2>"$_test_stderr"+else+"$@"+fiexit_code=$?iftest$exit_code-eq0&&!list_contains"$_test_ok"successthen
From: SZEDER Gábor <hidden> Date: 2018-02-09 02:43:16
Instead of 'test_must_fail git cmd... 2>output.err', which redirects
the standard error of the 'test_must_fail' helper function as well,
causing various issues as discussed in the previous patch.
With this patch t1404 succeeds when run with '-x', even with shells
not supporting $BASH_XTRACEFD.
Signed-off-by: SZEDER Gábor <redacted>
---
t/t1404-update-ref-errors.sh | 46 ++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
@@ -612,7 +612,7 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '# Now try to delete it while the `packed-refs` lock is held::>.git/packed-refs.lock&&test_when_finished"rm -f .git/packed-refs.lock"&&-test_must_failgitupdate-ref-d$prefix/foo>out2>err&&+test_must_failstderr=errgitupdate-ref-d$prefix/foo>out&&gitfor-each-ref$prefix>actual&&test_i18ngrep"Unable to create $Q.*packed-refs.lock$Q: File exists"err&&test_cmpunchangedactual
From: Eric Sunshine <hidden> Date: 2018-02-09 03:11:17
On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor [off-list ref] wrote:
quoted hunk
To check that a git command fails and to inspect its error message we
usually execute a command like this throughout our test suite:
test_must_fail git command --option 2>output.err
Note that this command doesn't limit the redirection to the git
command, but it redirects the standard error of the 'test_must_fail'
helper function as well. This is wrong for several reasons:
[...snip rationale...]
Signed-off-by: SZEDER Gábor <redacted>
---
diff --git a/t/README b/t/README
@@ -430,6 +430,10 @@ Don't: use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault).+ Don't redirect 'test_must_fail's standard error like+ 'test_must_fail git cmd 2>err'. Instead, run 'test_must_fail+ stderr=err git cmd'.
Perhaps not worth a re-roll, but it might be nice to cite a bit of the
rationale from the commit message for the "don't redirect" rule since
it's not necessarily obvious for readers of t/README why this
limitation exists (and it seems unlikely they would check this commit
message). The rationale here doesn't necessarily need to go into
exquisite detail of the commit message, but could be perhaps as simple
as:
Don't redirect 'test_must_fail's standard error to a file (e.g.
'test_must_fail git cmd 2>err') since error output from commands
run internally by 'test_must_fail' may pollute file "err".
Instead, run 'test_must_fail stderr=err git cmd'.
From: Eric Sunshine <hidden> Date: 2018-02-09 03:16:07
On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor [off-list ref] wrote:
Instead of 'test_must_fail git cmd... 2>output.err', which redirects
the standard error of the 'test_must_fail' helper function as well,
causing various issues as discussed in the previous patch.
ECANTPARSE: This either wants to say:
"Instead of <foo>, do <bar>."
or:
"'test_must_fail ... 2>...', which redirects ...
causes various issues discussed..."
but fails to say either.
With this patch t1404 succeeds when run with '-x', even with shells
not supporting $BASH_XTRACEFD.
Signed-off-by: SZEDER Gábor <redacted>
From: SZEDER Gábor <hidden> Date: 2018-02-09 03:33:28
On Fri, Feb 9, 2018 at 4:16 AM, Eric Sunshine [off-list ref] wrote:
On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor [off-list ref] wrote:
quoted
Instead of 'test_must_fail git cmd... 2>output.err', which redirects
the standard error of the 'test_must_fail' helper function as well,
causing various issues as discussed in the previous patch.
ECANTPARSE: This either wants to say:
"Instead of <foo>, do <bar>."
The "do <bar>" part is in the subject line.
or:
"'test_must_fail ... 2>...', which redirects ...
causes various issues discussed..."
Well, at this point I got the ECANTPARSE :)
but fails to say either.
quoted
With this patch t1404 succeeds when run with '-x', even with shells
not supporting $BASH_XTRACEFD.
Signed-off-by: SZEDER Gábor <redacted>
From: Jeff King <hidden> Date: 2018-02-09 14:21:40
On Fri, Feb 09, 2018 at 03:42:34AM +0100, SZEDER Gábor wrote:
To check that a git command fails and to inspect its error message we
usually execute a command like this throughout our test suite:
test_must_fail git command --option 2>output.err
Note that this command doesn't limit the redirection to the git
command, but it redirects the standard error of the 'test_must_fail'
helper function as well. This is wrong for several reasons:
- If that git command were to succeed or die for an unexpected
reason e.g. signal, then 'test_must_fail's helpful error message
would end up in the given file instead of on the terminal or in
't/test-results/$T.out', when the test is run with '-v' or
'--verbose-log', respectively.
- If the test is run with '-x', the trace of executed commands in
'test_must_fail' will go to stderr as well (except for more recent
Bash versions supporting $BASH_XTRACEFD), and then in turn will
end up in the given file.
I have to admit that I'm slightly negative on this approach, just
because:
1. It requires every caller to do something special, rather than just
using normal redirection. And the feature isn't as powerful as
redirection. E.g., some callers do:
test_must_fail foo >output 2>&1
But:
test_must_fail stderr=output foo >output
is not quite right (stdout and stderr outputs may clobber each
other, because they have independent position pointers).
2. The "-x" problems aren't specific to test_must_fail at all. They're
a general issue with shell functions.
I'm not entirely happy with saying "if you want to use -x, please use
bash". But given that it actually solves the problems everywhere with no
further effort, is it really that bad a solution?
For the error messages from test_must_fail, could we go in the same
direction, and send them to descriptor 4 rather than 2? We've already
staked out descriptor 4 as something magical that must be left alone
(see 9be795fb). If we can rely on that, then it becomes a convenient way
for functions to make sure their output is going to the script's stderr.
-Peff
From: SZEDER Gábor <hidden> Date: 2018-02-23 23:26:49
On Fri, Feb 9, 2018 at 3:21 PM, Jeff King [off-list ref] wrote:
On Fri, Feb 09, 2018 at 03:42:34AM +0100, SZEDER Gábor wrote:
quoted
To check that a git command fails and to inspect its error message we
usually execute a command like this throughout our test suite:
test_must_fail git command --option 2>output.err
Note that this command doesn't limit the redirection to the git
command, but it redirects the standard error of the 'test_must_fail'
helper function as well. This is wrong for several reasons:
- If that git command were to succeed or die for an unexpected
reason e.g. signal, then 'test_must_fail's helpful error message
would end up in the given file instead of on the terminal or in
't/test-results/$T.out', when the test is run with '-v' or
'--verbose-log', respectively.
- If the test is run with '-x', the trace of executed commands in
'test_must_fail' will go to stderr as well (except for more recent
Bash versions supporting $BASH_XTRACEFD), and then in turn will
end up in the given file.
I have to admit that I'm slightly negative on this approach
Well, now I'm not just slightly negative, see the patch series I will
send out in a minute :)
just
because:
1. It requires every caller to do something special, rather than just
using normal redirection. And the feature isn't as powerful as
redirection. E.g., some callers do:
test_must_fail foo >output 2>&1
Yeah, they do, but most of the time they shouldn't.
I've looked at uses of 'test_must_fail cmd... 2>&1', and also uses of
other test helper functions with stderr duplicated from stdout (and a
couple of cases of "plain" 'git cmd ... 2>&1' as well; that's where the
t6300-for-each-ref fix came from some days ago).
I think all but one of those cases would benefit from handling stdout
and stderr separately, and that one exception shouldn't use
'test_must_fail' for a platform command in the first place[1].
- Many of these tests check error messages after they mingled stderr
and stdout like this:
test_must_fail git cmd >out 2>&1 &&
(test_i18n)grep "relevant part of the error msg" out
In these cases there is no need for 2>&1 as they don't care about
stdout at all. They could just grep the failed command's stderr,
with the added benefit that they would ensure that the error message
indeed goes to stderr.
- A couple of these tests check the output more strictly:
test_must_fail git cmd >actual 2>&1 &&
echo "error message" >expect &&
test_(i18n)cmp expect actual
which also checks that nothing was printed on stdout, but does so
implicitly. Considering all the sloppy uses of 2>&1, it's hard to
tell whether it was deliberate to check the empty stdout, or
accidental, because the writer of the test used 'test_cmp' instead
of 'grep' for its verbosity on failure, or simply mixed up the two.
Therefore, I think it's better when it's written more explicitly:
test_must_fail git cmd >out 2>err &&
echo "exact error message" >expect &&
test_cmp expect err &&
test_must_be_empty out
... both when reading such a test and when looking at it's output on
failure.
- Then there are some cases that, for some reason, completely silence
a git command with:
test_must_fail git cmd >/dev/null 2>&1
The output of a command could turn out to be useful when debugging a
failing tests, so tests shouldn't do this, unless there is a very
good reason (e.g. the command produces a lot of output).
- Finally, a few tests check that something is surely missing from a
git command's output, both from stdout and stderr:
test_must_fail git cmd >out 2>&1 &&
! grep "should not be there" out
Or that a command is completely silent:
test_must_fail git cmd --quiet >out 2>&1 &&
test_must_be_empty out
Checking the emptiness of stdout and stderr separately would give
more info on failure, as it would tell us right away where the
unexpected output is coming from (though arguably the presence of
words like "error" or "fatal" in the output of 'test_must_be_empty'
would be a dead giveaway).
BTW, we already have a couple of careful tests that do:
git cmd >out 2>err &&
test_must_be_empty out &&
test_must_be_empty err
(Which makes me think that extending 'test_must_be_empty' to check
the emptiness of multiple files at once would be worth it: fewer
lines, fewer characters to type, and it could report all non-empty
files, not just the first.)
But:
test_must_fail stderr=output foo >output
is not quite right (stdout and stderr outputs may clobber each
other, because they have independent position pointers).
Relying on the combination of the unbuffered stderr and the
block-buffered stdout could be fragile to begin with, though in general
it doesn't really cause troubles if the data on stdout is less than the
buffer size or if there's no output on stderr after the stdout buffer is
flushed. The stdout buffer size is pagesize on Linux (I don't know
offhand about others), which is large enough compared to the typical
amount of output git commands produce in our test scripts, so we could
get away with it.
2. The "-x" problems aren't specific to test_must_fail at all. They're
a general issue with shell functions.
Sure. I tried to address 'test_must_fail' first, because, with about
90% of the cases, it is by far the worst offender among all test helper
functions with redirected stderr. 'test_expect_code' comes in second
far behind, and there are a mere handful of the combination of
'test_terminal' and 'perl'.
The 'test_expect_code 129 ... 2>&1' in t0012-help.sh's row of '$builtin
can handle -h' tests is particularly interesting, because it hides the
inconsistency that 76 commands send their help/usage text to stdout,
while 45 to stderr.
I'm not entirely happy with saying "if you want to use -x, please use
bash". But given that it actually solves the problems everywhere with no
further effort, is it really that bad a solution?
Well, not that bad a workaround, but in my opinion the real solution is
to make '-x' tracing work reliably with /bin/sh.
Since we strive for POSIX shell compatibility in our test scripts, I
dutifully run the test suite with /bin/sh. Switching shells for '-x' to
get more info about what went wrong (and then switching back) is a
hassle even when it happens to work, and rather annoying when it
doesn't. I've lost count of the various ways of running tests with '-x'
and Bash that I naively thought would Just Work, but didn't because of
the subtleties of GIT-BUILD-OPTIONS and/or re-exec in case of
'--verbose-log'. I find I still reached for various ad-hoc
debugging/tracing or 'test_pause' first, and kept '-x' only as last
resort.
If we start running tests with '-x' and Bash on Travis CI, then it won't
be able to (semi-)automatically catch non-POSIX constructs entering the
test scripts. And I do want to run all tests with '-x', to get the most
information about rarely occurring transient errors.
[1] - 't/lib-git-p4.sh' contains the following command:
test_must_fail kill $pid >/dev/null 2>&1