Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory
Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> What I had in mind was more a long the lines of this change. I only
> did the first several just for illustration (I added an 'exit' to
> mark where I stopped), but I think you get the idea. The point is
> that by doing things in subprocess inside test_expect_success you
> can avoid disrupting the main test process even when a test fails as
> much as possible, and this illustrates how you would deal with the
> "cd" and "export". What I didn't handle was the updates to
> .git/config whose effects by earlier tests are relied on later tests
> in this illustration.
>
> As to "table driven" vs "explicitly spelling out scripts, accepting
> some code repetition", I tend to favor the latter slightly, unless
> the table driven approach is done in such a way that each of its
> tests are truly independent from one another, i.e. if a row of the
> table represents one test, the tests would not be disrupted by
> reordering the rows, inserting a new row in the middle, or removing
> an existing row. From my experience, "table-driven" sets of tests
> whose rows have inter-dependency has been much harder to debug when
> I had to hunt down a bug that manifests itself only in one test in
> the middle.
>
> Hope this helps clarify what I meant.
If you want to go forward in this direction, three are a few additional things to note.
Show 5 quoted lines
> + test_expect_success "$name: is-bare-repository" "
> + (
> + $prep &&
> + test '$1' = \"\$(git rev-parse --is-bare-repository)\"
> + )"
This is not a new problem, but quoting the test body with dq instead of sq pair make it ugly to read. We might want to do something like
expect=$1
test_expect_success "$name: is-bare-repository" '
(
$prep &&
test "$outcome" = "$(git rev-parse --is-bare-repository)"
)'
> shift
> [ $# -eq 0 ] && return
Also, let's avoid []; i.e.
test $# = 0 && return
Show 5 quoted lines
> -# label is-bare is-inside-git is-inside-work prefix git-dir
> +# label prep is-bare is-inside-git is-inside-work prefix git-dir
>
> ROOT=$(pwd)
> +mkdir -p sub/dir work
Remember this place in the script, I may refer to it later.
Show 9 quoted lines
> -git config core.bare true
> -test_rev_parse 'core.bare = true' true false false
> +test_rev_parse subdirectory 'cd sub/dir' \
> + false false true sub/dir/ "$ROOT/.git"
>
> -git config --unset core.bare
> -test_rev_parse 'core.bare undefined' false false true
> +test_rev_parse 'core.bare = true' 'git config core.bare true' \
> + true false false
I didn't use "test_config" and "test_unconfig" here, because you cannot use them in a subprocess. If we truly want to make these tests independent from each other, we'd need to come up with a way for the tests to revert the configuration state back to where it was before they started.
Show 16 quoted lines
>
> -mkdir work || exit 1
> -cd work || exit 1
> -GIT_DIR=../.git
> -GIT_CONFIG="$(pwd)"/../.git/config
> -export GIT_DIR GIT_CONFIG
>
> -git config core.bare false
> -test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
> +test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
> + cd work &&
> + GIT_DIR=../.git &&
> + GIT_CONFIG="$(pwd)"/../.git/config &&
> + export GIT_DIR GIT_CONFIG &&
> + git config core.bare false' \
> + false false true ''
The "prep" step for this test is long and I'd assume that the later tests would need to do something similar themselves. To avoid repetition, create a helper shell function to reduce boilerplate before the series of these tests start (i.e. the place above) and call it from the prep parts with different arguments, e.g. making the above test into
test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
cd work &&
set_common_env ../.git false' \
false false true ''
by having something like
set_dir_and_core_bare () {
GIT_DIR=$1 &&
GIT_CONFIG="$(pwd)/$GIT_DIR/config" &&
export GIT_DIR GIT_CONFIG &&
git config core.bare "$2"
}> +exit
>
> git config core.bare true
> test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
Michael Rappazzo
