Re: [PATCH v8 2/2] test-config: add tests for the config_set API

[Junio C Hamano <gitster@pobox.com>]

Tanay Abhra <tanayabh@gmail.com> writes:

> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> new file mode 100755
> index 0000000..87a29f1
> --- /dev/null
> +++ b/t/t1308-config-set.sh
> @@ -0,0 +1,170 @@
> +#!/bin/sh
> +
> +test_description='Test git config-set API in different settings'
> +
> +. ./test-lib.sh
> +
> +# 'check section.key value' verifies that the entry for section.key is
> +# 'value'
> +check() {

(style) SP around both sides of ().

More importantly, will we ever have different kind of check in this
script, perhaps checking if all values for a multi-valued variables
appear in the output, checking if get_bool works, etc. in the
future?  I would imagine the answer is yes, and in that case this
should be renamed to be a bit more specific (i.e. no "too generic"
helper called "check" would exist in the final result).  Perhaps
call it "check_single", "check_get_value" or "check_value" (the last
one assumes that all your future checks are mostly about various
forms of "get")?

> +	echo "$2" >expected &&
> +	test-config get_value "$1" >actual 2>&1 &&
> +	test_cmp expected actual
> +}
> +
> +test_expect_success 'setup default config' '
> +	cat >.git/config << EOF

 - No SP after redirection operator.

 - If you are not using variable substition in the here-doc, it is
   more friendly to quote the end-of-here-doc token to tell the
   reader that they do not have to worry about them.

 - There may be core.* variables that are crucial for correct
   operation of the version of Git being tested, so wiping and
   replacing .git/config wholesale is not a good idea.  Appending
   your config items is sufficient for the purpose of these tests.

i.e.

	cat >>.git/config <<\EOF
        ...
	EOF

> +	[core]

I'd feel safer if you did not abuse [core] like this.  All you care
about is the config parsing, and you do not want future versions of
Git introducing core.MixedCase to mean something meaningful that
affects how "git config" and other commands work.

> +		penguin = very blue
> +		Movie = BadPhysics
> +		UPPERCASE = true
> +		MixedCase = true
> +		my =
> ...
> +	EOF
> +'
> +
> +test_expect_success 'get value for a simple key' '
> +	check core.penguin "very blue"
> +'
> +
> +test_expect_success 'get value for a key with value as an empty string' '
> +	check core.my ""
> +'
> +
> +test_expect_success 'get value for a key with value as NULL' '
> +	check core.foo "(NULL)"
> +'
> +
> +test_expect_success 'upper case key' '
> +	check core.UPPERCASE "true"
> +'
> +
> +test_expect_success 'mixed case key' '
> +	check core.MixedCase "true"
> +'

You would also need to see how

	check core.uppercase
        check core.MIXEDCASE

behave (after moving them out of "core." hierarchy, of course), like
the following checks for case insensitivity of the first token.  The
first and the last token are both supposed to be case insensitive,
no?

> +test_expect_success 'key with case insensitive section header' '
> +	check cores.baz "ball" &&
> +	check Cores.baz "ball" &&
> +	check CORES.baz "ball" &&
> +	check coreS.baz "ball"
> +'
> +
> +test_expect_success 'find value with misspelled key' '
> +	echo "Value not found for \"my.fOo Bar.hi\"" >expect &&
> +	test_must_fail test-config get_value "my.fOo Bar.hi" >actual &&

