Re: [PATCH 1/3] kunit: Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros

From: "André Almeida" <andrealmeid@riseup.net>
To: "Maíra Canal" <mairacanal@riseup.net>
Cc: melissa.srw@gmail.com, jose.exposito89@gmail.com,
	kuba@kernel.org, siqueirajordao@riseup.net,
	Isabella Basso <isabbasso@riseup.net>,
	magalilemes00@gmail.com, airlied@linux.ie, davidgow@google.com,
	Brendan Higgins <brendanhiggins@google.com>,
	davem@davemloft.net, javierm@redhat.com,
	tales.aparecida@gmail.com, linux-kselftest@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org,
	daniel@ffwll.ch
Subject: Re: [PATCH 1/3] kunit: Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros
Date: Tue, 2 Aug 2022 13:17:59 -0300	[thread overview]
Message-ID: <951b07d6-c4cb-0b15-aa16-9347a861d985@riseup.net> (raw)
In-Reply-To: <20220802161206.228707-2-mairacanal@riseup.net>

Hi Maíra,

Thanks for the patch!

Às 13:12 de 02/08/22, Maíra Canal escreveu:
> Currently, in order to compare arrays in KUnit, the KUNIT_EXPECT_EQ or
> KUNIT_EXPECT_FALSE macros are used in conjunction with the memcmp
> function, such as:
>     KUNIT_EXPECT_EQ(test, memcmp(foo, bar, size), 0);
> 
> Although this usage produces correct results for the test cases, when
> the expectation fails, the error message is not very helpful,
> indicating only the return of the memcmp function.
> 
> Therefore, create a new set of macros KUNIT_EXPECT_ARREQ and
> KUNIT_EXPECT_ARRNEQ that compare memory blocks until a determined size.
> In case of expectation failure, those macros print the hex dump of the
> memory blocks, making it easier to debug test failures for arrays.
> 
> That said, the expectation
> 
>     KUNIT_EXPECT_EQ(test, memcmp(foo, bar, size), 0);
> 
> would translate to the expectation
> 
>     KUNIT_EXPECT_ARREQ(test, foo, bar, size);
> 
> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> ---
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index d00d6d181ee8..0b537a8690e0 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -204,3 +204,46 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>  	kunit_assert_print_msg(message, stream);
>  }
>  EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format);
> +
> +/* Adds a hexdump of a buffer to a string_stream */
> +static void kunit_assert_hexdump(struct string_stream *stream,
> +		const void *buf, const size_t len)
> +{
> +	const u8 *ptr = buf;
> +	int i, linelen, remaining = len;
> +	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> +
> +	for (i = 0; i < len; i += 16) {
> +		linelen = min(remaining, 16);
> +		remaining -= 16;
> +
> +		hex_dump_to_buffer(ptr + i, linelen, 16, 1, linebuf, sizeof(linebuf), false);
> +
> +		string_stream_add(stream, "%.8x: %s\n", i, linebuf);
> +	}
> +}
> +
> +void kunit_arr_assert_format(const struct kunit_assert *assert,
> +				    const struct va_format *message,
> +				    struct string_stream *stream)
> +{
> +	struct kunit_arr_assert *arr_assert;
> +
> +	arr_assert = container_of(assert, struct kunit_arr_assert,
> +				     assert);
> +
> +	string_stream_add(stream,
> +			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> +			  arr_assert->text->left_text,
> +			  arr_assert->text->operation,
> +			  arr_assert->text->right_text);
> +
> +	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \n", arr_assert->text->left_text);
> +	kunit_assert_hexdump(stream, arr_assert->left_value, arr_assert->size);

I think using `:` instead of `==` gives a better output here

  [12:38:20] dst:
  [12:38:20] 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12
  [12:38:20] 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
  [12:38:20] result->expected:
  [12:38:20] 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
  [12:38:20] 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00

> +
> +	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \n", arr_assert->text->right_text);
> +	kunit_assert_hexdump(stream, arr_assert->right_value, arr_assert->size);
> +
> +	kunit_assert_print_msg(message, stream);
> +}
> +EXPORT_SYMBOL_GPL(kunit_arr_assert_format);