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

[Zebediah Figura]

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 );
@@ -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) );

[PATCH 2/2] dinput/tests: Set the callback event after setting "impl->invoked".

[Zebediah Figura]

Fixes an intermittent test failure.

Signed-off-by: Zebediah Figura 
---
 dlls/dinput/tests/hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/dinput/tests/hotplug.c b/dlls/dinput/tests/hotplug.c
index 2d000317c9f..97e9e69fa9f 100644
--- a/dlls/dinput/tests/hotplug.c
+++ b/dlls/dinput/tests/hotplug.c
@@ -526,8 +526,8 @@ static HRESULT WINAPI controller_handler_Invoke( IEventHandler_RawGameController
     trace( "iface %p, sender %p, controller %p\n", iface, sender, controller );

ok( sender == NULL, "got sender %p\n", sender );
-    SetEvent( impl->event );
     impl->invoked = TRUE;
+    SetEvent( impl->event );

return S_OK;
 }

[PATCH 2/2] dinput/tests: Set the callback event after setting "impl->invoked".

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=113957

Your paranoid android.

=== w1064_tsign (64 bit report) ===

dinput:
hotplug.c:926: Test failed: 1: controller removed handler not invoked
hotplug.c:926: Test failed: 1: controller removed handler not invoked

[Rémi Bernon]

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 don't think this one is failing so often.
> @@ -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.

[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.

[Rémi Bernon]

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
> > >
> > > 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.

I don't think interactive tests are useful, especially not here where 
there's no need for user input.
> 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.

[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
> > > >
> > > > 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.
>
> 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.