[PATCH 1/2] dinput/tests: Do not test that reports are identical when polling.

[Zebediah Figura (she/her)]

On 5/2/22 18:14, Rémi Bernon wrote:
> On 5/3/22 01:09, Zebediah Figura (she/her) wrote:
>> On 5/2/22 17:06, Rémi Bernon wrote:
>>> On 5/2/22 23:48, Zebediah Figura wrote:
>>>> This is dependent on timing, and currently fails occasionally both 
>>>> on Windows
>>>> and Wine.
>>>>
>>>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>>>> ---
>>>> For examples of failures:
>>>>
>>>> http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_newtb-w8/dinput:hid.html 
>>>>
>>>> http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_fgtb-debian11-win32/dinput:hid.html 
>>>>
>>>>
>>>>   dlls/dinput/tests/hid.c | 6 ++----
>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c
>>>> index 4e107625064..1162f154f8e 100644
>>>> --- a/dlls/dinput/tests/hid.c
>>>> +++ b/dlls/dinput/tests/hid.c
>>>> @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE 
>>>> async_file, int report_id, BOOL polle
>>>>           ok( ret, "GetOverlappedResult failed, last error %lu\n", 
>>>> GetLastError() );
>>>>           ok( value == (report_id ? 3 : 4), "GetOverlappedResult 
>>>> returned length %lu, expected %u\n",
>>>>               value, (report_id ? 3 : 4) );
>>>> -        /* first report should be ready and the same */
>>>> +        /* first report should be ready */
>>>>           ret = GetOverlappedResult( async_file, &overlapped, 
>>>> &value, FALSE );
>>>>           ok( ret, "GetOverlappedResult failed, last error %lu\n", 
>>>> GetLastError() );
>>>>           ok( value == (report_id ? 3 : 4), "GetOverlappedResult 
>>>> returned length %lu, expected %u\n",
>>>>               value, (report_id ? 3 : 4) );
>>>>           ok( memcmp( report, buffer + caps.InputReportByteLength, 
>>>> caps.InputReportByteLength ),
>>>>               "expected different report\n" );
>>>> -        ok( !memcmp( report, buffer, caps.InputReportByteLength ), 
>>>> "expected identical reports\n" );
>>>>           value = 10;
>>>>           SetLastError( 0xdeadbeef );
>>>
>>>
>>> This defeats a bit the purpose of the test, which is to validate that 
>>> in polled mode all pending reads should be completed at once when 
>>> device poll completes, whereas in non-polled mode, only one pending 
>>> read should complete for each received report.
>>
>> I figured as much, and my inclination is that the test just isn't 
>> worth keeping around in that case.
>>
>> Perhaps it can be marked interactive instead.
>>
> 
> 
> I don't think interactive tests are useful, especially not here where 
> there's no need for user input.

I've found interactive tests to be useful in DirectShow and ws2_32, for 
cases where it's difficult to impossible to write reliable tests, but 
where a "mostly reliable" test is still valuable. In such cases the test 
can't be automated, but it's already usually the case that signing off 
on a patch implies running the appropriate tests for it manually.

Obviously "interactive" is a bit of a misnomer in this case, except 
inasmuch as it means "requires some attention which is not automated".

> 
> 
>> Alternatively, as I'm sitting here trying to brainstorm, maybe we 
>> could, say, set the poll interval to 100 ms, queue 10 reads, and 
>> verify that they all complete in less than 200 ms. That'd also allow 
>> us to avoid infinite waits.
>>
>>> I don't think this one is failing so often.
>>
>> It's not often, but we really shouldn't be letting tests fail at all.
>>
> 
> It actually never failed, only the second one does.
> 

Hmm, I thought I encountered the first one failing locally, but I could 
be wrong. I'll just send a patch to remove the second part for now.