Re: [PATCH v2 1/4] archive: follow test standards around assertions

From: Stefan Beller <sbeller@google.com>
Date: Sep 27, 2018, 18:38 UTC
Message-ID: <CAGZ79kZ2aqW7X57KHPtjQiyWtwfk4SGZYRqwccgy9ji0s+sYYQ@mail.gmail.com>
In-Reply-To: <20180927012455.234876-2-steadmon@google.com>

On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon <steadmon@google.com> wrote:

>
> Move assertions outside of the check_tar function so that all top-level
> code is wrapped in a test_expect_* assertion.

Cool, I'll file this under modernizing the test infrastructure ;-)

Show 36 quoted lines

> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0a..c408e3a23d 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -62,11 +62,9 @@ check_tar() {
>         dir=$1
>         dir_with_prefix=$dir/$2
>
> -       test_expect_success ' extract tar archive' '
> -               (mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile
> -       '
> +       (mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile &&
>
> -       test_expect_success TAR_NEEDS_PAX_FALLBACK ' interpret pax headers' '
> +       if test_have_prereq TAR_NEEDS_PAX_FALLBACK ; then
>                 (
>                         cd $dir &&
>                         for header in *.paxheader
> @@ -82,16 +80,11 @@ check_tar() {
>                                 fi
>                         done
>                 )
> -       '
> +       fi &&
>
> -       test_expect_success ' validate filenames' '
> -               (cd ${dir_with_prefix}a && find .) | sort >$listfile &&
> -               test_cmp a.lst $listfile
> -       '
> -
> -       test_expect_success ' validate file contents' '
> -               diff -r a ${dir_with_prefix}a
> -       '
> +       (cd ${dir_with_prefix}a && find .) | sort >$listfile &&
> +       test_cmp a.lst $listfile &&
> +       diff -r a ${dir_with_prefix}a

Up to here we unwrapped code and removed test_expect_success and just executed the code as is, so later callers would need to encapsulate the call to check_tar with test_expect_success.

However as we are touching the code here, we can go further than just unwrapping it, usually we'd format one command a line,

    (
        cd ${dir_with_prefix}a &&
        find .
    ) | sort >$listfile &&
    test_cmp ...

I am not sure if that standard style is more legible in this case though.

Show 9 quoted lines

>  }
>
>  test_expect_success \
> @@ -143,19 +136,20 @@ test_expect_success \
>      'git archive' \
>      'git archive HEAD >b.tar'
>
> -check_tar b
> +test_expect_success 'extract archive' 'check_tar b'

Heh. Just looked into the file and the surrounding code is a wild mixture of the old style

test_expect_success \
    'git archive' \
    'git archive HEAD >b.tar'

check_tar b

and the new style

test_expect_success 'test name' '
 <TAB> command &&
 <TAB> command2
'

Maybe we could cleanup that file to look more like one of the newer tests (e.g. t3206, t0410) ?

But I guess for the purpose of getting the check_tar function usable inside a test, this would do enough.

Show 15 quoted lines

>
>  test_expect_success 'git archive --prefix=prefix/' '
>         git archive --prefix=prefix/ HEAD >with_prefix.tar
>  '
>
> -check_tar with_prefix prefix/
> +test_expect_success 'extract with prefix' 'check_tar with_prefix prefix/'
>
>  test_expect_success 'git-archive --prefix=olde-' '
>         git archive --prefix=olde- HEAD >with_olde-prefix.tar
>  '
>
> -check_tar with_olde-prefix olde-
> +test_expect_success 'extract with olde- prefix' \
> +       'check_tar with_olde-prefix olde-'

In new style this would look like

    test_expect_success 'extract with olde- prefix' '
        check_tar with_olde-prefix olde-
    '