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

On Wed, Apr 13, 2016 at 12:54 AM, Eric Sunshine [off-list ref] wrote:

On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo [off-list ref] wrote:

quoted

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

I felt that the changes are repetitive enough that it did not
necessitate separate patches.  Perhaps after simplifying based on your
suggestions, it will be even smaller.

quoted

Signed-off-by: Michael Rappazzo <redacted>
---

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

quoted

+'

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

Ditto.

quoted

+'

-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

quoted

+'

+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).

quoted

+'
+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).

quoted

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

    test_unconfig core.bare

quoted

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

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

Same comments for subsequent tests.

quoted

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

quoted

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

quoted

+       (
+               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.

I follow, and I can make that adjustment.  Thanks for the review.

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.

I was aiming for readability and the ability to test an individual
case.  The previous version was driven by a function, which did reduce
the repetition, but it made it not simple to run an individual test in
the case of a failure.  Would your table implementation be much
different from the function, in terms of readability?