Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 8, 2016, 18:05 UTC
- Message-ID
- <xmqqvb2ozapp.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <CANoM8SVr1_G6KevbGSHifGyQS-ei57q+5D+GE_QmKvf_ysF2Sg@mail.gmail.com>
Mike Rappazzo <rappazzo@gmail.com> writes:
>> Instead of cleaning things up like this, could you please please >> please fix these existing tests that chdir around without being in a >> subshell? If the "previous tests" failed before going down as this >> step expects, the "cd .. && rm -r" can make things worse.
> I originally copied the pattern from above this code: > ... > but Gábor had an objection to it [2]. So I went with this simple cleanup test.
The "|| exit 1" you see everywhere is a sure sign that these tests are not designed correctly. These existing tests that do too many things outside test_expect_success block needs to be redone, so that each of them can do what it wants to test even when previous ones failed.
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.
t/t1500-rev-parse.sh | 82 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 34 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index 48ee077..22b52c0 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -5,65 +5,79 @@ test_description='test git rev-parse' test_rev_parse() { name=$1 - shift - - test_expect_success "$name: is-bare-repository" \ - "test '$1' = \"\$(git rev-parse --is-bare-repository)\"" + prep=$2 + shift 2 + + test_expect_success "$name: is-bare-repository" " + ( + $prep && + test '$1' = \"\$(git rev-parse --is-bare-repository)\" + )" shift [ $# -eq 0 ] && return - test_expect_success "$name: is-inside-git-dir" \ - "test '$1' = \"\$(git rev-parse --is-inside-git-dir)\"" + test_expect_success "$name: is-inside-git-dir" " + ( + $prep && + test '$1' = \"\$(git rev-parse --is-inside-git-dir)\" + )" shift [ $# -eq 0 ] && return - test_expect_success "$name: is-inside-work-tree" \ - "test '$1' = \"\$(git rev-parse --is-inside-work-tree)\"" + test_expect_success "$name: is-inside-work-tree" " + ( + $prep && + test '$1' = \"\$(git rev-parse --is-inside-work-tree)\" + )" shift [ $# -eq 0 ] && return - test_expect_success "$name: prefix" \ - "test '$1' = \"\$(git rev-parse --show-prefix)\"" + test_expect_success "$name: prefix" " + ( + $prep && + test '$1' = \"\$(git rev-parse --show-prefix)\" + )" shift [ $# -eq 0 ] && return - test_expect_success "$name: git-dir" \ - "test '$1' = \"\$(git rev-parse --git-dir)\"" + test_expect_success "$name: git-dir" " + ( + $prep && + test '$1' = \"\$(git rev-parse --git-dir)\" + )" shift [ $# -eq 0 ] && return } -# 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 -test_rev_parse toplevel false false true '' .git +test_rev_parse toplevel : false false true '' .git -cd .git || exit 1 -test_rev_parse .git/ false true false '' . -cd objects || exit 1 -test_rev_parse .git/objects/ false true false '' "$ROOT/.git" -cd ../.. || exit 1 +test_rev_parse .git 'cd .git' false true false '' . -mkdir -p sub/dir || exit 1 -cd sub/dir || exit 1 -test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git" -cd ../.. || exit 1 +test_rev_parse .git/objects/ 'cd .git/objects' false true false '' "$ROOT/.git" -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 -mkdir work || exit 1 -cd work || exit 1 -GIT_DIR=../.git -GIT_CONFIG="$(pwd)"/../.git/config -export GIT_DIR GIT_CONFIG +test_rev_parse 'core.bare undefined' 'git config --unset core.bare || :' \ + false false true -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 '' + +exit git config core.bare true test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''