Re: [PATCH] Test suite
Dear diary, on Thu, May 12, 2005 at 02:01:34AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> told me that...
Show 11 quoted lines
> Commit 1da683e1247046796a094c4917bc0c4591530272
> Author Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700
> Committer Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700
>
> Test suite: infrastructure and examples.
>
> This adds the test suite infrastructure with two example tests.
> The current git-checkout-cache the example tests would fail this
> test and will be corrected in a separate patch.
>
> Signed-off-by: Junio C Hamano <junkio@cox.net>
Admittely, I'm not happy with this. From the design standpoint it looks mostly fine now, but the code is rather crude. I wanted to go over it at first and fix the obvious stuff, but it appeared to be overall quite broken, so I decided to return it to you for another iteration. I don't mind if you just fix the broken code for now, we can fix the semantics and design stuff later - what is in the patch is already good enough for the start.
I'll drop the testcases from your other patches for now so that we don't get long stalls.
Show 5 quoted lines
> --- /dev/null
> +++ b/t/test-lib.sh
> +# For repeatability, reset the environment to known value.
> +export LANG C
> +export TZ UTC
Dunno about *your* shell but this just exports variables $LANG, $C, $TZ and $UTC here. You probably wanted assignments there?
> +# Each test should start with something like this, after copyright notices:
> +#
> +# . ./testlib.sh
> +# test_description "$@" 'Description of this test...
> +# This test checks if command xyzzy does the right thing...
> +# '
I think this usage is pretty weird. Why not just
test_description='Description, blah blah.' . ./testlib.sh
I think it would be quite less confusing than test_description, which actually does effectively something different anyway.
> +
> +test_description () {
> + while case "$#" in 0) break;; esacDuh. This looks mysterious - why not a simple test?
Show 10 quoted lines
> + do
> + case "$1" in
> + -d|--d|--de|--deb|--debu|--debug)
> + debug=t; shift ;;
> + -h|--h|--he|--hel|--help)
> + eval echo '"$'$#'"'
> + exit 0
> + ;;
> + *)
> + break ;;
This branch makes no sense, I think.
Show 11 quoted lines
> + esac
> + done
> + test_failure=0
> +}
> +
> +say () {
> + echo "* $*"
> +}
> +
> +test_debug () {
> + case "$debug" in '') ;; ?*) eval "$*" ;; esacAgain, why not a simple test?
[ "$debug" ] && eval "$*"
(Actually, eval will do the wrong thing here - it just concatenates the arguments. Just "$@" would do, I guess.)
Show 32 quoted lines
> +}
> +
> +test_ok () {
> + echo "* $*";
> +}
> +
> +test_failure () {
> + echo "* $*";
> + test_failure=1;
> +}
> +
> +test_expect_failure () {
> + say "expecting failure: $1"
> + eval "$1"
> + case $? in
> + 0) test_failure "did not fail as expected" ;;
> + *) test_ok "failed as expected" ;;
> + esac
> +}
> +
> +test_expect_success () {
> + say "expecting success: $1"
> + eval "$1"
> + case $? in
> + 0) test_ok "succeeded as expected" ;;
> + *) test_failure "did not succeed as expected" ;;
> + esac
> +}
> +
> +test_done () {
> + case "$test_failure" in
> + 0) exit 0 ;;Please clean up after yourself in this case.
Show 16 quoted lines
> + '') echo "*** test script did not start with test_description";
> + exit 2 ;;
> + *) exit 1 ;;
> + esac
> +}
> +
> +# Test the binaries we have just built. The tests are kept in
> +# t/ subdirectory and are run in test-repo subdirectory.
> +PATH=$(pwd)/..:$PATH
> +
> +# Test repository
> +test=test-repo
> +rm -fr "$test"
> +mkdir "$test"
> +cd "$test"
> +git-init-db 2>/dev/null || error "cannot run git-init-db"
But there's no 'error' thing.
> --- /dev/null
> +++ b/t/t1000-checkout-cache.sh
> +git-update-cache --add path0 path1/file1
You should make sure even those preparations calls actually succeed.
> --- /dev/null
> +++ b/t/t1001-checkout-cache.sh
> +git-update-cache --add path0/file0
> +tree1=$(git-write-tree)
Here too.
The testcases currently utterly fail, which is not good sign - either they are ahead of the current code, or they are broken. This is the main hurdle making me not accept it yet - it does not work for me. If you fix this and the nits above, it can go in, I think.
* expecting failure: git-checkout-cache -a
checkout-cache: path0 already exists
error: checkout-cache: unable to create file path1/file1 (Not a directory)
* did not fail as expected
* expecting success: git-checkout-cache -f -a
error: checkout-cache: unable to create file path0 (Is a directory)
* succeeded as expected
* checkout failed
I consider this output... well, totally confusing. checkout-cache fails but testcase thinks it does not, then it fails again but testcase thinks it succeeded as expected but dies anyway.
I think it's too messy. I would much more appreciate output like this:
* checkout-cache test [1/3]: git-checkout-cache -a... passed
* checkout-cache test [2/3]: git-checkout-cache -f -a... NOT PASSED
Expected success, but the command failed. Output:
error: checkout-cache: unable to create file path0 (Is a directory)
* checkout-cache test [3/3]: git-checkout-cache foobar... NOT PASSED
Expected failure, but the command succeeded.
Much less cluttered, it is clear what went wrong etc.
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor
