How do I run tests under Valgrind?

Hi,

I tried running `make valgind` inside t/ and got:

    bug in test framework: multiple prerequisite tags do not work reliably

which means that even the basic tests don't pass.  Am I doing something wrong?

Ram

On Mon, Sep 17, 2012 at 10:31:07PM +0530, Ramkumar Ramachandra wrote:

No, that should work (and it does work here). I assume you can pass t0000 without --valgrind?

It seems very odd that valgrind would have an impact here, since those tests are not even running git.

-Peff

Hi Peff,

Jeff King wrote:

Yes. Here's the output from running it with --valgrind. I'm digging deeper to see what went wrong.

make_valgrind_symlink:6: permission denied:
/home/artagnon/src/git/t/../git-instaweb
./test-lib.sh:487: no matches found:
/home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/bin:/usr/local/bin:/usr/bin:/bin:/usr/games:/home/artagnon/.ec2/bin:/home/artagnon/.ec2/bin/git-*
Initialized empty Git repository in /home/artagnon/src/git/t/trash
directory.test-lib/.git/
expecting success:
        find .git/objects -type f -print >should-be-empty &&
        test_line_count = 0 should-be-empty

ok 1 - .git/objects should be empty after git init in an empty repo

expecting success:
        find .git/objects -type d -print >full-of-directories &&
        test_line_count = 3 full-of-directories

ok 2 - .git/objects should have 3 subdirectories

expecting success:
        :

ok 3 - success is reported like this

checking known breakage:
        false

not ok 4 - pretend we have a known breakage # TODO known breakage

expecting success:
        mkdir passing-todo &&
        (cd passing-todo &&
        cat >passing-todo.sh <<-EOF &&
        #!/bin/sh

        test_description='A passing TODO test

        This is run in a sub test-lib so that we do not get incorrect
        passing metrics
        '

        # Point to the t/test-lib.sh, which isn't in ../ as usual
        TEST_DIRECTORY="/home/artagnon/src/git/t"
        . "$TEST_DIRECTORY"/test-lib.sh

        test_expect_failure 'pretend we have fixed a known breakage' '
                :
        '

        test_done
        EOF
        chmod +x passing-todo.sh &&
        ./passing-todo.sh >out 2>err &&
        ! test -s err &&
        sed -e 's/^> //' >expect <<-\EOF &&
        > ok 1 - pretend we have fixed a known breakage # TODO known breakage
        > # fixed 1 known breakage(s)
        > # passed all 1 test(s)
        > 1..1
        EOF
        test_cmp expect out)

test_cmp:1: command not found: diff -u not ok - 5 pretend we have fixed a known breakage (run in sub test-lib) # # mkdir passing-todo && # (cd passing-todo && # cat >passing-todo.sh <<-EOF && # #!/bin/sh # # test_description='A passing TODO test # # This is run in a sub test-lib so that we do not get incorrect # passing metrics # ' # # # Point to the t/test-lib.sh, which isn't in ../ as usual # TEST_DIRECTORY="/home/artagnon/src/git/t" # . "$TEST_DIRECTORY"/test-lib.sh # # test_expect_failure 'pretend we have fixed a known breakage' ' # : # ' # # test_done # EOF # chmod +x passing-todo.sh && # ./passing-todo.sh >out 2>err && # ! test -s err && # sed -e 's/^> //' >expect <<-\EOF && # > ok 1 - pretend we have fixed a known breakage # TODO known breakage # > # fixed 1 known breakage(s) # > # passed all 1 test(s) # > 1..1 # EOF # test_cmp expect out) #

expecting success:
        test_have_prereq HAVEIT &&
        haveit=yes

ok 6 - test runs if prerequisite is satisfied

skipping test: unmet prerequisite causes test to be skipped
        donthaveit=no

ok 7 # skip unmet prerequisite causes test to be skipped (missing DONTHAVEIT)

skipping test: test runs if prerequisites are satisfied
        test_have_prereq HAVEIT &&
        test_have_prereq HAVETHIS &&
        haveit=yes

ok 8 # skip test runs if prerequisites are satisfied (missing HAVETHIS,HAVEIT)

skipping test: unmet prerequisites causes test to be skipped
        donthaveit=no

ok 9 # skip unmet prerequisites causes test to be skipped (missing HAVEIT,DONTHAVEIT)

skipping test: unmet prerequisites causes test to be skipped
        donthaveiteither=no

