Re: [PATCH 1/2] unicode: kunit: refactor selftest to kunit tests

[Gabriel Krisman Bertazi <krisman@suse.de>]

Gabriela Bittencourt <gbittencourt@lkcamp.dev> writes:

>
> -static ssize_t utf8len(const struct unicode_map *um, enum utf8_normalization n,
> -		const char *s)
> +static ssize_t utf8len(const struct unicode_map *um, enum utf8_normalization n, const char *s)
>  {

Please, do not make indentation-only changes, specially as part of a larger
change.  It makes the patch much harder to review.


>  	return utf8nlen(um, n, s, (size_t)-1);
>  }
>  
>  static int utf8cursor(struct utf8cursor *u8c, const struct unicode_map *um,
> -		enum utf8_normalization n, const char *s)
> +		      enum utf8_normalization n, const char *s)

likewise.

>  {
>  	return utf8ncursor(u8c, um, n, s, (unsigned int)-1);
>  }
>  
> -static void check_utf8_nfdi(struct unicode_map *um)
> +static void check_utf8_nfdi(struct kunit *test)
>  {
>  	int i;
>  	struct utf8cursor u8c;
> +	struct unicode_map *um = test->priv;
>  
>  	for (i = 0; i < ARRAY_SIZE(nfdi_test_data); i++) {
>  		int len = strlen(nfdi_test_data[i].str);
> @@ -181,28 +161,29 @@ static void check_utf8_nfdi(struct unicode_map *um)
>  		int j = 0;
>  		unsigned char c;
>  
> -		test((utf8len(um, UTF8_NFDI, nfdi_test_data[i].str) == nlen));
> -		test((utf8nlen(um, UTF8_NFDI, nfdi_test_data[i].str, len) ==
> -			nlen));
> +		KUNIT_EXPECT_EQ(test, utf8len(um, UTF8_NFDI, nfdi_test_data[i].str), nlen);
> +		KUNIT_EXPECT_EQ(test, utf8nlen(um, UTF8_NFDI, nfdi_test_data[i].str, len),
> +				nlen);
> -		if (utf8cursor(&u8c, um, UTF8_NFDI, nfdi_test_data[i].str) < 0)
> -			pr_err("can't create cursor\n");
> +		KUNIT_EXPECT_GE_MSG(test, utf8cursor(&u8c, um, UTF8_NFDI, nfdi_test_data[i].str),
> +				    0, "Can't create cursor\n");

These KUNIT_ macros are way less readable than the existing code,
IMO. the old macro makes it obvious what we are checking, without having
to dig into the definition.  But, meh, I can live with it.



>  
>  		while ((c = utf8byte(&u8c)) > 0) {
> -			test_f((c == nfdi_test_data[i].dec[j]),
> -			       "Unexpected byte 0x%x should be 0x%x\n",
> -			       c, nfdi_test_data[i].dec[j]);
> +			KUNIT_EXPECT_EQ_MSG(test, c, nfdi_test_data[i].dec[j],
> +					    "Unexpected byte 0x%x should be 0x%x\n",
> +					    c, nfdi_test_data[i].dec[j]);
>  			j++;
>  		}
>  
> -		test((j == nlen));
> +		KUNIT_EXPECT_EQ(test, j, nlen);
>  	}
>  }
>  
> -static void check_utf8_nfdicf(struct unicode_map *um)
> +static void check_utf8_nfdicf(struct kunit *test)
>  {
>  	int i;
>  	struct utf8cursor u8c;
> +	struct unicode_map *um = test->priv;
>  
>  	for (i = 0; i < ARRAY_SIZE(nfdicf_test_data); i++) {
>  		int len = strlen(nfdicf_test_data[i].str);
> @@ -210,29 +191,30 @@ static void check_utf8_nfdicf(struct unicode_map *um)
>  		int j = 0;
>  		unsigned char c;
>  
> -		test((utf8len(um, UTF8_NFDICF, nfdicf_test_data[i].str) ==
> -				nlen));
> -		test((utf8nlen(um, UTF8_NFDICF, nfdicf_test_data[i].str, len) ==
> -				nlen));
> +		KUNIT_EXPECT_EQ(test, utf8len(um, UTF8_NFDICF, nfdicf_test_data[i].str),
> +				nlen);
> +		KUNIT_EXPECT_EQ(test, utf8nlen(um, UTF8_NFDICF, nfdicf_test_data[i].str, len),
> +				nlen);
>  
> -		if (utf8cursor(&u8c, um, UTF8_NFDICF,
> -				nfdicf_test_data[i].str) < 0)
> -			pr_err("can't create cursor\n");
> +		KUNIT_EXPECT_GE_MSG(test,
> +				    utf8cursor(&u8c, um, UTF8_NFDICF, nfdicf_test_data[i].str),
> +				    0, "Can't create cursor\n");
>  
>  		while ((c = utf8byte(&u8c)) > 0) {
> -			test_f((c == nfdicf_test_data[i].ncf[j]),
> -			       "Unexpected byte 0x%x should be 0x%x\n",
> -			       c, nfdicf_test_data[i].ncf[j]);
> +			KUNIT_EXPECT_EQ_MSG(test, c, nfdicf_test_data[i].ncf[j],
> +					    "Unexpected byte 0x%x should be 0x%x\n",
> +					    c, nfdicf_test_data[i].ncf[j]);
>  			j++;
>  		}
>  
> -		test((j == nlen));
> +		KUNIT_EXPECT_EQ(test, j, nlen);
>  	}
>  }
>  
> -static void check_utf8_comparisons(struct unicode_map *table)
> +static void check_utf8_comparisons(struct kunit *test)
>  {
>  	int i;
> +	struct unicode_map *um = test->priv;
>  
>  	for (i = 0; i < ARRAY_SIZE(nfdi_test_data); i++) {
>  		const struct qstr s1 = {.name = nfdi_test_data[i].str,
> @@ -240,8 +222,9 @@ static void check_utf8_comparisons(struct unicode_map *table)
>  		const struct qstr s2 = {.name = nfdi_test_data[i].dec,
>  					.len = sizeof(nfdi_test_data[i].dec)};
>  
> -		test_f(!utf8_strncmp(table, &s1, &s2),
> -		       "%s %s comparison mismatch\n", s1.name, s2.name);
> +		// strncmp returns 0 when strings are equal

We don't do comments with // in the kernel (aside from SPDX).  Please, use /* */.

> +		KUNIT_EXPECT_EQ_MSG(test, utf8_strncmp(um, &s1, &s2), 0,
> +				    "%s %s comparison mismatch\n", s1.name, s2.name);

Yuck. This is even less readable.  Is there a kunit macro that receives
an expression, similar to WARN_ON/BUG_ON? It would be way more readable.
Something like this.

KUNIT_BLA(test, utf8_strncmp(um, &s1,&s2) == 0, "BAD TEST!")


-- 
Gabriel Krisman Bertazi