Re: [PATCH 2/6] t7408: merge short tests, factor out testing method

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2016, 20:45 UTC
Message-ID: <xmqqeg63vtar.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160804195159.7788-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

> Tests consisting of one line each can be consolidated to have fewer tests
> to run as well as fewer lines of code.
>
> When having just a few git commands, do not create a new shell but
> use the -C flag in Git to execute in the correct directory.

Good motivations.

Show 12 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index afcc629..1416cbd 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -10,6 +10,16 @@ base_dir=$(pwd)
>  
>  U=$base_dir/UPLOAD_LOG

Is this line needed anywhere?

We (perhaps unfortunately) still need $base_dir because we want to give an absolute file:/// URL to "submodule add", but I do not think we use $U, so let's get rid of it.

> +test_alternate_usage()
> +{

According to Documentation/CodingGuidelines, this should be:

    test_alternate_usage () {

Somehow the helper name sounds as if it is testing if an alternate is used correctly (i.e. the machinery may attempt to use alternate but not in a correct way), not testing if an alternate is correctly used (i.e. the machinery incorrectly forgets to use an alternate at all), but maybe it is just me.

> +	alternates_file=$1
> +	working_dir=$2

These are good (they can be on a single line), but you would want &&-chain just as other lines.

> +	test_line_count = 1 $alternates_file &&

This needs to quote "$alternates_file" especially in a helper function you have no control over future callers of.

I wonder if we want to check the actual contents of the alternate; it may force us to worry about the infamous "should we expect $(pwd)/something or $PWD/something" if we did so, so it is not a strong suggestion.

> +	echo "0 objects, 0 kilobytes" >expect &&
> +	git -C $working_dir count-objects >current &&
> +	diff expect current

It is more customary to name two "expect" vs "actual", and compare them using "test_cmp" not "diff".

Show 30 quoted lines

> +}
> +
>  test_expect_success 'preparing first repository' '
>  	test_create_repo A &&
>  	(
> @@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' '
>  	)
>  '
>  
> -test_expect_success 'submodule add --reference' '
> +test_expect_success 'submodule add --reference uses alternates' '
>  	(
>  		cd super &&
>  		git submodule add --reference ../B "file://$base_dir/A" sub &&
>  		git commit -m B-super-added
> -	)
> -'
> -
> -test_expect_success 'after add: existence of info/alternates' '
> -	test_line_count = 1 super/.git/modules/sub/objects/info/alternates
> -'
> -
> -test_expect_success 'that reference gets used with add' '
> -	(
> -		cd super/sub &&
> -		echo "0 objects, 0 kilobytes" > expected &&
> -		git count-objects > current &&
> -		diff expected current
> -	)
> -'

Completely unrelated tangent, but after seeing the "how would we make a more intelligent choice of the diff boundary" topic, I wondered if we can notice that at this point there is a logical boundary and do something intelligent about it. All the removed lines above have become "test_alternate" we see below, while all the removed lines below upto "test_alternate" correspond to the updated test at the end.

Show 21 quoted lines

> -test_expect_success 'cloning superproject' '
> -	git clone super super-clone
> -'
> -
> -test_expect_success 'update with reference' '
> -	cd super-clone && git submodule update --init --reference ../B
> -'
> -
> -test_expect_success 'after update: existence of info/alternates' '
> -	test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
> +	) &&
> +	test_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub
>  '
>  
> -test_expect_success 'that reference gets used with update' '
> -	cd super-clone/sub &&
> -	echo "0 objects, 0 kilobytes" > expected &&
> -	git count-objects > current &&
> -	diff expected current
> +test_expect_success 'updating superproject keeps alternates' '
> +	test_when_finished "rm -rf super-clone" &&

This one is new; we do not remove A, B or super. Does it matter if we leave super-clone behind? Is super-clone so special?

Show 6 quoted lines

> +	git clone super super-clone &&
> +	git -C super-clone submodule update --init --reference ../B &&
> +	test_alternate_usage super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
>  '
>  
>  test_done