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

From: Martin Ågren <martin.agren@gmail.com>
Date: Jul 29, 2019, 20:04 UTC
Message-ID: <CAN0heSoU7XUN8GOFzzBrLG0RbY65hhYUn_-_Ax1DfFsNZ64hpA@mail.gmail.com>
In-Reply-To: <69dc3696e715f9be9e545e0142244e16bdd489cc.1563490164.git.steadmon@google.com>

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:

Show 7 quoted lines

> +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...

Show 7 quoted lines

> +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.

Martin