Re: [PATCHv5 2/8] t7408: merge short tests, factor out testing method

From: Jacob Keller <jacob.keller@gmail.com>
Date: Aug 23, 2016, 22:10 UTC
Message-ID: <CA+P7+xrmsNBxmoR6+2C9ZVvXcVH7D+O3iYqKhVrbAsaNUXzF_A@mail.gmail.com>
In-Reply-To: <20160815215327.15682-3-sbeller@google.com>

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:

Show 8 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.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Looks good. The resulting test file is easier to read, and you created a common function to perform checking if we used alternates instead of duplicating that part multiple times.

Show 20 quoted lines

>  t/t7408-submodule-reference.sh | 48 ++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index b84c6748..dff47af 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -8,6 +8,15 @@ test_description='test clone --reference'
>
>  base_dir=$(pwd)
>
> +test_alternate_is_used () {
> +       alternates_file="$1" &&
> +       working_dir="$2" &&
> +       test_line_count = 1 "$alternates_file" &&
> +       echo "0 objects, 0 kilobytes" >expect &&
> +       git -C "$working_dir" count-objects >actual &&
> +       test_cmp expect actual
> +}
> +

This change wasn't mentioned in the description. You updated the tests to use a stronger check for when alternates is in use. This is good. I might have mentioned it in the description but it's not worth a re-roll.

Show 5 quoted lines

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

The stronger variant was used once here, but you use it in several locations. It's good to have extracted this as the new tests are more readable.

Thanks, Jake