Re: [PATCH 4/4] t5509: add basic tests for hideRefs

From: Lukas Fleischer <lfleischer@lfos.de>
Date: Nov 2, 2015, 06:25 UTC
Message-ID: <20151102062449.24971.39537@typhoon>
In-Reply-To: <CAPig+cR7sCqigz=--_d-bW0X=+vpbV-_N9hxriLH9fMjLJUfsA@mail.gmail.com>

On Sun, 01 Nov 2015 at 22:13:51, Eric Sunshine wrote:

Show 18 quoted lines

> On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer <lfleischer@lfos.de> wrote:
> > Test whether regular and full hideRefs patterns work as expected when
> > namespaces are used.
> >
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> > ---
> > diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> > @@ -82,4 +82,33 @@ test_expect_success 'mirroring a repository using a ref namespace' '
> >         )
> >  '
> >
> > +test_expect_success "Hide namespaced refs with transfer.hideRefs" '
> 
> None of the other tests in this file capitalize the test description.
> These new test descriptions should probably follow suit by beginning
> with lowercase. It is also typical to use single quotes for the
> description rather than double.
>

Good catch. I originally put these tests somewhere else and noticed that adding them to t5509 is much better since we already set up the whole namespace infrastructure there. Seems like I forgot to adjust the style. Fixed that locally.

Show 22 quoted lines

> > +       cd pushee &&
> > +       test_config transfer.hideRefs refs/tags &&
> > +       GIT_NAMESPACE=namespace git ls-remote "ext::git %s ." >actual &&
> > +       printf "$commit1    refs/heads/master\n" >expected &&
> > +       test_cmp expected actual &&
> > +       cd ..
> 
> If any of the commands above "cd .." fail, then "cd .." will never be
> invoked, thus subsequent tests will fail since they won't be executed
> in the expected directory. The typical way to handle this is to place
> the "cd foo" and remaining test body in a subshell, and drop "cd .."
> altogether. When the subshell exits (via success or failure), the
> working directory will be restored automatically.
> 
>     test_expect_success '...' '
>         (
>             cd pushee &&
>             test_config ... &&
>             ...
>         )
>     '
> [...]

I chose the `cd ..` approach because test_config does not work from a subshell. However, searching the Git log for "test_config", I found 1a9a23e (t7610: don't use test_config in a subshell, 2015-09-05) and da568b6 (t7800: don't use test_config in a subshell, 2015-09-05) which suggest to use the -C switch. The test cases now look like this:

    test_expect_success '[...]' '
        test_config -C pushee transfer.hideRefs [...] &&
        (
            cd pushee &&
            [...]
        )
    '

Thanks!