Re: [PATCH 6/6] t/t5528-push-default: test pushdefault workflows

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2013, 22:17 UTC
Message-ID: <7v8v25ycd4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1371640304-26019-7-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 34 quoted lines

> Introduce test_pushdefault_workflows(), and test that all push.default
> modes work with central and triangular workflows as expected.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5528-push-default.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index b599186..eabc09d 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -39,6 +39,26 @@ test_push_failure () {
>  	test_cmp expect actual
>  }
>  
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow
> +test_pushdefault_workflow () {
> +	workflow=central
> +	pushdefault=parent1
> +	if test -n "${4-}"; then
> +		workflow=triangular
> +		pushdefault=parent2
> +	fi
> +test_expect_success "push.default = $2 $1 in $workflow workflows" "
> +	test_config branch.master.remote parent1 &&
> +	test_config branch.master.merge refs/heads/foo &&
> +	test_config remote.pushdefault $pushdefault &&
> +	test_commit commit-for-$2${4+-triangular} &&
> +	test_push_$1 $2 $3 ${4+repo2}
> +"

Please indent the above; it is hard to spot where the actual test begins and ends otherwise.

Show 14 quoted lines

> +}
> +
>  test_expect_success '"upstream" pushes to configured upstream' '
>  	git checkout master &&
>  	test_config branch.master.remote parent1 &&
> @@ -115,4 +135,20 @@ test_expect_success 'push to existing branch, upstream configured with different
>  	test_cmp expect-other-name actual-other-name
>  '
>  
> +## test_pushdefault_workflow() arguments:
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow

What happens in current/upstream/simple modes is affected by the current branch, but it is unclear on what branch do these all run at the first glance. It would be helpful to add a comment here that says what the starting condition is, something like:

    # We are on 'master', which integrates with 'foo' from parent1
    # Both parent1 and parent2 repositories have 'master' and 'foo'
    # branches.

here. It took me a while to tell what some of these comments wanted to say without such a comment.

> +test_pushdefault_workflow success current master  # breaks push/pull symmetry

On 'master', push.default = current and we are in central workflow. Because we do not use upstream but current, that deliberately breaks the symmetry. We make sure that 'master' is updated.

Looks correct.

> +test_pushdefault_workflow success upstream foo    # preserves push/pull symmetry

We use 'upstream'; current branch is set to integrate with 'foo' and that is what is updated. Looks good.

> +test_pushdefault_workflow failure simple master   # errors out on asymmetry

Simple is a safer form of 'current' in the central workflow. The current branch is set to integrate with 'foo', which is different name from the current branch'es name 'master', and the push fails.

Looks correct, but do we want to make sure 'foo' is not updated here as well?

> +test_pushdefault_workflow success matching master # always works

This also should update 'foo'; do we want to make sure that happens too?

Otherwise we won't be able to tell between matching and current.

> +test_pushdefault_workflow success current master triangular  # always works

OK.

> +test_pushdefault_workflow failure upstream foo triangular    # always errors out

OK.

> +test_pushdefault_workflow success simple master triangular   # works like current

OK, because in triangular it is like current.

> +test_pushdefault_workflow success matching master triangular # always works

OK.

As this step adds a useful helper function for testing, it appears to me that the helper can and should check the postcondition more carefully, e.g. not just making sure what should be updated is updated, but what should not be updated is not touched.

Other than that, looks fine to me.

> +
>  test_done