Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 13, 2016, 04:54 UTC
Message-ID: <CAPig+cT8oD1jJdDrk+YPoquGfTh6m3m-ha0J+er42jOoxnVxzg@mail.gmail.com>
In-Reply-To: <1460200767-32864-2-git-send-email-rappazzo@gmail.com>

On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:

Show 7 quoted lines

> t1500-rev-parse has many tests which change directories and leak
> environment variables.  This makes it difficult to add new tests without
> minding the environment variables and current directory.
>
> Each test is now setup, executed, and cleaned up without leaving anything
> behind.  Tests which have textual expectations have been converted to use
> test_cmp (which will show a diff when the test is run with --verbose).

It might be easier to review this if broken into several cleanup and modernization patches, however, some comments below...

Show 11 quoted lines

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,88 +3,571 @@
> +test_expect_success '.git/: is-bare-repository' '
> +       (cd .git && test false = "$(git rev-parse --is-bare-repository)")
> +'
>
> -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> +test_expect_success '.git/: is-inside-git-dir' '
> +       (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")

Simpler:

    test true = "$(git -C .git rev-parse --is-inside-git-dir)"

Show 5 quoted lines

> +'
>
> -ROOT=$(pwd)
> +test_expect_success '.git/: is-inside-work-tree' '
> +       (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")

Ditto.

Show 10 quoted lines

> +'
>
> -test_rev_parse toplevel false false true '' .git "$ROOT/.git"
> +test_expect_success '.git/: prefix' '
> +       (
> +               cd .git &&
> +               echo >expected &&
> +               git rev-parse --show-prefix >actual &&
> +               test_cmp expected actual
> +       )

Likewise, you could drop the entire subshell:

    echo >expected &&
    git -C .git rev-parse --show-prefix >actual &&
    test_cmp expected actual

Show 9 quoted lines

> +'
>
> +test_expect_success '.git/: git-dir' '
> +       (
> +               cd .git &&
> +               echo . >expected &&
> +               git rev-parse --git-dir >actual &&
> +               test_cmp expected actual
> +       )

Same here and for many subsequent tests (which I won't quote).

Show 5 quoted lines

> +'
> +test_expect_success 'core.bare = true: is-bare-repository' '
> +       git config core.bare true &&
> +       test_when_finished "git config core.bare false" &&
> +       test true = "$(git rev-parse --is-bare-repository)"

Simpler:

    test_config core.bare true

and then you can drop 'test_when_finished' altogether. However, even simpler:

    test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"

which allows you to drop 'test_config', as well.

Ditto for subsequent tests (which I won't quote).

> +'
> +test_expect_success 'core.bare undefined: is-bare-repository' '
> +       git config --unset core.bare &&

    test_unconfig core.bare

> +       test_when_finished "git config core.bare false" &&

Why does this need to re-instate core.bare?

Same comments for subsequent tests.

Show 13 quoted lines

> +       test false = "$(git rev-parse --is-bare-repository)"
> +'
> +test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
> +       mkdir work &&
> +       test_when_finished "rm -rf work && git config core.bare false" &&
> +       (
> +               cd work &&
> +               export GIT_DIR=../.git &&
> +               export GIT_CONFIG="$(pwd)"/../.git/config
> +               git config core.bare false &&
> +               test false = "$(git rev-parse --is-bare-repository)"
> +       )
> +'

Same comments about -C to avoid the subshell and -c for configuration.

Also, you can do one-shot environment variable setting for the command invocation, so the subshell goes away, and everything inside the subshell collapses to:

    test false = "$(GIT_DIR=... GIT_CONFIG=...
        git -C work -c core.bare=false rev-parse ...)"

Additionally, I'm confused about why this test "reverts" the core.bare=false by setting core.bare=false in 'test_when_finished'.

Ditto for subsequent tests.

> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
> +       mkdir work &&
> +       cp -r .git repo.git &&
> +       test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&

If 'cp' fails, then 'test_when_finished' will never be invoked, which means that the cleanup will never happen; thus 'test_when_finished' needs to be called earlier. Ditto for subsequent tests.

Show 8 quoted lines

> +       (
> +               cd work &&
> +               export GIT_DIR=../repo.git &&
> +               export GIT_CONFIG="$(pwd)"/../repo.git/config
> +               git config core.bare false &&
> +               test false = "$(git rev-parse --is-bare-repository)"
> +       )
> +'

Closing comments:

By using -C, -c, and one-shot environment variables, you can ditch the subshells, and most of these tests should collapse to one or two lines.

There seems to be a lot of repetition here. To reduce the repetition, have you considered encoding the state which varies between tests into a table and making the tests table-driven. Or, by encoding the varying state in some nested for-loops? The nice thing about driving the tests from a table or for-loops is that it is easier to see at a glance if all cases are covered.