git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 0/25] detecting &&-chain breakage
Date: Fri, 20 Mar 2015 06:04:30 -0400	[thread overview]
Message-ID: <20150320100429.GA17354@peff.net> (raw)

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

             reply	other threads:[~2015-03-20 10:04 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-20 10:04 Jeff King [this message]
2015-03-20 10:05 ` [PATCH 01/25] t/test-lib: introduce --chain-lint option Jeff King
2015-03-25  2:53   ` SZEDER Gábor
2015-03-25  3:05     ` Jeff King
2015-03-20 10:06 ` [PATCH 02/25] t: fix severe &&-chain breakage Jeff King
2015-03-20 10:06 ` [PATCH 03/25] t: fix moderate " Jeff King
2015-03-20 10:07 ` [PATCH 04/25] t: fix trivial " Jeff King
2015-03-20 10:07 ` [PATCH 05/25] t: assume test_cmp produces verbose output Jeff King
2015-03-20 10:09 ` [PATCH 06/25] t: use verbose instead of hand-rolled errors Jeff King
2015-03-20 10:09 ` [PATCH 07/25] t: use test_must_fail instead of hand-rolled blocks Jeff King
2015-03-20 10:10 ` [PATCH 08/25] t: fix &&-chaining issues around setup which might fail Jeff King
2015-03-20 10:11 ` [PATCH 09/25] t: use test_might_fail for diff and grep Jeff King
2015-03-20 10:11 ` [PATCH 10/25] t: use test_expect_code instead of hand-rolled comparison Jeff King
2015-03-20 10:12 ` [PATCH 11/25] t: wrap complicated expect_code users in a block Jeff King
2015-03-20 10:12 ` [PATCH 12/25] t: avoid using ":" for comments Jeff King
2015-03-20 10:12 ` [PATCH 13/25] t3600: fix &&-chain breakage for setup commands Jeff King
2015-03-20 10:12 ` [PATCH 14/25] t7201: fix &&-chain breakage Jeff King
2015-03-20 10:13 ` [PATCH 15/25] t9502: " Jeff King
2015-03-20 17:48   ` Johannes Sixt
2015-03-20 18:03     ` Jeff King
2015-03-20 10:13 ` [PATCH 16/25] t6030: use modern test_* helpers Jeff King
2015-03-20 10:13 ` [PATCH 17/25] t0020: " Jeff King
2015-03-25  0:23   ` SZEDER Gábor
2015-03-25  2:56     ` Jeff King
2015-03-25  5:24       ` [PATCH 0/8] more &&-chaining test fixups Jeff King
2015-03-25  5:25         ` [PATCH 1/8] perf-lib: fix ignored exit code inside loop Jeff King
2015-03-25  5:28         ` [PATCH 2/8] t0020: fix ignored exit code inside loops Jeff King
2015-03-25  5:28         ` [PATCH 3/8] t3305: fix ignored exit code inside loop Jeff King
2015-03-25  8:40           ` Johan Herland
2015-03-25  5:29         ` [PATCH 4/8] t7701: " Jeff King
2015-03-25  5:29         ` [PATCH 5/8] t: fix some trivial cases of ignored exit codes in loops Jeff King
2015-03-25  5:30         ` [PATCH 6/8] t: simplify loop exit-code status variables Jeff King
2015-03-25 17:27           ` Junio C Hamano
2015-03-25 17:43             ` Jeff King
2015-03-25  5:31         ` [PATCH 7/8] t0020: use test_* helpers instead of hand-rolled messages Jeff King
2015-03-25  5:32         ` [PATCH 8/8] t9001: drop save_confirm helper Jeff King
2015-03-25 17:29         ` [PATCH 0/8] more &&-chaining test fixups Junio C Hamano
2015-03-20 10:13 ` [PATCH 18/25] t1301: use modern test_* helpers Jeff King
2015-03-24 23:51   ` SZEDER Gábor
2015-03-25  2:45     ` Jeff King
2015-03-20 10:13 ` [PATCH 19/25] t6034: " Jeff King
2015-03-24 23:43   ` SZEDER Gábor
2015-03-20 10:13 ` [PATCH 20/25] t4117: " Jeff King
2015-03-20 10:13 ` [PATCH 21/25] t9001: use test_when_finished Jeff King
2015-03-25  2:00   ` SZEDER Gábor
2015-03-25  2:47     ` Jeff King
2015-03-20 10:13 ` [PATCH 22/25] t0050: appease --chain-lint Jeff King
2015-03-20 10:13 ` [PATCH 23/25] t7004: fix embedded single-quotes Jeff King
2015-03-20 10:13 ` [PATCH 24/25] t0005: fix broken &&-chains Jeff King
2015-03-20 10:13 ` [PATCH 25/25] t4104: drop hand-rolled error reporting Jeff King
2015-03-20 10:23 ` [PATCH 0/25] detecting &&-chain breakage Jeff King
2015-03-20 14:28 ` Michael J Gruber
2015-03-20 14:32   ` [PATCH 26/27] t/*svn*: fix moderate " Michael J Gruber
2015-03-20 14:32     ` [PATCH 27/27] t9104: fix test for following larger parents Michael J Gruber
2015-03-20 18:04     ` [PATCH 26/27] t/*svn*: fix moderate &&-chain breakage Junio C Hamano
2015-03-20 19:35       ` Junio C Hamano
2015-03-20 20:02         ` Jeff King
2015-03-20 20:13           ` Jeff King
2015-03-23  9:36             ` Michael J Gruber
2015-03-20 17:57   ` [PATCH 0/25] detecting " Jeff King
2015-03-20 17:44 ` Junio C Hamano
2015-03-20 18:00   ` Junio C Hamano
2015-03-20 18:04     ` Jeff King
2015-03-20 18:33       ` Junio C Hamano
2015-03-20 23:18 ` Eric Sunshine
2015-03-21  8:19   ` Jeff King
2015-03-21 18:01     ` Junio C Hamano
2015-03-21 22:23       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150320100429.GA17354@peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).