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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 18, 2015 at 12:41 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> 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.

> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]