Re: [PATCH v3 20/35] userdiff tests: assert that new built-in drivers have tests

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 28, 2021, 15:51 UTC
Message-ID: <878s78kx5s.fsf@evledraar.gmail.com>
In-Reply-To: <377d555c-6bd0-4206-3f71-d4d51aaca692@kdbg.org>

On Sun, Feb 28 2021, Johannes Sixt wrote:

Show 65 quoted lines

> Am 24.02.21 um 20:51 schrieb Ævar Arnfjörð Bjarmason:
>> Add an assertion to the userdiff test framework to check that
>> everything except a narrow whitelist of existing built-in patterns has
>> tests.
>> 
>> Since this test framework was added we've added new patterns without
>> any tests. Let's make it obvious in the future in the diff for such
>> patches that they should have those tests.
>> 
>> For anything with tests we can skip the "does the pattern compile?"
>> test, as the actual tests will check that for us.
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t4018-diff-funcname.sh | 30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>> 
>> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
>> index b80546b4d7f..a3058fda130 100755
>> --- a/t/t4018-diff-funcname.sh
>> +++ b/t/t4018-diff-funcname.sh
>> @@ -15,6 +15,19 @@ test_expect_success 'setup' '
>>  	sort <builtin-drivers >builtin-drivers.sorted &&
>>  	test_cmp builtin-drivers.sorted builtin-drivers &&
>>  
>> +	# Do not add anything to this list. New built-in drivers should have
>> +	# tests
>> +	cat >drivers-no-tests <<-\EOF &&
>> +	ada
>> +	bibtex
>> +	csharp
>> +	html
>> +	objc
>> +	pascal
>> +	ruby
>> +	tex
>> +	EOF
>> +
>>  	# for regexp compilation tests
>>  	echo A >A.java &&
>>  	echo B >B.java
>> @@ -22,7 +35,12 @@ test_expect_success 'setup' '
>>  
>>  for p in $(cat builtin-drivers)
>>  do
>> -	test_expect_success "builtin $p pattern compiles" '
>> +	P=$(echo $p | tr 'a-z' 'A-Z')
>> +	if grep -q $p drivers-no-tests
>> +	then
>> +		test_set_prereq NO_TEST_FOR_DRIVER_$P
>> +	fi
>> +	test_expect_success NO_TEST_FOR_DRIVER_$P "builtin $p pattern compiles" '
>>  		echo "*.java diff=$p" >.gitattributes &&
>>  		test_expect_code 1 git diff --no-index \
>>  			A.java B.java 2>msg &&
>
> Please drop this hunk. It is extremly distracting to see, e.g.,
>
> ok 8 # skip builtin cpp pattern compiles (missing NO_TEST_FOR_DRIVER_CPP)
> ok 9 - builtin cpp wordRegex pattern compiles
>
> It says "NO_TEST_FOR_DRIVER_CPP", but I know we have tests. I have to
> waste time to check that something not related to "we have tests for the
> driver" is meant here. It may be just a matter of naming the
> prerequisite, but I think we do not need this optimization.

Perhaps some other way to do this is better. I did the prerequisite just to avoid indenting that whole part and I figured it was more readable, but apparently not on the "more readable".

I do think it makes sense to keep this in some form, i.e. not test the compilation if we know we have later tests to compile the regex. It's providing self-documenting code to show that once we add tests for the few missing drivers we can delete that test entirely.

And if a later change does the same check on the t4034/* files the --word-diff check can also go.

Show 28 quoted lines

>> @@ -119,11 +137,17 @@ test_diff_funcname () {
>>  	'
>>  }
>>  
>> +>drivers-had-no-tests
>>  for what in $diffpatterns
>>  do
>>  	test="$TEST_DIRECTORY/t4018/$what.sh"
>>  	if ! test -e "$test"
>>  	then
>> +		git -C t4018 ls-files ':!*.sh' "$what*" >other-tests &&
>> +		if ! test -s other-tests
>> +		then
>> +			echo $what >>drivers-had-no-tests
>> +		fi
>>  		continue
>>  	fi &&
>>  
>> @@ -135,4 +159,8 @@ do
>>  	. "$test"
>>  done
>>  
>> +test_expect_success 'we should not have new built-in drivers without tests' '
>> +	test_cmp drivers-no-tests drivers-had-no-tests
>> +'
>> +
>>  test_done
>>