Re: [PATCH 01/25] t/test-lib: introduce --chain-lint option
On Wed, Mar 25, 2015 at 03:53:52AM +0100, SZEDER Gábor wrote: cmd1 for i in a b c; do cmd2 $i done cmd3 which will not notice failures of cmd2 a or cmd b s/cmd b/cmd2 b/ ? Yes, but the patches are already in next, so it is sadly too late for commit message fixups. - it cannot find a missing -chain inside a block or subfunction, like: [...] And inside subshells. [...] Yeah, I had mentally filed them with block, but true subshells are probably the most common place. However, I'd suspect a good portion of them are going to be the trivial type, especially if they involve setting up the sub-environment at the top of the subshell. E.g., something like this: cmd1 ( FOO=bar; export FOO cmd2 ) cmd3 does not break the outer chain (which is what --chain-lint checks). It does break the chain inside the subshell, but we never expect variable assignment or export to fail (it is nice to fix it, of course, but the main purpose in fixing the ones in my trivial patch was more about shutting up --chain-lint to make the real breakages more obvious). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/25] t/test-lib: introduce --chain-lint option
Quoting Jeff King p...@peff.net:
However, there are a number of places it cannot reach:
- it cannot find a failure to break out of loops on error,
like:
cmd1
for i in a b c; do
cmd2 $i
done
cmd3
which will not notice failures of cmd2 a or cmd b
s/cmd b/cmd2 b/ ?
- it cannot find a missing -chain inside a block or
subfunction, like:
foo () {
cmd1
cmd2
}
foo
bar
which will not notice a failure of cmd1.
And inside subshells. I think it's worth mentioning, too, because
subshells are used frequently when setting environment variables
( GIT_FOO=bar export GIT_FOO cmd1 ... ) test_cmp
or changing directory
( cd subdir cmd1 ... ) test_cmp
I was wondering whether we could do better here with helper functions,
something along the lines of:
# Set and export environment variable, automatically unsetting it after
# the test is over
test_setenv () {
eval $1=\$2
export $1
# sane_unset, to allow unsetting during the test
test_when_finished sane_unset '$1'
}
# Change to given directory, automatically return to current working
# directory after the test is over
test_cd () {
test_when_finished cd '$PWD'
cd $1
}
With these the above examples would become
test_setenv GIT_FOO bar cmd1 ... test_cmp
and
test_cd subdir cmd1 ... test_cmp
which means increased coverage for --chain-lint.
Thanks for working on this. I looked into this after seeing Jonathan's
patch back then, got quite far but never reached a chain-lint-clean
state, and only sent patches for the two most amusing breakages
(ddeaf7ef0d, 056f34bbcd).
I'm glad it's off my TODO list and I don't have to rebase a 1.5 year old
branch to current master :)
Gábor
- it only checks tests that you run; every platform will
have some tests skipped due to missing prequisites,
so it's impossible to say from one run that the test
suite is free of broken -chains. However, all tests get
run by _somebody_, so eventually we will notice problems.
- it does not operate on test_when_finished or prerequisite
blocks. It could, but these tends to be much shorter and
less of a problem, so I punted on them in this patch.
This patch was inspired by an earlier patch by Jonathan
Nieder:
http://article.gmane.org/gmane.comp.version-control.git/235913
This implementation and all bugs are mine.
Signed-off-by: Jeff King p...@peff.net
---
t/README | 10 ++
t/test-lib.sh | 16
2 files changed, 26 insertions(+)
diff --git a/t/README b/t/README
index d5bb0c9..35438bc 100644
--- a/t/README
+++ b/t/README
@@ -168,6 +168,16 @@ appropriately before running make.
Using this option with a RAM-based filesystem (such as tmpfs)
can massively speed up the test suite.
+--chain-lint::
+--no-chain-lint::
+ If --chain-lint is enabled, the test harness will check each
+ test to make sure that it properly -chains all commands (so
+ that a failure in the middle does not go unnoticed by the final
+ exit code of the test). This check is performed in addition to
+ running the tests themselves. You may also enable or disable
+ this feature by setting the GIT_TEST_CHAIN_LINT environment
+ variable to 1 or 0, respectively.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..50b3d3f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -232,6 +232,12 @@ do
--root=*)
root=$(expr z$1 : 'z[^=]*=\(.*\)')
shift ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1
+ shift ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0
+ shift ;;
-x)
trace=t
verbose=t
@@ -524,6 +530,16 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test ${GIT_TEST_CHAIN_LINT:-0} != 0; then
+ # 117 is magic because it is unlikely to match the exit
+ # code of other programs
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ error bug in the test script: broken -chain: $1
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
--
2.3.3.520.g3cfbb5d
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/25] t/test-lib: introduce --chain-lint option
It's easy to miss an -chain in a test script, like:
test_expect_success 'check something important' '
cmd1
cmd2
cmd3
'
The test harness will notice if cmd3 fails, but a failure of
cmd1 or cmd2 will go unnoticed, as their exit status is lost
after cmd3 runs.
The toy example above is easy to spot because the cmds are
all the same length, but real code is much more complicated.
It's also difficult to detect these situations by statically
analyzing the shell code with regexps (like the
check-non-portable-shell script does); there's too much
context required to know whether a -chain is appropriate
on a given line or not.
This patch instead lets the shell check each test by
sticking a command with a specific and unusual return code
at the top of each test, like:
(exit 117)
cmd1
cmd2
cmd3
In a well-formed test, the non-zero exit from the first
command prevents any of the rest from being run, and the
test's exit code is 117. In a bad test (like the one above),
the 117 is lost, and cmd3 is run.
When we encounter a failure of this check, we abort the test
script entirely. For one thing, we have no clue which subset
of the commands in the test snippet were actually run.
Running further tests would be pointless, because we're now
in an unknown state. And two, this is not a test failure
in the traditional sense. The test script is buggy, not the
code it is testing. We should be able to fix these problems
in the script once, and not have them come back later as a
regression in git's code.
After checking a test snippet for --chain-lint, we do still
run the test itself. We could actually have a pure-lint
mode which just checks each test, but there are a few
reasons not to. One, because the tests are executing
arbitrary code, which could impact the later environment
(e.g., that could impact which set of tests we run at all).
And two, because a pure-lint mode would still be expensive
to run, because a significant amount of code runs outside of
the test_expect_* blocks. Instead, this option is designed
to be used as part of a normal test suite run, where it adds
very little overhead.
Turning on this option detects quite a few problems in
existing tests, which will be fixed in subsequent patches.
However, there are a number of places it cannot reach:
- it cannot find a failure to break out of loops on error,
like:
cmd1
for i in a b c; do
cmd2 $i
done
cmd3
which will not notice failures of cmd2 a or cmd b
- it cannot find a missing -chain inside a block or
subfunction, like:
foo () {
cmd1
cmd2
}
foo
bar
which will not notice a failure of cmd1.
- it only checks tests that you run; every platform will
have some tests skipped due to missing prequisites,
so it's impossible to say from one run that the test
suite is free of broken -chains. However, all tests get
run by _somebody_, so eventually we will notice problems.
- it does not operate on test_when_finished or prerequisite
blocks. It could, but these tends to be much shorter and
less of a problem, so I punted on them in this patch.
This patch was inspired by an earlier patch by Jonathan
Nieder:
http://article.gmane.org/gmane.comp.version-control.git/235913
This implementation and all bugs are mine.
Signed-off-by: Jeff King p...@peff.net
---
t/README | 10 ++
t/test-lib.sh | 16
2 files changed, 26 insertions(+)
diff --git a/t/README b/t/README
index d5bb0c9..35438bc 100644
--- a/t/README
+++ b/t/README
@@ -168,6 +168,16 @@ appropriately before running make.
Using this option with a RAM-based filesystem (such as tmpfs)
can massively speed up the test suite.
+--chain-lint::
+--no-chain-lint::
+ If --chain-lint is enabled, the test harness will check each
+ test to make sure that it properly -chains all commands (so
+ that a failure in the middle does not go unnoticed by the final
+ exit code of the test). This check is performed in addition to
+ running the tests themselves. You may also enable or disable
+ this feature by setting the GIT_TEST_CHAIN_LINT environment
+ variable to 1 or 0, respectively.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..50b3d3f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -232,6 +232,12 @@ do
--root=*)
root=$(expr z$1 : 'z[^=]*=\(.*\)')
shift ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1
+ shift ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0
+ shift ;;
-x)
trace=t
verbose=t
@@ -524,6 +530,16 @@ test_eval_ () {
test_run_ ()