Is test_must_fail suffice here?  git_config_get_value() has two
kinds of non-zero return values (one for "error", the other for "not
found").  Shouldn't test-config let the caller tell these two kinds
apart in some way?  It would be reasonable to do so with its exit
status, I would imagine, and in that case, test_expect_code may be
more appropriate here.

> +	test_cmp expect actual

Are you sure the expect and actual should match here?

Oh by the way, in "check()" helper shell function you spelled
the correct answer "expected" but here you use "expect" (like almost
all the other existing tests).  Perhaps s/expected/expect/ while we
fix the helper function?

> +'
> +
> +test_expect_success 'find value with the highest priority' '
> +	check core.baz "hask"
> +'
> +
> +test_expect_success 'find integer value for a key' '
> +	echo 65 >expect &&
> +	test-config get_int lamb.chop >actual &&
> +	test_cmp expect actual
> +'

Perhaps

	check_config () {
		op=$1 key=$2 &&
                shift &&
                shift &&
                printf "%s\n" "$@" >expect &&
                test-config "$op" "$key" >actual &&
		test_cmp expect actual
	}

and use it like so?

	check_config get_value core.mixedcase true
        check_config get_int lamb.chop 65
        check_config get_bool goat.head 1
        check_config get_value_multi core.baz sam bat hask

> +test_expect_success 'find integer if value is non parse-able' '
> +	echo 65 >expect &&
> +	test_must_fail test-config get_int lamb.head >actual &&
> +	test_must_fail test_cmp expect actual

Do not use test_must_fail on anything other than "git" command.
Worse yet, you are not just interested in seeing expect and actual
differ.  When get_int finds something that is not an integer, you do
not just expect the output from the command to be any random string
that is not the correct answer.  You expect it to be empty, no?

	>expect &&
	test_expect_code 1 test-config get_int lamb.head >actual &&
        test_cmp expect actual

or something (assuming that you chose to exit with 1 when you get an
error, but I didn't check).

Extending the check_config helper a bit more, perhaps

	check_config () {
		case "$1" in
                fail)
			>expect &&
                        test_expect_code 1 test-config "$2" "$3" >actual
                	;;
		*)
                	op=$1 key=$2 &&
                        shift &&
                        shift &&
	                printf "%s\n" "$@" >expect &&
	                test-config "$op" "$key" >actual
			;;
		esac &&
                test_cmp expect actual
	}

or something like that?

> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 0000000..dc313c2
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,125 @@
> +#include "cache.h"
> +#include "string-list.h"
> +
> +/*
> + * This program exposes the C API of the configuration mechanism
> + * as a set of simple commands in order to facilitate testing.
> + *
> + * Reads stdin and prints result of command to stdout:
> + *
> + * get_value -> prints the value with highest priority for the entered key
> + *
> + * get_value_multi -> prints all values for the entered key in increasing order
> + *		     of priority
> + *
> + * get_int -> print integer value for the entered key or die
> + *
> + * get_bool -> print bool value for the entered key or die
> + *
> + * configset_get_value -> returns value with the highest priority for the entered key
> + * 			from a config_set constructed from files entered as arguments.
> + *
> + * configset_get_value_multi -> returns value_list for the entered key sorted in
> + * 				ascending order of priority from a config_set
> + * 				constructed from files entered as arguments.
> + *
> + * Examples:
> + *
> + * To print the value with highest priority for key "foo.bAr Baz.rock":
> + * 	test-config get_value "foo.bAr Baz.rock"
> + *
> + */
> +
> +
> +int main(int argc, char **argv)
> +{
> +	int i, val;
> +	const char *v;
> +	const struct string_list *strptr;
> +	struct config_set cs;
> +	git_configset_init(&cs);
> +
> +	if (argc < 2) {
> +		fprintf(stderr, "Please, provide a command name on the command-line\n");
> +		return 1;
> +	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
> +		if (!git_config_get_value(argv[2], &v)) {
> +			if (!v)
> +				printf("(NULL)\n");

This one is dubious.  Is this for things like

	(in .git/config)
	[receive]
        	fsckobjects

and asking with

	$ git config receive.fsckobjects

which I think gives an empty string?  We may want to be consistent.

> +			else
> +				printf("%s\n", v);
> +			return 0;
> +		} else {
> +			printf("Value not found for \"%s\"\n", argv[2]);
> +			return 1;

So "not found" is signalled with exit code 1.  die() coming from
other errors will give us something like 128, and you exit with 2
when add-file fails (below), so the caller indeed can tell these
cases apart.

The tests that use test_must_fail in your test scripts should be
updated to use test_expect_code then.

> +		}
> +	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
> +...
> +	}
> +
> +	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
> +	return 1;

This is an error of different kind, no?  Use a different exit code
for it.  Instead of fprintf/return, using die() is fine here.

> +}