From: Jonathan Nieder <jrnieder@gmail.com>
Date: Fri, 06 Aug 2010 22:57:05 GMT
Subject: Re: [PATCH v2] test-lib: user-friendly alternatives to test [!] [-d|-f]
Message-ID: <20100806225705.GA2534@burratino>
In-Reply-To: <1281027831-22739-1-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy wrote:

> The helper functions are implemented, documented, and used in a few
> places to validate them

When I first read this, I thought you were saying these helpers
already existed.  This is where the rationale goes, anyway, so maybe:

	Add new test_file_must_not_exist et al helpers for
	use by tests to more loudly diagnose failures that
	manifest themselves by the existence or nonexistence
	of a file or directory.

	So now you can use

		test_file_must_exist foo "so there"

	from your test, and when it fails due to foo being
	absent or being a symlink instead, instead of silence
	you will get (if debugging with "-v") the helpful message

		file foo does not exist. so there.

> +++ b/t/README
> @@ -467,6 +467,14 @@ library for your script to use.
>     <expected> file.  This behaves like "cmp" but produces more
>     helpful output when the test is run with "-v" option.
>  
> + - test_file_must_exist <file> [<diagnosis>]
> +   test_file_must_not_exist <file> [<diagnosis>]
> +   test_dir_must_exist <dir> [<diagnosis>]
> +   test_dir_must_not_exist <dir> [<diagnosis>]
> +
> +   check whether a file/directory exists or doesn't. <diagnosis> will
> +   be displayed if the test fails.

Maybe:

	- 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.

> +++ b/t/t3404-rebase-interactive.sh
> +++ b/t/t3407-rebase-abort.sh
[examples]

Makes sense.

> +++ b/t/test-lib.sh
> @@ -541,6 +541,38 @@ test_external_without_stderr () {
>  	fi
>  }
>  
> +# debugging-friendly alternatives to "test [!] [-f|-d]"
> +# The commands test the existence or non-existance of $1. $2 can be
> +# given to provide a more precise diagnosis.
> +test_file_must_exist () {
> +	if ! [ -f "$1" ]; then
> +		echo "file $1 doesn't exist. $*"
> +		false
> +	fi
> +}

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.

> +test_file_must_not_exist () {
> +	if [ -f "$1" ]; then
> +		echo "file $1 exists. $*"
> +		false
> +	fi
> +}

What should happen if $1 exists and is not a file?

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