Re: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Fri, Mar 20, 2015 at 1:10 AM, Jeff King p...@peff.net wrote:
On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
Clever (Jonathan's too); much nicer than trying to special case only here-doc.
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the -chaining).
I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.
Thanks for working on this. It looks like this technique should be a
valuable addition to test-lint. (I had intended, but haven't yet found
time to dig into it, so I'm happy to hear of your progress.)
--
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: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the -chaining).
I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.
-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
test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 09:37:12PM -0400, Eric Sunshine wrote:
Thanks. I notice that a large number of broken -chains are on
here-docs. I really wish you could put the on the EOF line at the
end of the here-doc. I understand _why_ that this not the case, but
mentally it is where I want to type it, and I obviously sometimes fail
to go back and fix it. I don't think there's a better solution in POSIX
sh, though.
I wonder if test-lint could be enhanced to detect this sort of problem?
That would be nice, but it's complicated. A naive:
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..3a6d8d8 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -22,6 +22,7 @@ while () {
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
/\btest\s+[^=]*==/ and err 'test a == b is not portable (please use
=)';
/\bexport\s+[A-Za-z0-9_]*=/ and err 'export FOO=bar is not portable
(please use FOO=bar export FOO)';
+ / -?.?EOF(.*)/ $1 !~ // and err 'here-doc with broken -chain';
# this resets our $. for each file
close ARGV if eof;
}
yields quite a few false positives, because of course we don't know
which are meant to be at the end of the chain and which are not. And
finding that out is tough. We'd have to actually parse to the end of
the here-doc ourselves, then see if it was the end of the test_expect
block.
I think it would be simpler to ask the shell to check this for us, like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
I seem to recall people looked at doing this sort of lint a while ago,
but we never ended up committing anything. I wonder if it was because of
all of these false positives.
diff --git a/t/t3010-ls-files-killed-modified.sh
b/t/t3010-ls-files-killed-modified.sh
index 6d3b828..62fce10 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -62,7 +62,7 @@ test_expect_success 'git update-index --add to add various
paths.' '
cd submod$i git commit --allow-empty -m empty $i
) || break
done
- git update-index --add submod[12]
+ git update-index --add submod[12]
(
cd submod1
git commit --allow-empty -m empty 1 (updated)
@@ -99,12 +99,12 @@ test_expect_success 'git ls-files -k to show killed files.'
'
'
test_expect_success 'git ls-files -k output (w/o icase)' '
- git ls-files -k .output
+ git ls-files -k .output
test_cmp .expected .output
'
test_expect_success 'git ls-files -k output (w/ icase)' '
- git -c core.ignorecase=true ls-files -k .output
+ git -c core.ignorecase=true ls-files -k .output
test_cmp .expected .output
'
-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: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
[+cc Jonathan, whose patch I apparently subconsciously copied]
On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
I seem to recall people looked at doing this sort of lint a while ago,
but we never ended up committing anything. I wonder if it was because of
all of these false positives.
This turns out to be rather annoying to grep for in the list archives,
but I found at least one discussion:
http://article.gmane.org/gmane.comp.version-control.git/235913
I don't know why we didn't follow it up then. Perhaps because the patch
there (which is rather similar to what I have above) was not
conditional, so whole chunks of the test suite needed fixing. There are
enough problems that we would probably want to do this conditionally,
fix them over time, and then finally flip the feature on by default.
-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
