From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH v3 4/6] ntoskrnl.exe/tests: Add some HidP_Get*Caps tests.
Message-Id: <25454ec9-a72d-0139-82d2-3d57c76e7344@codeweavers.com>
Date: Tue, 8 Jun 2021 16:27:56 -0500
In-Reply-To: <fd2a3c86-2a02-13dc-6031-da5a1e5815ba@codeweavers.com>
References: <20210604091220.265735-1-rbernon@codeweavers.com> <20210604091220.265735-4-rbernon@codeweavers.com> <6360e559-d4a7-7298-0246-e0919221a543@codeweavers.com> <fd2a3c86-2a02-13dc-6031-da5a1e5815ba@codeweavers.com>

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");
}

Just brainstorming, but some apparent advantages:

* easy to read,

* easy to reuse debugstr_some_struct() in other places, including 
implementation (granted, we may need to copy code around, but that's 
still code sharing ultimately. Personally I'd like to see Wine be more 
amenable to adding internal helpers, though.)

* less risk of making copy-paste errors or accidentally skipping elements,

* makes dealing with todo_wine easier, including partial todo_wine,

* no need to do the __LINE__ hack.

One thing that would be nice to have, while we're at it, is a 
debugstr_bytes() helper; maybe also debugstr_ints(). That'd be nice in 
general, but could also serve as a poor man's debugstr_some_struct().

Maybe I'll start workshopping some of these; apparently 2021 is the year 
that I improve our testing infrastructure...

> I can wrap the lines though ;)

That would be nice ;-)