From: "Rémi Bernon" Subject: Re: [PATCH v3 4/6] ntoskrnl.exe/tests: Add some HidP_Get*Caps tests. Message-Id: <1b6a0882-3c2a-bd6b-824a-97e53c75c451@codeweavers.com> Date: Tue, 8 Jun 2021 23:37:47 +0200 In-Reply-To: References: <20210604091220.265735-1-rbernon@codeweavers.com> <20210604091220.265735-4-rbernon@codeweavers.com> <6360e559-d4a7-7298-0246-e0919221a543@codeweavers.com> <25454ec9-a72d-0139-82d2-3d57c76e7344@codeweavers.com> <9d6b422b-3c57-8825-ac64-c5f04d63c1a5@codeweavers.com> 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. -- Rémi Bernon