Re: [PATCH v2 1/3] Add tests for describe with --work-tree

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2019, 16:47 UTC
Message-ID: <xmqq5zu65bz5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190128100659.GA6257@ash>

Duy Nguyen <pclouds@gmail.com> writes:

>> My function is a modified version of check_describe().
>
> Whoa. That function is 12 years old! I think our style has evolved a
> bit since then.

;-).

Show 8 quoted lines

> I mean chaining within a test. This is to make sure any failure
> triggers the test failure (as it should, if some command is expected
> to fail, we have other ways to catch it).
>
> I would start with something simple, not using shell function at
> all. Something like this as an example (I added run_describe() because
> that "git" command becomes too long). Have a look at the "do's and
> don'ts" in t/README too.

Thanks for guiding new contributors with an easy to understand example.

> BTW, careful about _success or _failure. You need to make sure bisect
> is not broken. If you add a test to confirm a broken case then it
> should be test_expect_failure (and the test suite will pass). Then
> when you fix it you can flip it to test_expect_success.

And if the fix is simple enough (i.e. a good rule of thumb is if the fixes themselves without tests need to be multi-patch series, it is not simple enough), have a single patch that has both fix and test that expects success, instead of splitting them into two to make a two patch series whose first step expects a failure and whose second step fixes and flips failure to success.