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

Commit Message

Josh Steadmon July 18, 2019, 10:57 p.m. UTC

From: Michael J Gruber <[e-mail]>

Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".

[js: renamed test as suggested in review comments]

Signed-off-by: Michael J Gruber <[e-mail]>
Signed-off-by: Josh Steadmon <[e-mail]>
---
 ...> t7503-pre-commit-and-pre-merge-hooks.sh} | 66 ++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-hooks.sh} (67%)

Comments

Martin Ågren July 29, 2019, 8:04 p.m. UTC | #1

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <[e-mail]> 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.


Martin

Josh Steadmon July 29, 2019, 11:43 p.m. UTC | #2

On 2019.07.29 22:04, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <[e-mail]> 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.

Martin Ågren July 30, 2019, 7:13 a.m. UTC | #3

On Tue, 30 Jul 2019 at 01:43, Josh Steadmon <[e-mail]> wrote:
>
> On 2019.07.29 22:04, Martin Ågren wrote:
> > This script seems to me like if it passes 100%, we can be fairly sure
> > we're ok, but [...]

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

Thinking a bit more about this, this test uses two identical hooks, runs
some commands and verifies that "the" hook was run (or not, with
--no-verify). If the implementation started calling the wrong hook
(pre-commit / pre-merge) or both hooks, we wouldn't notice.

Please forgive my braindump below, I'm on the run so I'm just throwing
this out there:

Perhaps (first do some modernizing of this script, to protect various
setup steps, use "write_script", etc, then) make the existing hook a
tiny bit pre-commit-specific, e.g., by doing something like "echo
pre-commit >>executed-hooks", then at select places check "test_cmp
executed-hooks pre-commit" (against "echo pre-commit >pre-commit"),
"test_path_is_missing executed-hooks", and so on, coupled with some
"test_when_finished 'rm -f executed_hooks'". Then the tests added for
this series would use a very similar hook, appending and checking for
"pre-merge[-commit]", That should make us fairly certain that we're
running precisely the wanted hook, I think.

Martin

Josh Steadmon July 31, 2019, 6:34 p.m. UTC | #4

On 2019.07.30 09:13, Martin Ågren wrote:
> On Tue, 30 Jul 2019 at 01:43, Josh Steadmon <[e-mail]> wrote:
> >
> > On 2019.07.29 22:04, Martin Ågren wrote:
> > > This script seems to me like if it passes 100%, we can be fairly sure
> > > we're ok, but [...]
> 
> > Will squash these as you said in V3. Will also think about whether
> > another test approach would make more sense here.
> 
> Thinking a bit more about this, this test uses two identical hooks, runs
> some commands and verifies that "the" hook was run (or not, with
> --no-verify). If the implementation started calling the wrong hook
> (pre-commit / pre-merge) or both hooks, we wouldn't notice.
> 
> Please forgive my braindump below, I'm on the run so I'm just throwing
> this out there:
> 
> Perhaps (first do some modernizing of this script, to protect various
> setup steps, use "write_script", etc, then) make the existing hook a
> tiny bit pre-commit-specific, e.g., by doing something like "echo
> pre-commit >>executed-hooks", then at select places check "test_cmp
> executed-hooks pre-commit" (against "echo pre-commit >pre-commit"),
> "test_path_is_missing executed-hooks", and so on, coupled with some
> "test_when_finished 'rm -f executed_hooks'". Then the tests added for
> this series would use a very similar hook, appending and checking for
> "pre-merge[-commit]", That should make us fairly certain that we're
> running precisely the wanted hook, I think.
> 
> Martin

That sounds like a reasonable approach, thank you for the suggestions. I
will work on this for V3.

Patch

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-hooks.sh
similarity index 67%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-hooks.sh
index 984889b39d..36ae87f7ef 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-hooks.sh
@@ -1,9 +1,22 @@ 
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge hooks'
 
 . ./test-lib.sh
 
+test_expect_success 'root commit' '
+
+	echo "root" > file &&
+	git add file &&
+	git commit -m "zeroth" &&
+	git checkout -b side &&
+	echo "foo" > foo &&
+	git add foo &&
+	git commit -m "make it non-ff" &&
+	git checkout master
+
+'
+
 test_expect_success 'with no hook' '
 
 	echo "foo" > file &&
@@ -12,6 +25,14 @@  test_expect_success 'with no hook' '
 
 '
 
+test_expect_success 'with no hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with no hook' '
 
 	echo "bar" > file &&
@@ -20,15 +41,25 @@  test_expect_success '--no-verify with no hook' '
 
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now install hook that always succeeds
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-commit"
+MERGEHOOK="$HOOKDIR/pre-merge"
 mkdir -p "$HOOKDIR"
 cat > "$HOOK" <<EOF
 #!/bin/sh
 exit 0
 EOF
 chmod +x "$HOOK"
+cp -p "$HOOK" "$MERGEHOOK"
 
 test_expect_success 'with succeeding hook' '
 
@@ -38,6 +69,14 @@  test_expect_success 'with succeeding hook' '
 
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 
 	echo "even more" >> file &&
@@ -46,11 +85,20 @@  test_expect_success '--no-verify with succeeding hook' '
 
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now a hook that fails
 cat > "$HOOK" <<EOF
 #!/bin/sh
 exit 1
 EOF
+cp -p "$HOOK" "$MERGEHOOK"
 
 test_expect_success 'with failing hook' '
 
@@ -68,6 +116,22 @@  test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success 'with failing hook (merge)' '
+
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master
+
+'
+
+test_expect_success '--no-verify with failing hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '