diff mbox series

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

Message ID 20210224195129.4004-21-avarab@gmail.com (mailing list archive)
State New
Headers show
Series userdiff: refactor + test + doc + misc improvements | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 24, 2021, 7:51 p.m. UTC
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(-)

Comments

Johannes Sixt Feb. 28, 2021, 10:34 a.m. UTC | #1
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.

> @@ -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
>
Ævar Arnfjörð Bjarmason Feb. 28, 2021, 3:51 p.m. UTC | #2
On Sun, Feb 28 2021, Johannes Sixt wrote:

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

>> @@ -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
>>
diff mbox series

Patch

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 &&
@@ -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