Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
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)")
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 actualShow 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_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.
Michael Rappazzo
