[PATCH v3 4/6] ntoskrnl.exe/tests: Add some HidP_Get*Caps tests.
Rémi Bernon
rbernon at codeweavers.com
Tue Jun 8 16:33:58 CDT 2021
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.
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list