ok 10 # skip unmet prerequisites causes test to be skipped (missing DONTHAVEIT,HAVEIT)

bug in test framework: multiple prerequisite tags do not work reliably
FATAL: Unexpected exit with code 1

Yeah, I know!

Ram

On Mon, Sep 17, 2012 at 10:53:18PM +0530, Ramkumar Ramachandra wrote:

That's certainly odd. It sounds like the valgrind setup is broken for you. Can you run:

  sh -x t0000-basic.sh --valgrind

and see what's happening near those weird errors?

Lack of diff is going to be a problem. What OS is this? Do you really not have diff? Or is there something funny going on with your PATH?

-Peff

Hi again,

Jeff King wrote:

Not helpful:

+ . ./test-lib.sh
+ mkdir -p test-results
+ basename t0000-basic.sh .sh
+ BASE=test-results/t0000-basic
+ GIT_TEST_TEE_STARTED=done /usr/bin/zsh t0000-basic.sh --valgrind
+ tee test-results/t0000-basic.out

It's plain Ubuntu.  Ofcourse I have `diff`- I don't know what's going on.

Ram

On Mon, Sep 17, 2012 at 11:09:27PM +0530, Ramkumar Ramachandra wrote:

Oh, bleh. Stupid automatic --tee for valgrind. Try this:

  SHELL="/usr/bin/zsh -x" ./t0000-basic.sh --valgrind

I am also doing my tests with "dash" as my shell. You might try setting your SHELL to /bin/sh to see if it makes a difference.

Maybe the PATH-munging in test-lib.sh is screwing up your PATH somehow. But it all looks pretty simple. The "sh -x" output can maybe tell us more.

-Peff

Am 17.09.2012 19:44, schrieb Jeff King:

Shouldn't -v be used as well? Or is --valgrind different?

-- Hannes

On Mon, Sep 17, 2012 at 07:55:24PM +0200, Johannes Sixt wrote:

It turns "-v" on automatically.

-Peff

Hi,

Jeff King wrote:

I got tons of output, but found it unhelpful.  Any other ideas?

Ram

On Mon, Sep 17, 2012 at 11:19:35PM +0530, Ramkumar Ramachandra wrote:

Weird. The line in question is this:

  test "#!" != "$(head -c 2 < "$symlink_target")"

