Re: [PATCH v5 2/2] test-config: Add tests for the config_set API

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 6, 2014, 18:33 UTC
Message-ID: <vpqzjgm49s8.fsf@anie.imag.fr>
In-Reply-To: <1404631162-18556-3-git-send-email-tanayabh@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 5 quoted lines

> +test_expect_success 'get value for a simple key' '
> +	echo "very blue" >expect &&
> +	test-config get_value core.penguin >actual &&
> +	test_cmp expect actual
> +'

All these tests would greatly benefit from a helper like

test_expect_config () {
	echo "1" >expect &&
	test-config get_value "$2" >actual &&
	test_cmp expect actual
}

Then, all the 3-liners below would become 1-liners.

Should not block inclusion, but may be worth considering.

Show 11 quoted lines

> +test_expect_success 'get value for a key with value as an empty string' '
> +	echo "" >expect &&
> +	test-config get_value core.my >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get value for a key with value as NULL' '
> +	echo "(NULL)" >expect &&
> +	test-config get_value core.foo >actual &&
> +	test_cmp expect actual
> +'

[...]

Show 9 quoted lines

> +test_expect_success 'key with case sensitive subsection' '
> +	echo "mixed-case" >expect &&
> +	echo "upper-case" >>expect &&
> +	echo "lower-case" >>expect &&
> +	test-config get_value "my.Foo bAr.hi" >actual &&
> +	test-config get_value "my.FOO BAR.hi" >>actual &&
> +	test-config get_value "my.foo bar.hi" >>actual &&
> +	test_cmp expect actual
> +'

This would become a 3-liner with my helper.

Show 9 quoted lines

> +test_expect_success 'key with case insensitive section header' '
> +	echo "ball" >expect &&
> +	echo "ball" >>expect &&
> +	echo "ball" >>expect &&
> +	test-config get_value cores.baz >actual &&
> +	test-config get_value Cores.baz >>actual &&
> +	test-config get_value CORES.baz >>actual &&
> +	test_cmp expect actual
> +'

I think you miss a simple case: get_value with a case that doesn't exist in the config file, like "get_value coreS.baz".

> +test_expect_success 'find value with the highest priority' '
> +	echo hask >expect &&
> +	test-config get_value "core.baz">actual &&

Space before >.

> diff --git a/test-config.c b/test-config.c

No time for a real review of this file, but from a quick look, it seems OK.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/