Re: [PATCH v4 2/9] config tests: add "NULL" tests for *_get_value_multi()

> On Mon, Feb 06 2023, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> +test_NULL_in_multi () {
>>> +	local op="$1" &&
>>> +	local file="$2" &&
>>> +
>>> +	test_expect_success "$op: NULL value in config${file:+ in $file}" '
>>> +		config="$file" &&
>>> +		if test -z "$config"
>>> +		then
>>> +			config=.git/config &&
>>> +			test_when_finished "mv $config.old $config" &&
>>> +			mv "$config" "$config".old
>>> +		fi &&
>>> +
>>> +		cat >"$config" <<-\EOF &&
>>> +		[a]key=x
>>> +		[a]key
>>> +		[a]key=y
>>> +		EOF
>>> +		case "$op" in
>>> +		*_multi)
>>> +			cat >expect <<-\EOF
>>> +			x
>>> +			(NULL)
>>> +			y
>>> +			EOF
>>> +			;;
>>> +		*)
>>> +			cat >expect <<-\EOF
>>> +			y
>>> +			EOF
>>> +			;;
>>> +		esac &&
>>> +		test-tool config "$op" a.key $file >actual &&
>>> +		test_cmp expect actual
>>> +	'
>>> +}
>>> +
>>> +test_NULL_in_multi "get_value_multi"
>>> +test_NULL_in_multi "configset_get_value" "my.config"
>>> +test_NULL_in_multi "configset_get_value_multi" "my.config"
>>
>> I frankly preferred v3's tests over this version. v3 is slightly
>> verbose, but at least the lack of logic made it easy to read and
>> understand. I'd be okay with it if we get a big DRY-ness benefit, but 2
>> conditionals for 3 cases seems quite un-DRY to me.
>
> Note that the v3 version didn't test the get_value_multi(), adjusting
> the v3 version with copy/paste to test that as well is why I made this a
> function. From the CL's range-diff:
>
>     -    When the "t/t1308-config-set.sh" tests were added in [1] only one of
>     -    the three "(NULL)" lines in "t/helper/test-config.c" had any test
>     -    coverage. This change adds tests that stress the remaining two.
>     +    When parts of the config_set API were tested for in [1] they didn't
>     +    add coverage for 3/4 of the "(NULL)" cases handled in
>     +    "t/helper/test-config.c". We'd test that case for "get_value", but not
>     +    "get_value_multi", "configset_get_value" and
>     +    "configset_get_value_multi".
>     +
>     +    We now cover all of those cases, which in turn expose the details of
>     +    how this part of the config API works.
>
> Of course that wouldn't address an outstanding point that we should just
> copy/paste these anyway, but maybe that addresses your feedback...