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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 11, 2014, 18:18 UTC
Message-ID: <xmqq8unz4v4c.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1405049655-4265-3-git-send-email-tanayabh@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 15 quoted lines

> 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")?

Show 7 quoted lines

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

Show 28 quoted lines

> +		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?

Show 10 quoted lines

> +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?

Show 11 quoted lines

> +'
> +
> +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?

Show 54 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

> +}