On Fri, Aug 5, 2022 at 8:18 PM Maíra Canal wrote: > > On 8/5/22 01:44, David Gow wrote: > > On Thu, Aug 4, 2022 at 5:59 AM Maíra Canal wrote: > >> > >> Currently, in order to compare memory blocks 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_MEMEQ and > >> KUNIT_EXPECT_MEMNEQ 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 memory blocks. > >> > >> Other than the style changes, this v3 brings alignment to the bytes, making > >> it easier to identify the faulty bytes. So, on the previous version, the > >> output from a failure would be: > >> [14:27:42] # xrgb8888_to_rgb565_test: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_format_helper_test.c:248 > >> [14:27:42] Expected dst == result->expected, but > >> [14:27:42] dst == > >> [14:27:42] 33 0a <60> 12 00 a8 00 00 <00> 00 8e 6b <33> 0a 60 12 > >> [14:27:42] 00 00 <00> 00 00 a8 <8e> 6b 33 0a 00 00 <00> 00 > >> [14:27:42] result->expected == > >> [14:27:42] 33 0a <61> 12 00 a8 00 00 <01> 00 8e 6b <31> 0a 60 12 > >> [14:27:42] 00 00 <01> 00 00 a8 <81> 6b 33 0a 00 00 <01> 00 > >> > >> Now, with the alignment, the output is: > >> [14:27:42] # xrgb8888_to_rgb565_test: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_format_helper_test.c:248 > >> [14:27:42] Expected dst == result->expected, but > >> [14:27:42] dst == > >> [14:27:42] 33 0a <60> 12 00 a8 00 00 <00> 00 8e 6b <33> 0a 60 12 > >> [14:27:42] 00 00 <00> 00 00 a8 <8e> 6b 33 0a 00 00 <00> 00 > >> [14:27:42] result->expected == > >> [14:27:42] 33 0a <61> 12 00 a8 00 00 <01> 00 8e 6b <31> 0a 60 12 > >> [14:27:42] 00 00 <01> 00 00 a8 <81> 6b 33 0a 00 00 <01> 00 > >> > >> Moreover, on the raw output, there were some indentation problems. Those > >> problems were solved with the use of KUNIT_SUBSUBTEST_INDENT. > >> > >> The first patch of the series introduces the KUNIT_EXPECT_MEMEQ and > >> KUNIT_EXPECT_MEMNEQ. The second patch adds an example of memory block > >> expectations on the kunit-example-test.c. And the last patch replaces the > >> KUNIT_EXPECT_EQ for KUNIT_EXPECT_MEMEQ on the existing occurrences. > >> > >> Best Regards, > >> - Maíra Canal > >> > >> v1 -> v2: https://lore.kernel.org/linux-kselftest/2a0dcd75-5461-5266-2749-808f638f4c50@riseup.net/T/#m402cc72eb01fb3b88d6706cf7d1705fdd51e5da2 > >> > >> - Change "determinated" to "specified" (Daniel Latypov). > >> - Change the macro KUNIT_EXPECT_ARREQ to KUNIT_EXPECT_MEMEQ, in order to make > >> it easier for users to infer the right size unit (Daniel Latypov). > >> - Mark the different bytes on the failure message with a <> (Daniel Latypov). > >> - Replace a constant number of array elements for ARRAY_SIZE() (André Almeida). > >> - Rename "array" and "expected" variables to "array1" and "array2" (Daniel Latypov). > >> > >> v2 -> v3: https://lore.kernel.org/linux-kselftest/20220802212621.420840-1-mairacanal@riseup.net/T/#t > >> > >> - Make the bytes aligned at output. > >> - Add KUNIT_SUBSUBTEST_INDENT to the output for the indentation (Daniel Latypov). > >> - Line up the trailing \ at macros using tabs (Daniel Latypov). > >> - Line up the params to the functions (Daniel Latypov). > >> - Change "Increament" to "Augment" (Daniel Latypov). > >> - Use sizeof() for array sizes (Daniel Latypov). > >> > >> Maíra Canal (3): > >> kunit: Introduce KUNIT_EXPECT_MEMEQ and KUNIT_EXPECT_MEMNEQ macros > >> kunit: Add KUnit memory block assertions to the example_all_expect_macros_test > >> kunit: Use KUNIT_EXPECT_MEMEQ macro > >> > >> .../gpu/drm/tests/drm_format_helper_test.c | 6 +- > >> include/kunit/assert.h | 34 +++++++++ > >> include/kunit/test.h | 76 +++++++++++++++++++ > >> lib/kunit/assert.c | 56 ++++++++++++++ > >> lib/kunit/kunit-example-test.c | 7 ++ > >> net/core/dev_addr_lists_test.c | 4 +- > >> 6 files changed, 178 insertions(+), 5 deletions(-) > >> > >> -- > >> 2.37.1 > >> > >> -- > >> You received this message because you are subscribed to the Google Groups "KUnit Development" group. > >> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > >> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220803215855.258704-1-mairacanal%40riseup.net. > > > > These patches look pretty good to me overall, but I was unable to > > apply v3 to test -- it looks like the mail client has wrapped some > > lines or something... > > > > davidgow@slicestar:~/linux-kselftest$ git am > > ./v3_20220803_mairacanal_introduce_kunit_expect_memeq_and_kunit_expect_memneq_macros.mbx > > Applying: kunit: Introduce KUNIT_EXPECT_MEMEQ and KUNIT_EXPECT_MEMNEQ macros > > error: corrupt patch at line 24 > > Patch failed at 0001 kunit: Introduce KUNIT_EXPECT_MEMEQ and > > KUNIT_EXPECT_MEMNEQ macros > > > > Checkpatch also picks up an issue: > > ERROR: patch seems to be corrupt (line wrapped?) > > #62: FILE: include/kunit/assert.h:255: > > const struct va_format *message, > > > > v2 applied clearnly, so it seems to be specific to v3. > > I'll check this issue and submit a v4. Thank you! > Thanks! > > > > In general, I like the patches, though. While I think there are a few > > places it'd be slightly suboptimale if it's being used to compare more > > structured data, such as the prospect of comparing padding between > > elements, as well as the output formatting not being ideal. It's > > perfect for the cases where memcmp() otherwise would be used, though. > > Do you any take on how to make the output formatting more ideal? > I don't actually think we need to change any of the formatting in this patch, I'm just noting that usinng MEMEQ()/MEMNEQ() might not be the best choice for comparing, e.g., structs (and that comparing their members individually might be better there). _Maybe_ that's something that could be mentioned in the documentation, but I wouldn't change the code at all. Cheers, -- David