[PATCH v3 4/6] ntoskrnl.exe/tests: Add some HidP_Get*Caps tests.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Jun 8 16:39:12 CDT 2021
On 6/8/21 4:37 PM, Rémi Bernon wrote:
> On 6/8/21 11:35 PM, Zebediah Figura (she/her) wrote:
>> 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?
>>
>
> Eh sorry for my bad english, I mean that traces are muted after a being
> printed too many times, and it happens too often when I needed the traces.
Yes, I find the choice of defaults is a bit unfortunate, I'd rather see
muting off by default, and enabled only for things like the testbot
where we really care about report length.
More information about the wine-devel
mailing list