[PATCH 0/3] Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros

[PATCH 0/3] Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros

[Maíra Canal]

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

For example, if I am using the KUNIT_EXPECT_ARREQ macro and apply the
following diff (introducing a test failure) to the 
drm/tests/drm_format_helper.c:

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 3106abb3bead..942aa131a768 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -131,9 +131,9 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
                    .rgb565_result = {
                        .dst_pitch = 10,
                        .expected = {
-                               0x0A33, 0x1260, 0xA800, 0x0000, 0x0000,
-                               0x6B8E, 0x0A33, 0x1260, 0x0000, 0x0000,
-                               0xA800, 0x6B8E, 0x0A33, 0x0000, 0x0000,
+                               0x0A31, 0x1260, 0xA800, 0x0000, 0x0000,
+                               0x6B81, 0x0A33, 0x1260, 0x0000, 0x0000,
+                               0xA801, 0x6B8E, 0x0A33, 0x0000, 0x0000,
                        },
                        .expected_swab = {
                                0x330A, 0x6012, 0x00A8, 0x0000, 0x0000,}}}

I will get a test failure with the following form:

➜ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y --kconfig_add CONFIG_VIRTIO_UML=y \
  'drm_format_helper_test'
  [...]
  [12:38:20] ================= xrgb8888_to_rgb565_test ==================
  [12:38:20] [PASSED] single_pixel_source_buffer
  [12:38:20] [PASSED] single_pixel_clip_rectangle
  [12:38:20] [PASSED] well_known_colors
  [12:38:20] # xrgb8888_to_rgb565_test: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_format_helper_test.c:248
  [12:38:20] Expected dst == result->expected, but
  [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
  [12:38:20] not ok 4 - destination_pitch
  [12:38:20] [FAILED] destination_pitch
  [12:38:20] # Subtest: xrgb8888_to_rgb565_test
  [12:38:20] # xrgb8888_to_rgb565_test: pass:3 fail:1 skip:0 total:4
  [12:38:20] not ok 2 - xrgb8888_to_rgb565_test
  [...]
  [12:38:20] ============= [FAILED] drm_format_helper_test ==============
  [12:38:20] ============================================================
  [12:38:20] Testing complete. Ran 8 tests: passed: 7, failed: 1
  [12:38:20] Elapsed time: 3.713s total, 0.002s configuring, 3.546s building, 0.135s running

Noticed that, with the hex dump, it is possible to check which bytes are
making the test fail. So, it is easier to debug the cause of the failure.

The first patch of the series introduces the KUNIT_EXPECT_ARREQ and
KUNIT_EXPECT_ARRNEQ. The second patch adds an example of array expectations
on the kunit-example-test.c. And the last patch replaces the KUNIT_EXPECT_EQ
for KUNIT_EXPECT_ARREQ on the existing occurrences.

Best Regards,
- Maíra Canal

Maíra Canal (3):
  kunit: Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros
  kunit: add KUnit array assertions to the example_all_expect_macros_test
  kunit: use KUNIT_EXPECT_ARREQ macro

 .../gpu/drm/tests/drm_format_helper_test.c    |  6 +-
 include/kunit/assert.h                        | 35 +++++++++
 include/kunit/test.h                          | 76 +++++++++++++++++++
 lib/kunit/assert.c                            | 43 +++++++++++
 lib/kunit/kunit-example-test.c                |  7 ++
 net/core/dev_addr_lists_test.c                |  4 +-
 6 files changed, 166 insertions(+), 5 deletions(-)

-- 
2.37.1

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

[Maíra Canal]

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>
---
 include/kunit/assert.h | 35 +++++++++++++++++++
 include/kunit/test.h   | 76 ++++++++++++++++++++++++++++++++++++++++++
 lib/kunit/assert.c     | 43 ++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index 4b52e12c2ae8..b8fac8eec0af 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -256,4 +256,39 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
 				    const struct va_format *message,
 				    struct string_stream *stream);
 