Is there some problem with running "head" on your system? If it were head failing to open git-instaweb, it would look more like:

  $ head -c 2 /etc/shadow
  head: cannot open `/etc/shadow' for reading: Permission denied

-Peff

Hi Peff,

I was able to reproduce the problem on all my machines, and I consider this very disturbing. However, I was successfully able to corner the issue. I have an overtly long $PATH that's not getting split properly by `IFS=:` in one corner case -- in other words, this shell script fails to execute properly when called with `--tee` (just set a really long $PATH and try):

    case "$GIT_TEST_TEE_STARTED, $* " in
    done,*)
    	# do not redirect again
    	;;    #!/bin/sh

    case "$GIT_TEST_TEE_STARTED, $* " in
    done,*)
    	# do not redirect again
    	;;
    *' --tee '*|*' --va'*)
    	mkdir -p test-results
    	BASE=test-results/$(basename "$0" .sh)
    	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
    	 echo $? > $BASE.exit) | tee $BASE.out
    	test "$(cat $BASE.exit)" = 0
    	exit
    	;;
    esac

    OLDIFS=$IFS
    IFS=:
    for path in $PATH
    do
        ls "$path"/git-* 2> /dev/null |
        while read file
        do
    	echo $file
        done
    done

I'm still trying to figure out what exactly the problem is, and how to patch it.

Thanks.

Ram

Hi again,

Ramkumar Ramachandra wrote:

Oops. Looks like it has nothing to do with an overtly long $PATH. It has something to do with $SHELL being zsh though, because other shells work. Looking deeper into this.

Ram

On 09/21/2012 09:58 PM, Ramkumar Ramachandra wrote:

Zsh doesn't do word-splitting by default on variable expansions:

    $ zsh -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
    1 2 3

unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility mode somehow:

    $ zsh -o SH_WORD_SPLIT -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
    1
    2
    3

    $ zsh -c 'emulate sh; v="1 2 3"; for x in $v; do echo "$x"; done'
    1
    2
    3

More info at: <http://zsh.sourceforge.net/FAQ/zshfaq02.html>

HTH,
  Stefano

Hi Stefano,

Stefano Lattarini wrote:

... but didn't we set $IFS for this purpose? The following segment of code works:

    IFS=:
    for path in $PATH
    do
        ls "$path"/git-* 2> /dev/null |
        while read file
        do
    	    echo $file
        done
    done

Ram

Hi again,

Ramkumar Ramachandra wrote:

I'm sorry, it doesn't.  That is the problem.

Ram

On Fri, Sep 21, 2012 at 10:13:09PM +0200, Stefano Lattarini wrote:

Oh. It sounds like setting $SHELL to zsh is really the problem, then. If it is not Bourne-compatible when called as "zsh", then it really should be called in a way that turns on compatibility mode (bash will do this when called as "sh", but you can also do it with "bash --posix").

-Peff

On 09/21/2012 10:49 PM, Jeff King wrote:

AFAIK, if Zsh is called as "sh", it too will run in Bourne compatibility mode; not sure how to force this compatibility from the command line though (albeit I'd guess there is some way to do so).

As further reference, here is the trick autoconf uses to (try to) put Zsh in Bourne compatibility mode after it has been invoked:

    if test -n "${ZSH_VERSION+set}" && (emulate sh) >/dev/null 2>&1; then
       emulate sh
       NULLCMD=:
       # Pre-4.2 versions of Zsh do word splitting on ${1+"$@"}, which
       # is contrary to our usage.  Disable this feature.
       alias -g '${1+"$@"}'='"$@"'
       setopt NO_GLOB_SUBST
    fi

You might think to use something like that in 't/test-lib.sh' (and other "shell libraries" of the testsuite), but sadly that would not be enough; this excerpt from the Automake suite shows why:

   # If Zsh is not started directly in POSIX-compatibility mode, it has some
   # incompatibilities in the handling of $0 that conflict with our usage;
   # i.e., $0 inside a file sourced with the '.' builtin is temporarily set
   # to the name of the sourced file.  Work around that.  The apparently
   # useless 'eval' here is needed by at least dash 0.5.2, to prevent it
   # from bailing out with an error like "Syntax error: Bad substitution".
   # Note that a bug in some versions of Zsh prevents us from resetting $0
   # in a sourced script, so the use of $argv0.  For more info see:
   #  <http://www.zsh.org/mla/workers/2009/msg01140.html>
   eval 'argv0=${functrace[-1]%:*}' && test -f "$argv0" || {
     echo "Cannot determine the path of running test script." >&2
     echo "Your Zsh (version $ZSH_VERSION) is probably too old." >&2
     exit 99
   }

Since I see that '$0' is used in (at least) 't/perf/perf-lib.sh' and 't/test-lib.sh', you'd need to copy the snippet above (or write an equivalent one) in those files, and change them to use '$argv0' instead of '$0' in few (but not all) places. Not sure whether it's worth it though -- given that you seems to have solved the issue already with a simpler change, I'd say it's not.

Regards,
  Stefano

On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:

Thanks for digging. I think this case, though, is that we were simply using the wrong variable ($SHELL instead of $SHELL_PATH). Your workarounds would help if somebody put zsh into $SHELL_PATH, but fundamentally that is not a sane thing to be doing, so I think we can just consider doing so user error and not bother working around it.

-Peff

On 09/22/2012 07:47 PM, Jeff King wrote:

FWIW, I agree.

Best regards,
  Stefano

Jeff King <peff@peff.net> writes:

Yeah, 100% agreed with your assessment, including the part thanking Stefano.

Thanks.

Replace $SHELL with an explicit `/bin/sh`, as some shells do not support all the features used in the script. For example, ZSH does not respect IFS, which is used in line 478.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote:

I don't think that is the right thing to do. The point of SHELL is to point at a bourne-compatible shell. On some systems, the main reason to set it is that /bin/sh is _broken_, and we are trying to avoid it.

A bigger question is: why are you setting SHELL=zsh in the first place?

-Peff

Hi,

Jeff King wrote:

But you're only avoiding it in the --tee/ --va* codepath. In the normal codepath, you're stuck with /bin/sh anyway.

I use ZSH as my primary shell, so SHELL is set to zsh when I run tests. How can we trust $SHELL to be a bourne-compatible shell?

Ram

On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:

No, the #!-header is only information. When you run "make test" we actually invoke the shell ourselves using $SHELL_PATH.

Ah, my fault. I was thinking we overrode $SHELL along with $SHELL_PATH, but we do not.

The correct patch is to stop using $SHELL, but not to switch to a manual /bin/sh. It should use $SHELL_PATH instead, which is how you tell git your path to a sane bourne shell.

-Peff

Hi Peff,

Jeff King wrote:

My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the Makefile. Which shell is it supposed to point to? If you're proposing to use a variable that's only set in the Makefile in the test, you're not allowing users to run the test as a standalone- that's not a good change, is it?

Ram

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Inside a makefile the variable SHELL is special in that it is never imported from the environment. If not set it defaults to /bin/sh.

Andreas.

Andreas Schwab <schwab@linux-m68k.org> writes:

Ahh, I forgot about that. Then the current construct is perfectly fine.

The reference to ${SHELL-/bin/sh} in the test need to be updated to SHELL_PATH as Peff suggested in the other subthread.

Thanks.

Hi Junio,

Junio C Hamano wrote:

For that, the entire block needs to be moved down to come after `. GIT_BUILD_DIR="$TEST_DIRECTORY"/..`. Is this okay?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Have you tested it with --tee (or valgrind) and does that work?

Hi again,

On Sat, Sep 22, 2012 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:

Yes, it works.

Ram

Here's a patch.

-- 8< --
From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sat, 22 Sep 2012 10:25:10 +0530
Subject: [PATCH] test-lib: do not trust $SHELL

Do not trust $SHELL to be a bourne-compatible shell. Instead, use the Makefile variable $SHELL_PATH. This fixes a bug: when a test was run with --tee and $SHELL was set to ZSH, $PATH on line 479 was not getting split due to ZSH not respecting $IFS.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/test-lib.sh | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The part that this starts letting run, which the original "Re-run the command under tee as early as possible" wanted to avoid running, does not affect anything that would affect how we run that tee magic (e.g. "mkdir -p test-results" will still create it directly inside the directory the test script was started in), so I think this patch is safe _for now_.

However, it forces people who need to update earlier parts of this script to be extra careful; it has been true before the patch, and the patch makes it even more so.

I am not opposed to queuing this as an interim solution, but I wonder if we can get rid of that double-launch altogether.

Instead of re-launching the script with its output piped to "tee", can't we do the same by redirecting our standard output to the file in the file, and spawn a "tail -f" that reads from the file and outputs to our original output? Something along the lines of:

        mkdir -p test-results
        tee_base=test-results/$(basename "$0" .sh)

        # empty the file and start "tail -f" on it ...
        : >"$tee_base.out"
        ( tail -f "$tee_base.out" ) &
        tee_pid=$!
	trap 'kill $tee_pid; exit' 0 1 2 3
	# ... and then redirect our output to it
        exec >"$tee_base.out"

and wrap it in a shell helper function that is called from where the parsing of the command line arguments for "--tee" happens, and don't forget to kill $tee_pid when we exit.

Hrm?

Hi Junio,

Junio C Hamano wrote:

I see you've queued it in `pu` after rewriting the commit message.

Good idea. I'll write a patch to do this once the interim solution graduates to `master`.

Ram

Ramkumar Ramachandra <artagnon@gmail.com> writes:

SHELL_PATH is always supposed to point to a Bourne that can be used to run POSIXy shell scripts. I think the fallback you pointed out above assumes that the majority of people who type "make" use Bourne compatibles as their $SHELL and the default is to help the majority.

It may not hurt to add a note to INSTALL for people who use $SHELL that is not Bourne (csh and zsh users, but there may be others) that they need to set SHELL_PATH, of course.

On Sat, Sep 22, 2012 at 03:04:50AM +0530, Ramkumar Ramachandra wrote:

Something bourne compatible. If you are on a sane system, /bin/sh is fine (and is the default).

It gets written to GIT-BUILD-OPTIONS, which should get pulled in by test-lib.sh. There may be an ordering problem with the command line parsing though.

-Peff

Jeff King <peff@peff.net> writes:

SHELL is set to the login shell by default.

Andreas.

On Fri, Sep 21, 2012 at 11:08:34PM +0200, Andreas Schwab wrote:

Yeah, sorry, I was thinking this was coming from our $SHELL_PATH Makefile variable, and that he was setting that. The real solution is to properly use $SHELL_PATH instead of $SHELL.

-Peff