git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Matthieu Moy <Matthieu.Moy@imag.fr>,
	git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v2] test-lib: user-friendly alternatives to test [!]  [-d|-f]
Date: Sat, 7 Aug 2010 00:21:34 +0000	[thread overview]
Message-ID: <AANLkTimiSJQPcZRZ06BamJPkd8PBkm7CaMcsKRSdEeP_@mail.gmail.com> (raw)
In-Reply-To: <20100806225705.GA2534@burratino>

On Fri, Aug 6, 2010 at 22:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Matthieu Moy wrote:
>        - test_file_exists <name> [<diagnosis>]
>        - test_dir_exists <name> [<diagnosis>]
>
>          Check that <name> exists and is a file or directory,
>          printing a diagnostic if it does not.  The <diagnosis>
>          if present will be used to give some added context to
>          the diagnostic.
>
>        - test_does_not_exist <name> [<diagnosis>]
>
>          Check that <name> does not exist, printing a
>          diagnostic if it does.  The <diagnosis> will be
>          printed on failure as added context if present.
>
> I think the ..._must_exist names put the emphasis in the
> wrong place, and they look funny in "if" statements.

Personally I'd prefer something where I can just think in the "test"
bultin terms and still get debug info, something like (pseduocode)

    gittest() {
        test "$#" = 3 && { bool='!'; shift; } || bool=
        test=$1; shift
        args="$@"; shift
        case "$test" in
        -f)
            # Handle the common case of -f with a custom message
        ;;
        -d)
            # Same for -d
        ;;
        *)
            # Just pass a switch to test and say "test with the -X
switch", or something
esac
    }

    gittest ! -f ~/.gitconfig
    gittest -f ~/.gitconfig
    gittest -d /tmp
    gittest ! -d /tmp
    gittest -s /tmp

I'll never be able to fit more test_* functions in my brain, and I
wrote the docs :)

> Style nitpick: if statementss in the test-lib have tended to look like
>
>  if [ foo ]
>  then
>        bar
>  fi
>
> so far.  Here the whole function is a glorified "test -f", so I wonder
> if
>
>        [ -f "$1" ] ||
>        {
>                echo >&2 "file $1 doesn't exist. $*"
>                false
>        }
>
> would not be clearer.  I dunno.

This is the style we usually use:

    if ! test -f "$1"
    then
        echo >&2 "file $1 doesn't exist. $*"
        false
    fi

> I have often run into silent test failures of the sort your patch
> is designed to avoid.  Thanks for tackling it.

Yeah, having more intra-test progress is definitely good. Right now I
just remove things from the tests in an ad-hoc fashion until they
start passing if they fail when I debug them.

I mentioned that we could emit these test progress reports as TAP in a
previous E-Mail. Here's how that could look like:

    $ perl -MTest::More=no_plan -E '
        subtest "A git test" => sub {
            pass("doing test -f file");
            pass("git commit ...");
            pass("test_tick...");
            done_testing();
        } for 1 .. 2
    '
        ok 1 - doing test -f file
        ok 2 - git commit ...
        ok 3 - test_tick...
        1..3
    ok 1 - A git test
        ok 1 - doing test -f file
        ok 2 - git commit ...
        ok 3 - test_tick...
        1..3
    ok 2 - A git test
    1..2

I.e. we could make these intra-test progress reports machine readable.

As the example shows the obvious next step would be to make other
utility functions like test_commit() emit a progress status as well.

  reply	other threads:[~2010-08-07  0:21 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-05 13:00 [PATCH] rebase -i: add exec command to launch a shell command Matthieu Moy
2010-08-05 13:31 ` Ævar Arnfjörð Bjarmason
2010-08-05 16:47   ` Matthieu Moy
2010-08-05 16:54     ` [PATCH 1/2] " Matthieu Moy
2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
2010-08-05 17:03       ` [PATCH v2] " Matthieu Moy
2010-08-06 22:57         ` Jonathan Nieder
2010-08-07  0:21           ` Ævar Arnfjörð Bjarmason [this message]
2010-08-09 16:25       ` [PATCH 2/2] " Junio C Hamano
2010-08-10 11:00         ` [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e] Matthieu Moy
2010-08-10 11:11           ` Joshua Juran
2010-08-10 11:18             ` [PATCH v3] " Matthieu Moy
2010-08-05 18:24     ` [PATCH] rebase -i: add exec command to launch a shell command Erik Faye-Lund
2010-08-05 18:37     ` Jacob Helwig
2010-08-05 20:16       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AANLkTimiSJQPcZRZ06BamJPkd8PBkm7CaMcsKRSdEeP_@mail.gmail.com \
    --to=avarab@gmail.com \
    --cc=Matthieu.Moy@imag.fr \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).