From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 04 Oct 2010 03:48:43 GMT
Subject: Re: [PATCHv6 01/16] test-lib: make test_expect_code a test command
Message-ID: <AANLkTinVvmJMEDhPcxa_CiOL2_RsYBdo-JywXi2gKeYp@mail.gmail.com>
In-Reply-To: <7vfwwmdbgl.fsf@alter.siamese.dyndns.org>

On Mon, Oct 4, 2010 at 00:54, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index f688bd3..c2f5f8d 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -130,22 +130,57 @@ test_expect_success 'tests clean up after themselves' '
>> ...
>> +#!$SHELL_PATH
>> +
>> +test_description='Failing tests with cleanup commands'
>> +
>> +# Point to the t/test-lib.sh, which isn't in ../ as usual
>> +TEST_DIRECTORY=\"$TEST_DIRECTORY\"
>
> Do we know at this point that the string in $TEST_DIRECTORY can safely be
> quoted by just a simple dq pair?  I don't think so--isn't the leading part
> of that string whatever the user can choose (i.e. $(cwd))?
>
> This seems sloppy.
>
>> +. \"\$TEST_DIRECTORY\"/test-lib.sh
>
> Once you get the right value in TEST_DIRECTORY, this one is correct.

Wouldn't the issue you and Jonathan are talking about disappear with:

    diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
    index c2f5f8d..69e219b 100755
    --- a/t/t0000-basic.sh
    +++ b/t/t0000-basic.sh
    @@ -58,7 +58,7 @@ test_expect_failure 'pretend we have a known breakage' '
     test_expect_success 'pretend we have fixed a known breakage (run
in sub test-lib)' "
         mkdir passing-todo &&
         (cd passing-todo &&
    -    cat >passing-todo.sh <<EOF &&
    +    cat >passing-todo.sh <<\EOF &&
     #!$SHELL_PATH

     test_description='A passing TODO test
    @@ -139,7 +139,7 @@ fi
     test_expect_success 'tests clean up even on failures' "
         mkdir failing-cleanup &&
         (cd failing-cleanup &&
    -    cat >failing-cleanup.sh <<EOF &&
    +    cat >failing-cleanup.sh <<\EOF &&
     #!$SHELL_PATH

     test_description='Failing tests with cleanup commands'

Note the change to passing-todo, that uses the same setup style.

With that the output of:

    $ rm -rfv trash*; ./t0000-basic.sh --debug; cat trash\
directory.t0000-basic/{passing-todo,failing-cleanup}/*.sh

Is now (cut):

    # Point to the t/test-lib.sh, which isn't in ../ as usual
    TEST_DIRECTORY="/home/avar/g/git/t"
    . "$TEST_DIRECTORY"/test-lib.sh

    test_expect_failure 'pretend we have fixed a known breakage' '
        :
    '

    test_done
    #!/bin/sh

    test_description='Failing tests with cleanup commands'

    # Point to the t/test-lib.sh, which isn't in ../ as usual
    TEST_DIRECTORY="/home/avar/g/git/t"
    . "$TEST_DIRECTORY"/test-lib.sh

Instead of previously being:

    # Point to the t/test-lib.sh, which isn't in ../ as usual
    TEST_DIRECTORY="/home/avar/g/git/t"
    . "/home/avar/g/git/t"/test-lib.sh

    test_expect_failure 'pretend we have fixed a known breakage' '
        :
    '

    test_done
    #!/bin/sh

    test_description='Failing tests with cleanup commands'

    # Point to the t/test-lib.sh, which isn't in ../ as usual
    TEST_DIRECTORY="/home/avar/g/git/t"
    . "/home/avar/g/git/t"/test-lib.sh

I.e. before the TEST_DIRECTORY was always expanded.