Skip to content

detecting &&-chain breakage

Jeff King git 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest25 patches 22 repliesIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/25t/test-lib: introduce --chain-lint optiont/README t/test-lib.sh+26 -02 replies
2/25t: fix severe &&-chain breakaget/t1300-repo-config.sh t/t1403-show-ref.sh t/t1700-split-index.sh +11 more+19 -190 replies
3/25t: fix moderate &&-chain breakaget/t0062-revision-walking.sh t/t0201-gettext-fallbacks.sh t/t1300-repo-config.sh +10 more+44 -440 replies
4/25t: fix trivial &&-chain breakaget/annotate-tests.sh t/t0000-basic.sh t/t0011-hashmap.sh +40 more+70 -680 replies
5/25t: assume test_cmp produces verbose outputt/t6012-rev-list-simplify.sh t/t6111-rev-list-treesame.sh+3 -120 replies
6/25t: use verbose instead of hand-rolled errorst/t4022-diff-rewrite.sh t/t4202-log.sh t/t6006-rev-list-format.sh +2 more+10 -450 replies
7/25t: use test_must_fail instead of hand-rolled blockst/t4124-apply-ws-rule.sh t/t6036-recursive-corner-cases.sh t/t9300-fast-import.sh+5 -140 replies
8/25t: fix &&-chaining issues around setup which might failt/t5503-tagfollow.sh t/t6032-merge-large-rename.sh t/t7201-co.sh +2 more+11 -80 replies
9/25t: use test_might_fail for diff and grept/t1002-read-tree-m-u-2way.sh t/t5550-http-fetch-dumb.sh+6 -60 replies
10/25t: use test_expect_code instead of hand-rolled comparisont/t0040-parse-options.sh t/t4035-diff-quiet.sh t/t4053-diff-no-index.sh+38 -440 replies
11/25t: wrap complicated expect_code users in a blockt/t0005-signals.sh t/t4026-color.sh t/t5004-archive-corner-cases.sh +1 more+13 -90 replies
12/25t: avoid using ":" for commentst/t4104-apply-boundary.sh t/t5533-push-cas.sh+10 -100 replies
13/25t3600: fix &&-chain breakage for setup commandst/t3600-rm.sh+18 -180 replies
14/25t7201: fix &&-chain breakaget/t7201-co.sh+6 -130 replies
15/25t9502: fix &&-chain breakaget/t9502-gitweb-standalone-parse-output.sh+6 -42 replies
16/25t6030: use modern test_* helperst/t6030-bisect-porcelain.sh+31 -600 replies
17/25t0020: use modern test_* helperst/t0020-crlf.sh+28 -11615 replies
18/25t1301: use modern test_* helperst/t1301-shared-repo.sh+7 -132 replies
19/25t6034: use modern test_* helperst/t6034-merge-rename-nocruft.sh+13 -531 reply
20/25t4117: use modern test_* helperst/t4117-apply-reject.sh+10 -560 replies
21/25t9001: use test_when_finishedt/t9001-send-email.sh+10 -202 replies
22/25t0050: appease --chain-lintt/t0050-filesystem.sh+8 -40 replies
23/25t7004: fix embedded single-quotest/t7004-tag.sh+1 -10 replies
24/25t0005: fix broken &&-chainst/t0005-signals.sh+2 -20 replies
25/25t4104: drop hand-rolled error reportingt/t4104-apply-boundary.sh+2 -80 replies

Reviews trailers per patch

1 of 25 patches has a Reviewed-by or Acked-by.

PatchJohan Herland
1/25
2/25
3/25
4/25
5/25
6/25
7/25
8/25
9/25
10/25
11/25
12/25
13/25
14/25
15/25
16/25
17/25A-b
18/25
19/25
20/25
21/25
22/25
23/25
24/25
25/25

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v1 thread

Cover letter [PATCH 0/25] detecting &&-chain breakage

This is a cleanup of the &&-chain lint I posted earlier:

http://thread.gmane.org/gmane.comp.version-control.git/265613/focus=265859

I don't know who came up with the idea for it originally, but the concept certainly was floating in the back of my mind from Jonathan's earlier version that is referenced in that thread.

The general idea is to detect &&-chain breakage that can lead to our tests yielding false success. The first patch implements and discusses the lint-check itself, which is quite simple. The bulk of the work was fixing all of the issues in the existing tests. :)

That didn't all need to happen immediately. I mainly wanted to start on it to answer two questions:

1. Are &&-chain breakages actually preventing us from seeing any test failures? Or is it mostly just pedantry, and we miss out only on knowing whether "cat >expect <<-\EOF" failed (which presumably it never does).

2. How bad are the false positives? Both how common, and how bad to work around.

But after a few hours, I reached a zen state and just kept going. So at the end of this series, the whole test suite is --chain-lint clean (modulo any tests that are skipped on my platform). We could even switch the checks on by default at the end of the series, but I didn't do that here. I think it would be sane to run them all the time, though; in the normal success case, they don't add any forks (the shell just runs "(exit) && ...", and realizes that the whole thing is one big &&-chain). I couldn't measure any time difference running the suite with and without it.

Anyway, to answer the questions: Yes, there were definitely tests whose values were being thrown away, and we would not have noticed if they failed. The good news is that all of them did pass once we started checking their results. Hooray.

There were a number of false positives, though as a percentage of the test suite, probably not many (it's just that we have quite a lot of tests). Most of them were in rather old tests, and IMHO the fixes I did actually improved the readability of the result. So overall I think this is a very positive change; I doubt it will get in people's way very often, and I look forward to having one less thing to worry about handling manually in review. The biggest downside is that I may have automated Eric Sunshine out of a job. :)

The patches are:

[01/25]: t/test-lib: introduce --chain-lint option [02/25]: t: fix severe &&-chain breakage
[03/25]: t: fix moderate &&-chain breakage
[04/25]: t: fix trivial &&-chain breakage
[05/25]: t: assume test_cmp produces verbose output [06/25]: t: use verbose instead of hand-rolled errors [07/25]: t: use test_must_fail instead of hand-rolled blocks [08/25]: t: fix &&-chaining issues around setup which might fail [09/25]: t: use test_might_fail for diff and grep [10/25]: t: use test_expect_code instead of hand-rolled comparison [11/25]: t: wrap complicated expect_code users in a block [12/25]: t: avoid using ":" for comments
[13/25]: t3600: fix &&-chain breakage for setup commands [14/25]: t7201: fix &&-chain breakage
[15/25]: t9502: fix &&-chain breakage
[16/25]: t6030: use modern test_* helpers
[17/25]: t0020: use modern test_* helpers
[18/25]: t1301: use modern test_* helpers
[19/25]: t6034: use modern test_* helpers
[20/25]: t4117: use modern test_* helpers
[21/25]: t9001: use test_when_finished
[22/25]: t0050: appease --chain-lint
[23/25]: t7004: fix embedded single-quotes
[24/25]: t0005: fix broken &&-chains
[25/25]: t4104: drop hand-rolled error reporting

It's a lot of patches, and a few of them are long. I tried to group them by functionality rather than file (though as you can see, some of the tests were unique enough snowflakes that it made sense to discuss their issues separately). I'm hoping it should be an easy review, if not a short one.

I'll spare you the full per-file diffstat, but the total is a very satisfying:

   84 files changed, 397 insertions(+), 647 deletions(-)

That's a lot of changes in a lot of hunks, but most of them are in files that haven't been touched in a while. The series merges cleanly with "pu", so I don't think I've stepped on anyone's topics in flight.

-Peff