Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook

[Josh Steadmon <steadmon@google.com>]

On 2019.07.29 22:04, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> > +test_expect_success '--no-verify with succeeding hook (merge)' '
> > +
> > +       git checkout side &&
> > +       git merge --no-verify -m "merge master" master &&
> > +       git checkout master
> > +
> > +'
> 
> This test doesn't even try to verify that the hook was actually ignored.
> That left me puzzled for a while...
> 
> > +test_expect_success '--no-verify with failing hook (merge)' '
> > +
> > +       git checkout side &&
> > +       git merge --no-verify -m "merge master" master &&
> > +       git checkout master
> > +
> > +'
> 
> ... but this would then (most likely) fail, so we would notice
> something's wrong.
> 
> This script seems to me like if it passes 100%, we can be fairly sure
> we're ok, but if some individual test fails, we shouldn't be surprised
> if its oneline description is a bit off compared to the bug. Similarly,
> quite a few tests could pass, despite their oneline description inducing
> the thought of "but surely, if /that/ were the problem, /those/ tests
> would fail".
> 
> Anyway, I realize this is just following the existing approach. I guess
> you could argue it has served us well for a long time.
> 
> I would probably prefer seeing the various hunks in this patch being
> squashed into the relevant commits (1/4 vs 3/4) to make those patches
> more self-describing.

Will squash these as you said in V3. Will also think about whether
another test approach would make more sense here.