Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2015, 21:15 UTC
Message-ID: <CAPig+cTaQAnWoz7=v9tOPZoeE3LTV+H6KD_tAzmOXi3Tjay_WA@mail.gmail.com>
In-Reply-To: <vpq3852dzry.fsf@anie.imag.fr>

On Wed, Mar 18, 2015 at 12:41 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> Similarly to the "merge doc and code", I personally prefer seeing code
> and tests in the same patch.

In this case, the patch introducing the tests is already quite long and intricate, almost to the point of being a burden to review. Combining the patches would likely push it over the edge. I'd elect to keep them separate.

Show 10 quoted lines

> Actually, my preference goes for a first patch that introduces the tests
> with test_expect_failure for things that are not yet implemented (and
> you can check that tests do not pass yet before you code), and then the
> patch introducing the feature doing
>
> -test_expect_failure
> +test_expect_success
>
> which documents quite clearly and concisely that you just made the
> feature work.

I also tend to favor adding "failure" tests which are flipped to "success" when appropriate, however, as this is an entirely new feature, this approach may be unsuitable (and perhaps overkill). Generally speaking, these new tests don't really make sense without the feature; and, more specifically, due to their nature, several of them will pass even without the feature implemented. As such, the current patch organization seems reasonable.