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

[Zebediah Figura (she/her)]

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
> >
> > For examples of failures:
> >
> > http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n...
> >
> > http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f...
> >
> > 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.

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.
> > @@ -2343,12 +2342,11 @@ 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 ), 
> > "expected identical reports\n" );
> >           send_hid_input( file, expect_small, sizeof(expect_small) );
>
> I'm not completely sure what this second one is trying to achieve, but 
> probably comparing the reports there makes indeed little sense, as the 
> polling frequency is increased a lot, and they will likely often differ.

Given 12dc52cd4a, it does strike me as somewhat unnecessary to test more 
than one report.