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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 6, 2023, 12:31 UTC
Message-ID: <230206.86edr36ki2.gmgdl@evledraar.gmail.com>
In-Reply-To: <kl6lzg9rgjqv.fsf@chooglen-macbookpro.roam.corp.google.com>

On Mon, Feb 06 2023, Glen Choo wrote:

Show 47 quoted lines

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