[PATCH v3 4/6] ntoskrnl.exe/tests: Add some HidP_Get*Caps tests.

[Zebediah Figura (she/her)]

On 6/8/21 4:33 PM, Rémi Bernon wrote:
> On 6/8/21 11:27 PM, Zebediah Figura (she/her) wrote:
>> On 6/8/21 4:10 PM, Rémi Bernon wrote:
>>> On 6/8/21 10:59 PM, Zebediah Figura (she/her) wrote:
>>>>> exp->NumberFeatureDataIndices, "unexpected caps 
>>>>> NumberFeatureDataIndices %d, expected %d\n", 
>>>>> caps->NumberFeatureDataIndices, exp->NumberFeatureDataIndices);
>>>>> +}
>>>>
>>>> These are some *really* long lines, and same with the ones below.
>>>>
>>>> I guess it's always nice to see what exactly differs, but maybe it's 
>>>> more worthwhile just to use memcmp()? I don't feel strongly about 
>>>> it, though.
>>>>
>>>
>>> I think memcmp is fine up to the moment where the test breaks. To 
>>> debug the issue it's nice to see what didn't match without having to 
>>> write those long lines yourself (same for debugstr BTW, I'd love to 
>>> have more helpers to dump the various Win32 structs readily available).
>>>
>>> And for instance I don't like the report memcmp very much, because it 
>>> doesn't tell you at all what's wrong with HidP_InitializeReportForID.
>>>
>>> This only needs to be written once and (hopefully) nobody will have 
>>> to look at it again. But then you can use it to check that both 
>>> struct match and have precise info when they don't.
>>>
>>> I would have like to be able to put individual todo_wine to replace 
>>> the additional tests for the partially matching structs, but it was 
>>> not convenient, so instead I'm just going to replace all the checks 
>>> with a single call to these functions when the todo_wine are fixed.
>>
>> Yeah, though on the other hand that's one reason it's nice to use 
>> memcmp - either they match or they don't.
>>
>> Just a bit of extra 2¢: in DirectShow I ended up using memcmp() to 
>> match AM_MEDIA_TYPE. If a test breaks, which is not infrequently, I 
>> temporarily add strmbase_dump_media_type() to check the difference. 
>> Actually I've considered adding an automatic helper like that to 
>> compare_media_types() everywhere, though it'd be nice to have a 
>> debugstr_* type helper instead so that I don't have to turn on 
>> +strmbase to use it.
>>
>> It may be a nice approach in general to structure things like
>>
>> static bool compare_some_struct(const SOME_STRUCT *s, const 
>> SOME_STRUCT *expect)
>> {
>>     if (!memcmp(s, expect, sizeof(*SOME_STRUCT)))
>>         return true;
>>     trace("Expected: %s\n", debugstr_some_struct(expect));
>>     trace("Received: %s\n", debugstr_some_struct(s));
>> }
>>
>> ...
>>
>> {
>>     ok(compare_some_struct(&s, &expect), "Structures didn't match.\n");
>> }
>>
> 
> I don't have good opinion of trace-ing failures, I find that it quickly 
> gets cut, and in general the traces you were interested it are after the 
> cut.

Sorry, I'm not sure I understand what you mean by this; can you please 
clarify?