+
+#define KUNIT_INIT_ARR_ASSERT_STRUCT(text_, left_val, right_val, size_) \
+	{                                                                 \
+		.assert = { .format = kunit_arr_assert_format },   \
+		.text = text_,                                            \
+		.left_value = left_val,                                   \
+		.right_value = right_val, .size = size_,                  \
+	}
+
+/**
+ * struct kunit_arr_assert - An expectation/assertion that compares two
+ *	memory blocks.
+ * @assert: The parent of this type.
+ * @text: Holds the textual representations of the operands and comparator.
+ * @left_value: The actual evaluated value of the expression in the left slot.
+ * @right_value: The actual evaluated value of the expression in the right slot.
+ * @size: Size of the memory block analysed in bytes.
+ *
+ * Represents an expectation/assertion that compares two memory blocks. For
+ * example, to expect that the first three bytes of foo is equal to the
+ * first three bytes of bar, you can use the expectation
+ * KUNIT_EXPECT_ARREQ(test, foo, bar, 3);
+ */
+struct kunit_arr_assert {
+	struct kunit_assert assert;
+	const struct kunit_binary_assert_text *text;
+	const void *left_value;
+	const void *right_value;
+	const size_t size;
+};
+
+void kunit_arr_assert_format(const struct kunit_assert *assert,
+				    const struct va_format *message,
+				    struct string_stream *stream);
+
 #endif /*  _KUNIT_ASSERT_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8ffcd7de9607..30547fc57c1e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -684,6 +684,36 @@ do {									       \
 			##__VA_ARGS__);					       \
 } while (0)
 
