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?