+#define KUNIT_ARRAY_ASSERTION(test,				       \
+				   assert_type,				       \
+				   left,				       \
+				   op,					       \
+				   right,				       \
+				   size,                       \
+				   fmt,					       \
+				   ...)					       \
+do {									       \
+	const void *__left = (left);				       \
+	const void *__right = (right);				       \
+	const size_t __size = (size);					       \
+	static const struct kunit_binary_assert_text __text = {		       \
+		.operation = #op,					       \
+		.left_text = #left,					       \
+		.right_text = #right,					       \
+	};								       \
+									       \
+	KUNIT_ASSERTION(test,						       \
+			assert_type,					       \
+			memcmp(__left, __right, __size) op 0,			       \
+			kunit_arr_assert,			       \
+			KUNIT_INIT_ARR_ASSERT_STRUCT(&__text,	    \
+							__left,		       \
+							__right,           \
+							__size),	       \
+			fmt,						       \
+			##__VA_ARGS__);					       \
+} while (0)
+
 #define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test,			       \
 						assert_type,		       \
 						ptr,			       \
@@ -952,6 +982,52 @@ do {									       \
 				   fmt,					       \
 				   ##__VA_ARGS__)
 
+/**
+ * KUNIT_EXPECT_ARREQ() - Expects that the first @size bytes of @left and @right are equal.
+ * @test: The test context object.
+ * @left: An arbitrary expression that evaluates to a determinated size.
+ * @right: An arbitrary expression that evaluates to a determinated size.
+ * @size: Number of bytes compared.
+ *
+ * Sets an expectation that the values that @left and @right evaluate to are
+ * equal. This is semantically equivalent to
+ * KUNIT_EXPECT_TRUE(@test, !memcmp((@left), (@right), (@size))). See
+ * KUNIT_EXPECT_TRUE() for more information.
+ */
+#define KUNIT_EXPECT_ARREQ(test, left, right, size) \
+	KUNIT_EXPECT_ARREQ_MSG(test, left, right, size, NULL)
+
+#define KUNIT_EXPECT_ARREQ_MSG(test, left, right, size, fmt, ...)		       \
+	KUNIT_ARRAY_ASSERTION(test,				       \
+				   KUNIT_EXPECTATION,			       \
+				   left, ==, right,			       \
+				   size,                       \
+				   fmt,					   \
+				   ##__VA_ARGS__)
+
+/**
+ * KUNIT_EXPECT_STRNEQ() - Expects that the first @size bytes of @left and @right are not equal.
+ * @test: The test context object.
+ * @left: An arbitrary expression that evaluates to a determinated size.
+ * @right: An arbitrary expression that evaluates to a determinated size.
+ * @size: Number of bytes compared.
+ *
+ * Sets an expectation that the values that @left and @right evaluate to are
+ * not equal. This is semantically equivalent to
+ * KUNIT_EXPECT_TRUE(@test, memcmp((@left), (@right), (@size))). See
+ * KUNIT_EXPECT_TRUE() for more information.
+ */
+#define KUNIT_EXPECT_ARRNEQ(test, left, right, size) \
+	KUNIT_EXPECT_ARRNEQ_MSG(test, left, right, size, NULL)
+
+#define KUNIT_EXPECT_ARRNEQ_MSG(test, left, right, size, fmt, ...)		       \
+	KUNIT_ARRAY_ASSERTION(test,				       \
+				   KUNIT_EXPECTATION,			       \
+				   left, !=, right,			       \
+				   size,                       \
+				   fmt,					   \
+				   ##__VA_ARGS__)
+
 /**
  * KUNIT_EXPECT_NULL() - Expects that @ptr is null.
  * @test: The test context object.
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);
+
+	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);
-- 
2.37.1

[PATCH 2/3] kunit: add KUnit array assertions to the example_all_expect_macros_test

[Maíra Canal]

Increament the example_all_expect_macros_test with the
KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros by creating a test
with array assertions.

Signed-off-by: Maíra Canal <mairacanal@riseup.net>
---
 lib/kunit/kunit-example-test.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index f8fe582c9e36..fc81a45d9cbc 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -86,6 +86,9 @@ static void example_mark_skipped_test(struct kunit *test)
  */
 static void example_all_expect_macros_test(struct kunit *test)
 {
+	const u32 array[] = { 0x0F, 0xFF };
+	const u32 expected[] = { 0x1F, 0xFF };
+
 	/* Boolean assertions */
 	KUNIT_EXPECT_TRUE(test, true);
 	KUNIT_EXPECT_FALSE(test, false);
@@ -109,6 +112,10 @@ static void example_all_expect_macros_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, "hi", "hi");
 	KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
 
+	/* Array assertions */
+	KUNIT_EXPECT_ARREQ(test, expected, expected, 2);
+	KUNIT_EXPECT_ARRNEQ(test, array, expected, 2);
+
 	/*
 	 * There are also ASSERT variants of all of the above that abort test
 	 * execution if they fail. Useful for memory allocations, etc.
-- 
2.37.1

[PATCH 3/3] kunit: use KUNIT_EXPECT_ARREQ macro

[Maíra Canal]

Use KUNIT_EXPECT_ARREQ to compare memory blocks in replacement of the
KUNIT_EXPECT_EQ macro. Therefor, the statement

    KUNIT_EXPECT_EQ(test, memcmp(foo, bar, size), 0);

is replaced by:

    KUNIT_EXPECT_ARREQ(test, foo, bar, size);

Signed-off-by: Maíra Canal <mairacanal@riseup.net>
---
 drivers/gpu/drm/tests/drm_format_helper_test.c | 6 +++---
 net/core/dev_addr_lists_test.c                 | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 26ecf3b4b137..3106abb3bead 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -217,7 +217,7 @@ static void xrgb8888_to_rgb332_test(struct kunit *test)
 
 	drm_fb_xrgb8888_to_rgb332(dst, result->dst_pitch, src, &fb,
 				  &params->clip);
-	KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
+	KUNIT_EXPECT_ARREQ(test, dst, result->expected, dst_size);
 }
 
 static void xrgb8888_to_rgb565_test(struct kunit *test)
@@ -245,11 +245,11 @@ static void xrgb8888_to_rgb565_test(struct kunit *test)
 
 	drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, src, &fb,
 				  &params->clip, false);
-	KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
+	KUNIT_EXPECT_ARREQ(test, dst, result->expected, dst_size);
 
 	drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, src, &fb,
 				  &params->clip, true);
-	KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected_swab, dst_size), 0);
+	KUNIT_EXPECT_ARREQ(test, dst, result->expected_swab, dst_size);
 }
 
 static struct kunit_case drm_format_helper_test_cases[] = {
diff --git a/net/core/dev_addr_lists_test.c b/net/core/dev_addr_lists_test.c
index 049cfbc58aa9..d79f517c08f6 100644
--- a/net/core/dev_addr_lists_test.c
+++ b/net/core/dev_addr_lists_test.c
@@ -71,11 +71,11 @@ static void dev_addr_test_basic(struct kunit *test)
 
 	memset(addr, 2, sizeof(addr));
 	eth_hw_addr_set(netdev, addr);
-	KUNIT_EXPECT_EQ(test, 0, memcmp(netdev->dev_addr, addr, sizeof(addr)));
+	KUNIT_EXPECT_ARREQ(test, netdev->dev_addr, addr, sizeof(addr));
 
 	memset(addr, 3, sizeof(addr));
 	dev_addr_set(netdev, addr);
-	KUNIT_EXPECT_EQ(test, 0, memcmp(netdev->dev_addr, addr, sizeof(addr)));
+	KUNIT_EXPECT_ARREQ(test, netdev->dev_addr, addr, sizeof(addr));
 }
 
 static void dev_addr_test_sync_one(struct kunit *test)
-- 
2.37.1

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

[André Almeida]

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);

Re: [PATCH 2/3] kunit: add KUnit array assertions to the example_all_expect_macros_test

[André Almeida]

Às 13:12 de 02/08/22, Maíra Canal escreveu:
> Increament the example_all_expect_macros_test with the
> KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros by creating a test
> with array assertions.
> 
> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> ---
>  lib/kunit/kunit-example-test.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index f8fe582c9e36..fc81a45d9cbc 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -86,6 +86,9 @@ static void example_mark_skipped_test(struct kunit *test)
>   */
>  static void example_all_expect_macros_test(struct kunit *test)
>  {
> +	const u32 array[] = { 0x0F, 0xFF };
> +	const u32 expected[] = { 0x1F, 0xFF };
> +
>  	/* Boolean assertions */
>  	KUNIT_EXPECT_TRUE(test, true);
>  	KUNIT_EXPECT_FALSE(test, false);
> @@ -109,6 +112,10 @@ static void example_all_expect_macros_test(struct kunit *test)
>  	KUNIT_EXPECT_STREQ(test, "hi", "hi");
>  	KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
>  
> +	/* Array assertions */
> +	KUNIT_EXPECT_ARREQ(test, expected, expected, 2);
> +	KUNIT_EXPECT_ARRNEQ(test, array, expected, 2);

ARRAY_SIZE() is usually better than constants is this case.

> +
>  	/*
>  	 * There are also ASSERT variants of all of the above that abort test
>  	 * execution if they fail. Useful for memory allocations, etc.

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

[Daniel Latypov]

On Tue, Aug 2, 2022 at 9:12 AM Maíra Canal <mairacanal@riseup.net> wrote:
>
> 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, if 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.

I totally agree with this.

The only reason I hadn't sent an RFC out for this so far is
* we didn't have enough use cases quite yet (now resolved)
* I wasn't sure how we'd want to format the failure message.

For the latter, right now this series produces
dst ==
00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12
00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
result->expected ==
00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00

I was thinking something like what KASAN produces would be nice, e.g.
from https://www.kernel.org/doc/html/v5.19/dev-tools/kasan.html#error-reports
(I'll paste the bit here, but my email client doesn't support
monospaced fonts, so it won't look nice on my end)

Memory state around the buggy address:
 ffff8801f44ec200: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
 ffff8801f44ec280: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>ffff8801f44ec300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03
                                                                ^
I just wasn't quite sure how to do it for a diff, since this only
really works well when showing one bad byte.
If we blindly followed that approach, we get

dst ==
>00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12
                                          ^
>00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
                        ^
result->expected ==
>00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
                                          ^
>00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00
                        ^

But perhaps we could instead highlight the bad bytes with something like
dst ==
00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12
00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00
result->expected ==
00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12
00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00

Thoughts, suggestions?

Re: [PATCH 2/3] kunit: add KUnit array assertions to the example_all_expect_macros_test

[Daniel Latypov]

On Tue, Aug 2, 2022 at 9:19 AM André Almeida <andrealmeid@riseup.net> wrote:
> Às 13:12 de 02/08/22, Maíra Canal escreveu:
> > Increament the example_all_expect_macros_test with the
> > KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros by creating a test
> > with array assertions.
> >
> > Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> > ---
> >  lib/kunit/kunit-example-test.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> > index f8fe582c9e36..fc81a45d9cbc 100644
> > --- a/lib/kunit/kunit-example-test.c
> > +++ b/lib/kunit/kunit-example-test.c
> > @@ -86,6 +86,9 @@ static void example_mark_skipped_test(struct kunit *test)
> >   */
> >  static void example_all_expect_macros_test(struct kunit *test)
> >  {
> > +     const u32 array[] = { 0x0F, 0xFF };
> > +     const u32 expected[] = { 0x1F, 0xFF };

Given the distance between the definition and their use, perhaps we
can give them clearer names.
E.g. array + diff_array, or array1 + array2, etc.

I think something to indicate they're arrays and that they're different.
The current name `expected` is a bit unclear.

> > +
> >       /* Boolean assertions */
> >       KUNIT_EXPECT_TRUE(test, true);
> >       KUNIT_EXPECT_FALSE(test, false);
> > @@ -109,6 +112,10 @@ static void example_all_expect_macros_test(struct kunit *test)
> >       KUNIT_EXPECT_STREQ(test, "hi", "hi");
> >       KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
> >
> > +     /* Array assertions */
> > +     KUNIT_EXPECT_ARREQ(test, expected, expected, 2);
> > +     KUNIT_EXPECT_ARRNEQ(test, array, expected, 2);
>
> ARRAY_SIZE() is usually better than constants is this case.

Note: that's actually incorrect!

Ah right, this was the other blocker I had in mind.
I wasn't sure how we'd handle the size parameter.

Users might think ARRAY_SIZE() is fine and copy-paste it.
But the size parameter is in units of bytes, not array elements!
If the element types are not 1 byte, it'll silently not compare the full array.

We'd want people to use
KUNIT_EXPECT_ARREQ(test, expected, expected, sizeof(expected));

But this doesn't work for `u32 *array`, since it'll silently just
compare 1 byte if people get them mixed up.

I don't know how we make a maximally fool-proof version of this macro :\

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

[Daniel Latypov]

On Tue, Aug 2, 2022 at 9:12 AM Maíra Canal <mairacanal@riseup.net> wrote:
>
> 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>
> ---
>  include/kunit/assert.h | 35 +++++++++++++++++++
>  include/kunit/test.h   | 76 ++++++++++++++++++++++++++++++++++++++++++
>  lib/kunit/assert.c     | 43 ++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index 4b52e12c2ae8..b8fac8eec0af 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -256,4 +256,39 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>                                     const struct va_format *message,
>                                     struct string_stream *stream);
>
> +
> +#define KUNIT_INIT_ARR_ASSERT_STRUCT(text_, left_val, right_val, size_) \
> +       {                                                                 \
> +               .assert = { .format = kunit_arr_assert_format },   \
> +               .text = text_,                                            \
> +               .left_value = left_val,                                   \
> +               .right_value = right_val, .size = size_,                  \
> +       }

FYI, I have an RFC series out to simplify assertions a bit more.
https://lore.kernel.org/linux-kselftest/20220525154442.1438081-4-dlatypov@google.com/
in particular eliminates these INIT_STRUCT macros.

That series would break the Rust for Linux one, so I've been waiting
to see how that plays out.
At this point, this series might go in before my RFC one, so I'll
likely rebase on top of yours.

But if not, I can provide a diff to help rebase this series on top of
mine at that time.

> +
> +/**
> + * struct kunit_arr_assert - An expectation/assertion that compares two
> + *     memory blocks.
> + * @assert: The parent of this type.
> + * @text: Holds the textual representations of the operands and comparator.
> + * @left_value: The actual evaluated value of the expression in the left slot.
> + * @right_value: The actual evaluated value of the expression in the right slot.
> + * @size: Size of the memory block analysed in bytes.
> + *
> + * Represents an expectation/assertion that compares two memory blocks. For
> + * example, to expect that the first three bytes of foo is equal to the
> + * first three bytes of bar, you can use the expectation
> + * KUNIT_EXPECT_ARREQ(test, foo, bar, 3);
> + */
> +struct kunit_arr_assert {
> +       struct kunit_assert assert;
> +       const struct kunit_binary_assert_text *text;
> +       const void *left_value;
> +       const void *right_value;
> +       const size_t size;
> +};
> +
> +void kunit_arr_assert_format(const struct kunit_assert *assert,
> +                                   const struct va_format *message,
> +                                   struct string_stream *stream);
> +
>  #endif /*  _KUNIT_ASSERT_H */
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 8ffcd7de9607..30547fc57c1e 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -684,6 +684,36 @@ do {                                                                              \
>                         ##__VA_ARGS__);                                        \
>  } while (0)
>
> +#define KUNIT_ARRAY_ASSERTION(test,                                   \
> +                                  assert_type,                                \
> +                                  left,                                       \
> +                                  op,                                         \
> +                                  right,                                      \
> +                                  size,                       \
> +                                  fmt,                                        \
> +                                  ...)                                        \
> +do {                                                                          \
> +       const void *__left = (left);                                   \
> +       const void *__right = (right);                                 \
> +       const size_t __size = (size);                                          \
> +       static const struct kunit_binary_assert_text __text = {                \
> +               .operation = #op,                                              \
> +               .left_text = #left,                                            \
> +               .right_text = #right,                                          \
> +       };                                                                     \
> +                                                                              \
> +       KUNIT_ASSERTION(test,                                                  \
> +                       assert_type,                                           \
> +                       memcmp(__left, __right, __size) op 0,                          \
> +                       kunit_arr_assert,                              \
> +                       KUNIT_INIT_ARR_ASSERT_STRUCT(&__text,       \
> +                                                       __left,                \
> +                                                       __right,           \
> +                                                       __size),               \
> +                       fmt,                                                   \
> +                       ##__VA_ARGS__);                                        \
> +} while (0)
> +
>  #define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test,                         \
>                                                 assert_type,                   \
>                                                 ptr,                           \
> @@ -952,6 +982,52 @@ do {                                                                              \
>                                    fmt,                                        \
>                                    ##__VA_ARGS__)
>
> +/**
> + * KUNIT_EXPECT_ARREQ() - Expects that the first @size bytes of @left and @right are equal.
> + * @test: The test context object.
> + * @left: An arbitrary expression that evaluates to a determinated size.

nit: "determinated" isn't a word, though it would make sense as one.
Perhaps instead:
  to the specified size
  to the specified @size
  to a predetermined size

> + * @right: An arbitrary expression that evaluates to a determinated size.
> + * @size: Number of bytes compared.


As noted on patch 2/3, this is very subtle.
The fact it's in "bytes" and not "array elements" can mix people up
who would likely assume ARRAY_SIZE() would be appropriate.

Should we perhaps internally do
  size_bytes = (size) * sizeof((left)[0])
so users can just deal with # of array elements and not bytes?

> + *
> + * Sets an expectation that the values that @left and @right evaluate to are
> + * equal. This is semantically equivalent to
> + * KUNIT_EXPECT_TRUE(@test, !memcmp((@left), (@right), (@size))). See
> + * KUNIT_EXPECT_TRUE() for more information.
> + */
> +#define KUNIT_EXPECT_ARREQ(test, left, right, size) \
> +       KUNIT_EXPECT_ARREQ_MSG(test, left, right, size, NULL)
> +
> +#define KUNIT_EXPECT_ARREQ_MSG(test, left, right, size, fmt, ...)                     \
> +       KUNIT_ARRAY_ASSERTION(test,                                    \
> +                                  KUNIT_EXPECTATION,                          \
> +                                  left, ==, right,                            \
> +                                  size,                       \
> +                                  fmt,                                    \
> +                                  ##__VA_ARGS__)
> +
> +/**
> + * KUNIT_EXPECT_STRNEQ() - Expects that the first @size bytes of @left and @right are not equal.

nit: s/STR/ARR

> + * @test: The test context object.
> + * @left: An arbitrary expression that evaluates to a determinated size.
> + * @right: An arbitrary expression that evaluates to a determinated size.
> + * @size: Number of bytes compared.
> + *
> + * Sets an expectation that the values that @left and @right evaluate to are
> + * not equal. This is semantically equivalent to
> + * KUNIT_EXPECT_TRUE(@test, memcmp((@left), (@right), (@size))). See
> + * KUNIT_EXPECT_TRUE() for more information.
> + */
> +#define KUNIT_EXPECT_ARRNEQ(test, left, right, size) \
> +       KUNIT_EXPECT_ARRNEQ_MSG(test, left, right, size, NULL)
> +
> +#define KUNIT_EXPECT_ARRNEQ_MSG(test, left, right, size, fmt, ...)                    \
> +       KUNIT_ARRAY_ASSERTION(test,                                    \
> +                                  KUNIT_EXPECTATION,                          \
> +                                  left, !=, right,                            \
> +                                  size,                       \
> +                                  fmt,                                    \
> +                                  ##__VA_ARGS__)
> +
>  /**
>   * KUNIT_EXPECT_NULL() - Expects that @ptr is null.
>   * @test: The test context object.
> 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);
> +       }
> +}

As noted on the cover letter, I think we probably want to have our
output make it easier to spot the differing bytes if possible.
It's sufficiently annoying that I hadn't bothered to do it, so perhaps
we can keep it simple like this for now and revisit it later.

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

[Maíra Canal]

On 8/2/22 13:59, 'Daniel Latypov' via KUnit Development wrote:
> On Tue, Aug 2, 2022 at 9:12 AM Maíra Canal <mairacanal@riseup.net> wrote:
>>
>> 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, if 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.
> 
> I totally agree with this.
> 
> The only reason I hadn't sent an RFC out for this so far is
> * we didn't have enough use cases quite yet (now resolved)
> * I wasn't sure how we'd want to format the failure message.
> 
> For the latter, right now this series produces
> dst ==
> 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12
> 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
> result->expected ==
> 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
> 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00
> 
> I was thinking something like what KASAN produces would be nice, e.g.
> from https://www.kernel.org/doc/html/v5.19/dev-tools/kasan.html#error-reports
> (I'll paste the bit here, but my email client doesn't support
> monospaced fonts, so it won't look nice on my end)
> 
> Memory state around the buggy address:
>  ffff8801f44ec200: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>  ffff8801f44ec280: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>> ffff8801f44ec300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03
>                                                                 ^
> I just wasn't quite sure how to do it for a diff, since this only
> really works well when showing one bad byte.
> If we blindly followed that approach, we get
> 
> dst ==
>> 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12
>                                           ^
>> 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
>                         ^
> result->expected ==
>> 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
>                                           ^
>> 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00
>                         ^
> 
> But perhaps we could instead highlight the bad bytes with something like
> dst ==
> 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12
> 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00
> result->expected ==
> 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12
> 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00

My problem with this approach is that the bytes get slightly misaligned
when adding the <>. Maybe if we aligned as:

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

Although I don't know exactly how we can produce this output. I was
using hex_dump_to_buffer to produce the hexdump, so maybe I need to
change the strategy to generate the hexdump.

I guess the KASAN approach could be easier to implement. But I guess it
can turn out to be a little polluted if many bytes differ. For example:

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

I don't know exactly with option I lean.


Thank you for your inputs, Daniel!

- Maíra Canal

> 
> Thoughts, suggestions?
> 

Re: [PATCH 2/3] kunit: add KUnit array assertions to the example_all_expect_macros_test

[Maíra Canal]

On 8/2/22 15:15, 'Daniel Latypov' via KUnit Development wrote:
> On Tue, Aug 2, 2022 at 9:19 AM André Almeida <andrealmeid@riseup.net> wrote:
>> Às 13:12 de 02/08/22, Maíra Canal escreveu:
>>> Increament the example_all_expect_macros_test with the
>>> KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros by creating a test
>>> with array assertions.
>>>
>>> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
>>> ---
>>>  lib/kunit/kunit-example-test.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
>>> index f8fe582c9e36..fc81a45d9cbc 100644
>>> --- a/lib/kunit/kunit-example-test.c
>>> +++ b/lib/kunit/kunit-example-test.c
>>> @@ -86,6 +86,9 @@ static void example_mark_skipped_test(struct kunit *test)
>>>   */
>>>  static void example_all_expect_macros_test(struct kunit *test)
>>>  {
>>> +     const u32 array[] = { 0x0F, 0xFF };
>>> +     const u32 expected[] = { 0x1F, 0xFF };
> 
> Given the distance between the definition and their use, perhaps we
> can give them clearer names.
> E.g. array + diff_array, or array1 + array2, etc.
> 
> I think something to indicate they're arrays and that they're different.
> The current name `expected` is a bit unclear.

Thank you for the note, I'll address it at v2.

> 
>>> +
>>>       /* Boolean assertions */
>>>       KUNIT_EXPECT_TRUE(test, true);
>>>       KUNIT_EXPECT_FALSE(test, false);
>>> @@ -109,6 +112,10 @@ static void example_all_expect_macros_test(struct kunit *test)
>>>       KUNIT_EXPECT_STREQ(test, "hi", "hi");
>>>       KUNIT_EXPECT_STRNEQ(test, "hi", "bye");
>>>
>>> +     /* Array assertions */
>>> +     KUNIT_EXPECT_ARREQ(test, expected, expected, 2);
>>> +     KUNIT_EXPECT_ARRNEQ(test, array, expected, 2);
>>
>> ARRAY_SIZE() is usually better than constants is this case.
> 
> Note: that's actually incorrect!
> 

Yep, that's my bad!

> Ah right, this was the other blocker I had in mind.
> I wasn't sure how we'd handle the size parameter.
> 
> Users might think ARRAY_SIZE() is fine and copy-paste it.
> But the size parameter is in units of bytes, not array elements!
> If the element types are not 1 byte, it'll silently not compare the full array.
> 
> We'd want people to use
> KUNIT_EXPECT_ARREQ(test, expected, expected, sizeof(expected));
> 
> But this doesn't work for `u32 *array`, since it'll silently just
> compare 1 byte if people get them mixed up.
> 
> I don't know how we make a maximally fool-proof version of this macro :\

This is a hard one also. I believe that use KUNIT_EXPECT_ARREQ(test,
expected, expected, sizeof(expected)); is more compliant to the
memcpy/memset/memcmp signature. Moreover, this problem also occur for
the KUNIT_EXPECT_EQ(test, memcmp(expected, expected, sizeof(expected)), 0);

I believe that the number of array elements will make it easier for
users to avoid mistakes.

I'll change it internally for size_bytes = (size) * sizeof((left)[0]) on v2.

Best Regards,
- Maíra Canal

> 

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

[Daniel Latypov]

On Tue, Aug 2, 2022 at 11:43 AM Maíra Canal <mairacanal@riseup.net> wrote:
> > But perhaps we could instead highlight the bad bytes with something like
> > dst ==
> > 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12
> > 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00
> > result->expected ==
> > 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12
> > 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00
>
> My problem with this approach is that the bytes get slightly misaligned
> when adding the <>. Maybe if we aligned as:
>
> dst:
> 00000000: <33> 0a 60 12  00  a8 00 00 00 00 <8e> 6b 33 0a 60 12
> 00000010:  00  00 00 00 <00> a8 8e 6b 33 0a  00  00 00 00
> result->expected:
> 00000000: <31> 0a 60 12  00  a8 00 00 00 00 <81> 6b 33 0a 60 12
> 00000010:  00  00 00 00 <01> a8 8e 6b 33 0a  00  00 00 00

And yes, that's a good point re alignment. Handling that would be
annoying and perhaps a reason to leave this off until later.

Perhaps in the short-term, we could add output like
  First differing byte at index 0
if others think that could be useful.

I'm quite surprised I didn't notice the first bytes differed (as you
can tell from my example), so I personally would have been helped out
by such a thing.

>
> Although I don't know exactly how we can produce this output. I was
> using hex_dump_to_buffer to produce the hexdump, so maybe I need to
> change the strategy to generate the hexdump.

Indeed, we'd probably have to write our own code to do this.
I think it might be reasonable to stick with the code as-is so we can
just reuse hex_dump_to_buffer.
We'd then be able to think about the format more and bikeshed without
blocking this patch.

But note: we could leverage string_stream to build up the output a bit
more easily than you might expect.
Here's a terrible first pass that you can paste into kunit-example-test.c

#include "string-stream.h"

static void diff_hex_dump(struct kunit *test, const u8 *a, const u8 *b,
                          size_t num_bytes, size_t row_size)
{
        size_t i;
        struct string_stream *stream1 = alloc_string_stream(test, GFP_KERNEL);
        struct string_stream *stream2 = alloc_string_stream(test, GFP_KERNEL);

        for (i = 0; i < num_bytes; ++i) {
                if (i % row_size) {
                        string_stream_add(stream1, " ");
                        string_stream_add(stream2, " ");
                } else {
                        string_stream_add(stream1, "\n> ");
                        string_stream_add(stream2, "\n> ");
                }

                if (a[i] == b[i]) {
                        string_stream_add(stream1, "%02x", a[i]);
                        string_stream_add(stream2, "%02x", b[i]);
                } else {
                        string_stream_add(stream1, "<%02x>", a[i]);
                        string_stream_add(stream2, "<%02x>", b[i]);
                }
        }
        string_stream_add(stream1, "\nwant");
        string_stream_append(stream1, stream2);

        kunit_info(test, "got%s\n", string_stream_get_string(stream1));
}


static void example_hex_test(struct kunit *test) {
        const u8 a1[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xde,
0xad, 0xbe, 0xef};
        const u8 a2[] = {0x1, 0x3, 0x2, 0x4, 0x5, 0x6, 0x7, 0xde,
0xad, 0xbe, 0xef};

        diff_hex_dump(test, a1, a2, sizeof(a1), 8);
}

It produces the following output:
    # example_hex_test: got
> 01 <02> <03> 04 05 06 07 de
> ad be ef
want
> 01 <03> <02> 04 05 06 07 de
> ad be ef

It doesn't handle re-aligning the other bytes as you'd pointed out above.

>
> I guess the KASAN approach could be easier to implement. But I guess it
> can turn out to be a little polluted if many bytes differ. For example:
>
> dst:
> 00000000: 33 31 31 31 31 31 31 31 31 31 8e 31 33 0a 60 12
>            ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^
> 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
>                        ^
> result->expected:
> 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
>            ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^
> 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00
>                        ^
>
> I don't know exactly with option I lean.

Agreed, it doesn't scale up too well when pointing out >1 buggy bytes.

Re: [PATCH 2/3] kunit: add KUnit array assertions to the example_all_expect_macros_test

[Daniel Latypov]

On Tue, Aug 2, 2022 at 12:00 PM Maíra Canal <mairacanal@riseup.net> wrote:
> > I don't know how we make a maximally fool-proof version of this macro :\
>
> This is a hard one also. I believe that use KUNIT_EXPECT_ARREQ(test,
> expected, expected, sizeof(expected)); is more compliant to the
> memcpy/memset/memcmp signature. Moreover, this problem also occur for
> the KUNIT_EXPECT_EQ(test, memcmp(expected, expected, sizeof(expected)), 0);
>
> I believe that the number of array elements will make it easier for
> users to avoid mistakes.

Actually, another idea: perhaps KUNIT_EXPECT_MEMEQ?
I think that might be clearer in terms of the semantics and people
could more easily infer the right unit (bytes).

Daniel